feat: modify account cloning to use transactions#974
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request refactors account cloning from a side-channel persistence model (global memory map + ledger storage) to direct transaction encoding. It adds CloneAccount (single, for <63KB), CloneAccountInit/CloneAccountContinue (multi-transaction for large accounts), and CleanupPartialClone instructions, an in-memory pending-clone tracker, on-chain clone-processing modules, and a chainlink cloner rewritten to use buffer/multi-tx flows. All ledger-based persistence for intermediate account data (AccountModDatas column, persister trait/impls, and related test stubs) was removed. Assessment against linked issues
Out-of-scope changes
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-account-cloner/src/lib.rs`:
- Around line 522-592: build_large_program_txs duplicates the chunking loop
found in build_large_account_txs; extract the common init/continue/is_last
chunking into a helper (e.g., build_large_txs_helper) that takes buffer_pubkey,
program_data (or account_data), fields, blockhash, and an
Option<Vec<Instruction>> finalize_ixs; move calls to Self::clone_init_ix,
Self::clone_continue_ix and self.sign_tx into that helper and have
build_large_program_txs pass Some(finalize_ixs) while build_large_account_txs
passes None, preserving existing logging and behavior for the final chunk.
- Around line 706-710: The loop waiting for slot advancement using current_slot
= self.accounts_db.slot() and while self.accounts_db.slot() == current_slot {
tokio::time::sleep(Duration::from_millis(25)).await; } is an unbounded
busy-wait; change it to a bounded wait by using a timeout or max-wait loop: wrap
the wait in tokio::time::timeout with a sensible Duration (or track start =
Instant::now() and break after a max Duration/iteration count), and handle the
timeout case (return an error or proceed accordingly) so the async task cannot
be blocked indefinitely; ensure you keep the same checks around
self.accounts_db.slot() and tokio::time::sleep but bail out on timeout.
- Line 460: The V4 path currently uses self.accounts_db.slot() directly while
the V1 path precomputes self.accounts_db.slot().saturating_sub(5).max(1) before
finalize, resulting in different remote_slot values; add a concise comment next
to the V4 slot computation (the line calling self.accounts_db.slot() and/or near
the LoaderV4State handling) explaining whether the unadjusted slot for
LoaderV4State is intentional and why it differs from the V1 saturating_sub(5)
behavior, or if it should be aligned—mention both the V1 precompute pattern and
the LoaderV4State remote_slot behavior so future readers understand the
rationale.
In `@magicblock-chainlink/src/cloner/errors.rs`:
- Around line 31-33: The SerializationError(String) variant discards the
original bincode error; replace it so the ClonerError preserves error
structure—either remove SerializationError(String) and rely on the existing
BincodeError(#[from] bincode::Error) variant, or change SerializationError to
wrap bincode::Error with #[from] (e.g., SerializationError(#[from]
bincode::Error)); then update the call site using .map_err(|e|
ClonerError::SerializationError(e.to_string())) to use the ? operator or let the
#[from] conversion produce ClonerError directly (i.e., return Err(e.into())/use
?), so the original error type and source chain are preserved.
In `@programs/magicblock/src/clone_account/common.rs`:
- Around line 131-138: The negative-branch currently does (-delta) as u64 which
panics/overflows for i64::MIN; update the computation of adjusted in the
negative branch to avoid negating delta directly by using delta.unsigned_abs()
(or casting delta to u64 via delta.unsigned_abs()) when calling
auth_lamports.checked_add, and keep the same error handling
(InstructionError::ArithmeticOverflow) if checked_add returns None; locate the
adjusted binding and delta variable in clone_account/common.rs and replace the
negation with a safe unsigned absolute conversion.
In `@programs/magicblock/src/clone_account/process_cleanup.rs`:
- Around line 28-29: The pubkey is being added to PENDING_CLONES too early in
process_clone_account_init (via add_pending_clone) before a fallible call
adjust_authority_lamports; move the add_pending_clone call to the very end of
process_clone_account_init so it runs only after all validations and
state-modifying operations succeed, keeping remove_pending_clone in
process_cleanup_partial_clone (which is intentionally called before
validate_and_get_index) unchanged; update references so add_pending_clone is
invoked after validate_authority, other state updates, and
adjust_authority_lamports completes without error to avoid transient
inconsistent entries in PENDING_CLONES.
In `@programs/magicblock/src/clone_account/process_clone_continue.rs`:
- Around line 83-90: The code currently calls remove_pending_clone(&pubkey) and
ignores its bool return; update process_clone_continue to check that return
value (after the is_pending_clone check) and handle a false result by either
logging a warning/error including the pubkey and context (use ic_msg! or
process_logger) or by returning an Err so callers can observe the race. Locate
the invocation of remove_pending_clone (in process_clone_continue) and add a
conditional: if remove_pending_clone(&pubkey) == false then emit a clear log
message (or propagate an error) indicating the pending clone was already removed
for pubkey to aid debugging of race conditions.
In `@programs/magicblock/src/clone_account/process_clone_init.rs`:
- Around line 95-97: Before allocating the vector, validate total_data_len
against a sensible upper bound (e.g., SOLANA_MAX_ACCOUNT_SIZE or 10 * 1024 *
1024) and ensure total_data_len >= initial_data.len(); if the check fails,
return an appropriate error. Add the check in process_clone_init (where
total_data_len, initial_data and data are used), e.g., validate total_data_len
as u64 or cast-safe comparison, then only perform let mut data = vec![0u8;
total_data_len as usize] and copy_from_slice after the bounds checks pass.
In `@programs/magicblock/src/clone_account/process_clone.rs`:
- Around line 63-64: The calculation of lamports_delta casts u64 values to i64
(variables: fields.lamports, current_lamports, account.borrow().lamports(),
lamports_delta) which can overflow; replace the unchecked cast with a checked
conversion and explicit overflow handling: convert both u64 values to a wider
signed type (e.g., i128) or use TryFrom/checked_sub on u64 and then convert to a
signed type, compute lamports_delta safely, and return/propagate an error if
conversion/overflow occurs instead of allowing wrapping. Ensure the error path
is handled where lamports_delta is used so callers won't operate on a wrapped
value.
In `@programs/magicblock/src/clone_account/process_finalize_buffer.rs`:
- Around line 125-138: Extract the repeated unsafe conversion into a single
helper (e.g., loader_v4_state_to_bytes) and replace the raw pointer logic in
build_loader_v4_data and process_set_program_authority with calls to that
helper; implement loader_v4_state_to_bytes in common.rs to take &LoaderV4State
and header_size (or compute LoaderV4State::program_data_offset() internally),
perform the single unsafe std::slice::from_raw_parts cast once, return a Vec<u8>
or &[u8] as appropriate, and update both functions to extend/concatenate using
the helper to remove duplication while preserving the documented safety comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@programs/magicblock/src/clone_account/common.rs`:
- Around line 117-138: The function validate_remote_slot currently rejects
incoming slots that are less-than-or-equal-to the current (using <=) but the doc
comment and log imply only older (<) are rejected; update the documentation and
log to match the actual semantics: change the doc comment on
validate_remote_slot to say the incoming_remote_slot must be "strictly newer"
(or "not strictly older") and update the ic_msg text to something like "incoming
remote_slot {} is not strictly newer than current {}; rejected" so the message
matches the existing <= check (no code changes to the <= needed if the intent is
to keep idempotency guard).
---
Duplicate comments:
In `@magicblock-account-cloner/src/lib.rs`:
- Around line 513-564: Both build_large_program_txs and build_large_account_txs
duplicate the same chunking pattern (init → multiple continue → final
continue+finalize). Extract that shared logic into a helper (e.g.,
build_chunked_txs or chunk_and_sign_txs) that takes the buffer_pubkey, data
slice (program_data/account_data), total_len, finalize_ixs, blockhash, and
callbacks or enums to produce the init and continue instructions (reuse
clone_init_ix and clone_continue_ix semantics) and then call sign_tx for each
formed transaction; replace the duplicated loops in build_large_program_txs and
build_large_account_txs to invoke the new helper and keep MAX_INLINE_DATA_SIZE,
clone_init_ix, clone_continue_ix, and sign_tx usage consistent.
In `@programs/magicblock/src/clone_account/common.rs`:
- Around line 142-161: No change required: the earlier i64::MIN edge case is
already handled in adjust_authority_lamports by using delta.unsigned_abs() when
delta is negative; leave the function as-is (references: function name
adjust_authority_lamports and the unsigned_abs() usage).
In `@programs/magicblock/src/clone_account/process_clone_init.rs`:
- Around line 95-97: Add an explicit upper-bound guard before the allocation of
vec![0u8; total_data_len as usize] in process_clone_init: check total_data_len
against a safe maximum (e.g., Solana account limit ~10_485_760 or your crate's
MAX_ACCOUNT_DATA_LEN) and return an appropriate error if exceeded; do this prior
to creating data so the allocation cannot request multiple GBs. Ensure you
reference the total_data_len, initial_data and data variables and return a clear
ProgramError/ custom error when the check fails.
In `@programs/magicblock/src/clone_account/process_clone.rs`:
- Around line 63-64: The code casts u64 lamports to i64 which will wrap for
values > i64::MAX; change the conversion to a checked/inf-safe conversion:
convert both fields.lamports and account.borrow().lamports() to a larger signed
type (e.g., i128) or use TryFrom<i64> semantics, compute the delta in that type
(e.g., let lamports_delta_i128 = (fields.lamports as i128) - (current_lamports
as i128)), then validate the result fits the expected range (or return an error)
and only cast to i64 after a successful bounds check; update the code around
current_lamports, fields.lamports and lamports_delta to use this safe conversion
and explicit error handling.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs (1)
111-123: 🧹 Nitpick | 🔵 TrivialEphemeral entries must be present in
account_modsto avoid count-mismatch errorThe guard at line 55 (
accounts_to_mod_len != account_mods_len) fires before the loop, so an ephemeral account included in the instruction accounts but absent fromaccount_modswill produce a misleadingAccountsToModifyNotMatchingAccountModificationserror rather than the more descriptive "skipping ephemeral account" log. The design is sound as long as callers always include ephemeral accounts inaccount_mods, but this invariant is implicit. A comment co-located with the count check (or on the calling side) would make this contract explicit.Additionally, any lamport delta embedded in the skipped modification is silently dropped, which means
lamports_to_debitwill not reflect those changes. This appears intentional (ephemeral accounts are ER-local and shouldn't affect the authority balance), but is worth a comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs` around lines 111 - 123, The pre-loop length check (accounts_to_mod_len != account_mods_len) relies on the invariant that ephemeral accounts appearing in the instruction accounts are also present in account_mods; add a concise comment next to that check documenting this contract and why ephemeral entries must be included (so the count comparison is valid). Also add a short comment at the is_ephemeral(account) branch (and/or near lamports_to_debit usage) stating that skipped ephemeral account modifications are intentionally dropped and therefore any lamport delta in account_mods for ephemeral accounts will not affect lamports_to_debit or authority balances. Reference the symbols: accounts_to_mod_len != account_mods_len, is_ephemeral, account_mods, lamports_to_debit, transaction_context.get_key_of_account_at_index, and the ic_msg! log.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@programs/magicblock/src/magicblock_processor.rs`:
- Around line 120-189: The new clone-related match arms (CloneAccount,
CloneAccountInit, CloneAccountContinue, CleanupPartialClone,
FinalizeProgramFromBuffer, SetProgramAuthority, FinalizeV1ProgramFromBuffer)
currently pass &signers to their handlers (process_clone_account,
process_clone_account_init, process_clone_account_continue,
process_cleanup_partial_clone, process_finalize_program_from_buffer,
process_set_program_authority, process_finalize_v1_program_from_buffer) while
existing arms like ModifyAccounts and ScheduleCommit pass signers by value; pick
one convention and make it consistent: either change these match-site calls to
pass signers (remove &), or change the handler function signatures to accept
&Signers for the older handlers—update all affected function signatures and
their call-sites together so every handler uses the same signers ownership
convention.
---
Outside diff comments:
In `@programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs`:
- Around line 111-123: The pre-loop length check (accounts_to_mod_len !=
account_mods_len) relies on the invariant that ephemeral accounts appearing in
the instruction accounts are also present in account_mods; add a concise comment
next to that check documenting this contract and why ephemeral entries must be
included (so the count comparison is valid). Also add a short comment at the
is_ephemeral(account) branch (and/or near lamports_to_debit usage) stating that
skipped ephemeral account modifications are intentionally dropped and therefore
any lamport delta in account_mods for ephemeral accounts will not affect
lamports_to_debit or authority balances. Reference the symbols:
accounts_to_mod_len != account_mods_len, is_ephemeral, account_mods,
lamports_to_debit, transaction_context.get_key_of_account_at_index, and the
ic_msg! log.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-account-cloner/src/lib.rs`:
- Around line 103-107: The send_tx function currently indexes tx.signatures[0],
which will panic if the signatures vec is empty; change send_tx to safely access
the first signature via tx.signatures.first() and return a descriptive
ClonerResult error (instead of panicking) when no signature is present before
calling self.tx_scheduler.execute(tx). Ensure the error message references the
send_tx context and the missing signature, and keep the return type as
ClonerResult<Signature>.
In `@programs/magicblock/src/clone_account/common.rs`:
- Around line 174-183: In get_deploy_slot, detect when
invoke_context.get_sysvar_cache().get_clock() returns None and emit a warning
(e.g., using the project logging macro like warn! or ic_msg!) before returning 0
so a missing sysvar cache is visible; update the logic around
get_sysvar_cache/get_clock in the get_deploy_slot function to log a clear
message including context (e.g., "sysvar cache missing when computing deploy
slot") on the None path and then return 0 as the fallback.
---
Duplicate comments:
In `@magicblock-account-cloner/src/lib.rs`:
- Around line 682-686: The loop that waits for a slot change using
block.subscribe() silently spins when recv().await returns Err because the
sender was dropped; update the wait loop in the code that sets current_slot and
calls self.block.subscribe()/block_updated.recv().await to handle the Err case
explicitly (break the loop or return) instead of discarding it, and add a hard
timeout/fallback (e.g., use a tokio timeout or a small sleep) so the task does
not busy-loop if the channel is closed or no event arrives; specifically modify
the branch around current_slot, block_updated, and recv().await to match on the
Result, break on Err, and enforce a bounded wait period before re-checking
self.block.load().slot.
instead of using side storage (stashing accounts in the ledger and then resolving them in runtime) we now embed accounts to clone directly into transactions thus greatly simplifying the whole cloning flow, and enabling full replication of ledger
those should prevent account overwrites due to racy account udpates from chainlink
c182cf5 to
ea3419f
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-account-cloner/src/lib.rs`:
- Around line 682-686: The loop waiting for a slot advance ignores errors from
block_updated.recv().await which causes a spin when the sender is dropped;
update the wait loop in the code that sets current_slot, obtains block_updated
via self.block.subscribe(), and then loops while self.block.load().slot ==
current_slot to handle the Result from block_updated.recv().await (e.g., match
or if let) and break out of the loop on Err (channel closed) instead of
discarding the error so the task stops rather than spinning.
In `@magicblock-account-cloner/src/util.rs`:
- Around line 12-16: The doc comment for derive_buffer_pubkey is misleading:
change "Uses validator_authority as owner" to explain that
validator_authority_id() is used as the program_id / PDA namespace for
Pubkey::find_program_address rather than the account owner; update the comment
above derive_buffer_pubkey to state that the PDA is derived in the
validator_authority_id() namespace (program ID) so readers understand why
Pubkey::find_program_address is called with validator_authority_id().
In `@programs/magicblock/src/clone_account/mod.rs`:
- Around line 50-56: PENDING_CLONES is an in-memory RwLock<HashSet<Pubkey>> that
is lost on validator restart, making partial clones unrecoverable; fix by either
persisting/restoring pending entries or making Init idempotent. Specifically,
update the handling around PENDING_CLONES, CloneAccountInit,
CloneAccountContinue(is_last=true) and CleanupPartialClone: either (A) persist
each Pubkey added/removed by CloneAccountInit/Continue/CleanupPartialClone to
durable storage and restore them into PENDING_CLONES on startup, or (B) change
CloneAccountInit to detect a partially-initialized target (instead of returning
NoPendingClone on subsequent Continue) and overwrite/reinitialize the account
state so re-running CloneAccountInit is safe; ensure NoPendingClone paths are
adjusted to trigger CleanupPartialClone semantics when appropriate.
In `@programs/magicblock/src/clone_account/process_cleanup.rs`:
- Around line 51-56: The code unsafely casts current_lamports: do not cast u64
to i64 unconditionally in process_cleanup; instead validate that
account.borrow().lamports() <= i64::MAX and return an error if not, then compute
let lamports_delta = -(current_lamports as i64) and call
close_buffer_account(account); finally call adjust_authority_lamports(auth_acc,
lamports_delta)?; reference the current_lamports variable and the
adjust_authority_lamports and close_buffer_account calls so you update the
cast/negation safely and fail early on out-of-range values.
In `@programs/magicblock/src/clone_account/process_finalize_buffer.rs`:
- Around line 86-89: The subtraction casts from u64 to i64 for
prog_current_lamports and buf_lamports can overflow; in the function handling
finalize buffer (look for prog_lamports, prog_current_lamports, buf_lamports in
process_finalize_buffer.rs) replace the naive u64→i64 casts with a safe
conversion: either convert all three values to a wider signed type (e.g., i128)
before computing lamports_delta, or use i64::try_from(...) (or TryInto) on
prog_current_lamports and buf_lamports and propagate an error on failure; ensure
the resulting lamports_delta type matches downstream usage and adjust
types/returns accordingly.
- Line 86: Replace the hardcoded Rent::default() usage with reading the Rent
sysvar from the sysvar cache (same approach used for Clock in get_deploy_slot())
so the rent-exempt calculation uses the runtime rent parameters; e.g., obtain
the Rent sysvar via the sysvar cache (Rent::get() or Rent::from_account_info on
the provided rent sysvar account) and call minimum_balance(program_data.len())
on that Rent value instead of Rent::default(); apply this change in
process_finalize_buffer (replace the let prog_lamports = Rent::default()...
line) and the analogous location in process_finalize_v1_buffer.
In `@programs/magicblock/src/clone_account/process_finalize_v1_buffer.rs`:
- Around line 119-124: The current computation of lamports_delta casts u64
values (prog_current, data_current, buf_current) to i64 which can silently wrap;
update the code in process_finalize_v1_buffer.rs to safely convert these u64s
before arithmetic (either convert all operands to a wider signed type like i128
and compute lamports_delta as i128, or use i64::try_from(...) / TryInto and
propagate an error if conversion fails) and then handle any required type
narrowing with explicit checks; ensure you reference
prog_acc.borrow().lamports(), data_acc.borrow().lamports(),
buf_acc.borrow().lamports(), and the variables prog_lamports and data_lamports
when changing the arithmetic so no silent wrap can occur.
In `@programs/magicblock/src/clone_account/process_set_authority.rs`:
- Around line 60-63: The unsafe ptr::read_unaligned into LoaderV4State is
unsound because LoaderV4Status is a repr(u64) enum with constrained
discriminants; replace that read with explicit, checked reads from data: parse
slot from data.get(0..8) into u64, parse authority bytes from data.get(8..40)
into [u8;32] and convert with Pubkey::from, parse the status discriminant from
data.get(40..48) into u64 (status_disc) and match it to
LoaderV4Status::{Retracted,Deployed,Finalized}; on any slice/try_from failure or
an invalid discriminant log via ic_msg!(invoke_context, ...) and return
Err(InstructionError::InvalidAccountData) instead of using unwraps, then
construct LoaderV4State { slot, authority_address_or_next_version, status } from
those validated pieces.
---
Duplicate comments:
In `@magicblock-account-cloner/src/lib.rs`:
- Around line 103-107: The send_tx function assumes tx.signatures[0] exists and
will panic on empty signatures; update send_tx to validate that
Transaction.signatures is non-empty before indexing (or handle the case where
signatures may be absent), return an appropriate ClonerResult error if empty,
and only call tx.signatures[0] after the check; ensure this logic is applied
around the tx_scheduler.execute(tx).await? flow so you don't extract the
signature before confirming presence (refer to send_tx, Transaction.signatures,
and tx_scheduler.execute).
In `@programs/magicblock/src/clone_account/common.rs`:
- Around line 177-183: get_deploy_slot currently returns 0 silently when
invoke_context.get_sysvar_cache().get_clock() is None; update get_deploy_slot to
log a warning before falling back so the 5-slot cooldown miss is visible. Inside
get_deploy_slot (referencing InvokeContext, get_sysvar_cache, get_clock), detect
the None branch and emit a warning (e.g., warn! or ic_msg!) with a clear message
indicating the clock sysvar was unavailable and that slot 0 is being used as a
fallback, then return 0; keep the existing saturating_sub(5) logic for the Some
branch.
In `@programs/magicblock/src/clone_account/process_clone_continue.rs`:
- Around line 83-84: The call to remove_pending_clone(&pubkey) in the is_last
branch silently ignores the boolean result; update the is_last handling in
process_clone_continue (where remove_pending_clone, pubkey and is_last are used)
to check the return value and handle the false case explicitly — e.g., log a
warning or informational message including the pubkey (and context) when
remove_pending_clone returns false to surface concurrent-removal races, or
otherwise take the appropriate alternate action instead of discarding the
result.
In `@programs/magicblock/src/clone_account/process_clone_init.rs`:
- Around line 95-97: Guard the unbounded allocation by validating total_data_len
before creating data: check total_data_len (u32) against a defined
MAX_ACCOUNT_SIZE (e.g., 10 * 1024 * 1024) and return an error if it exceeds that
limit; only then safely cast to usize and allocate vec![0u8; total_data_len as
usize]. Apply this check near where total_data_len, initial_data, and data are
used (the allocation block) and ensure you also verify initial_data.len() <=
total_data_len to avoid slice panics.
In `@programs/magicblock/src/clone_account/process_clone.rs`:
- Around line 63-64: The code casts u64 lamports to i64 which can overflow;
update the computation in process_clone.rs (symbols: current_lamports,
lamports_delta, fields.lamports) to use a wider signed type or checked
arithmetic instead of a direct u64→i64 cast — e.g., convert both values to i128
(let current_lamports = account.borrow().lamports() as i128; let lamports_delta
= fields.lamports as i128 - current_lamports) or use checked_sub/checked_add on
u64 and handle overflow with an error, ensuring no unchecked downcast to i64
occurs.
In `@programs/magicblock/src/magicblock_processor.rs`:
- Around line 124-193: The new clone-related match arms (CloneAccount,
CloneAccountInit, CloneAccountContinue, CleanupPartialClone) are passing
&signers while older arms pass signers by value; change the calls to pass
signers by value (remove the & from &signers) when invoking
process_clone_account, process_clone_account_init,
process_clone_account_continue, and process_cleanup_partial_clone so the
ownership convention matches existing arms, and if any callee signature (e.g.,
process_clone_account*) expects a reference, update the callee to take ownership
(or vice versa) so the call sites and function signatures are consistent.

Summary
Compatibility
Testing
Checklist
Summary by CodeRabbit
New Features
Refactors