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. |
📝 WalkthroughWalkthroughThe PR modifies the Assessment against linked issues
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-api/src/fund_account.rs (1)
25-39: 🧹 Nitpick | 🔵 Trivial
fund_account_with_dataunconditionally zeros existing account data — the root cause of issue#984.This function's "exists" branch (line 33) resets data to zeroes even for pre-existing accounts. The PR correctly bypasses it for
MagicContext, but any future caller relying on data preservation for a sized account will hit the same trap. Consider either renaming the function to clarify it always resets data, or adding apreserve_dataflag / a separatefund_account_preserve_datahelper to prevent repeat bugs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-api/src/fund_account.rs` around lines 25 - 39, The function fund_account_with_data currently overwrites existing account data by calling set_data(vec![0; size]) in the exists branch (accountsdb.get_account -> acc.set_data), which causes unintended zeroing; update this function to either (A) accept a preserve_data: bool parameter and only call acc.set_data when preserve_data is false or when the existing data length differs from size, or (B) split into two helpers (fund_account_with_zeroed_data and fund_account_preserve_data) and move the zeroing behavior into the former; ensure callers of fund_account_with_data (and any tests) are updated to use the appropriate new API and keep the insert_account call (accountsdb.insert_account) unchanged.
🤖 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-api/src/fund_account.rs`:
- Around line 85-89: The AccountSharedData creation for the MagicContext uses
the system program as owner; update the owner argument in the
AccountSharedData::new call that uses CONTEXT_LAMPORTS and MagicContext::SIZE to
pass &magic_program::id() (the magic program ID) instead of &Default::default()
so the account owner matches the tests and the ephemeral vault account.
---
Outside diff comments:
In `@magicblock-api/src/fund_account.rs`:
- Around line 25-39: The function fund_account_with_data currently overwrites
existing account data by calling set_data(vec![0; size]) in the exists branch
(accountsdb.get_account -> acc.set_data), which causes unintended zeroing;
update this function to either (A) accept a preserve_data: bool parameter and
only call acc.set_data when preserve_data is false or when the existing data
length differs from size, or (B) split into two helpers
(fund_account_with_zeroed_data and fund_account_preserve_data) and move the
zeroing behavior into the former; ensure callers of fund_account_with_data (and
any tests) are updated to use the appropriate new API and keep the
insert_account call (accountsdb.insert_account) unchanged.
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 `@magicblock-api/src/fund_account.rs`:
- Line 82: The code currently calls .expect() in fund_magic_context which can
panic if insert_account failed (its Result was discarded with let _ = ...);
change fund_magic_context(accountsdb: &AccountsDb) to return ApiResult<()> (or
the crate's error type), stop discarding the Result from insert_account (use ?
to propagate errors), and replace the .expect("magic context should have been
created") retrieval with a fallible call that uses ? to return a proper error
instead of panicking; update callers to handle the ApiResult<()> accordingly
(references: function fund_magic_context, the earlier insert_account call, and
the retrieval after fund_account_with_data).
| @@ -30,7 +30,6 @@ pub(crate) fn fund_account_with_data( | |||
| ) { | |||
| let account = if let Some(mut acc) = accountsdb.get_account(pubkey) { | |||
There was a problem hiding this comment.
I meant performing an early return here, so that we don't modify existing account here, for validator authority we ideally need to a way to keep the balance intact, e.g. when an account is removed from accountsdb/closed the lamports should be refunded to validator keypair (as it acted as sponsor during account creation), but that's irrelevant to this task, and it's not like it's physically possible to exhaust u64::MAX lamports in any feasable amount of time.
Summary
Fixes MagicContext reset on restart.
Compatibility
Testing
Checklist
Summary by CodeRabbit