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 changes how the MAGIC_CONTEXT_PUBKEY account is funded and created in magicblock-api/src/fund_account.rs. fund_account_with_data now returns early if the target account already exists instead of overwriting it; when missing, it creates a new AccountSharedData. fund_magic_context introduces a CONTEXT_LAMPORTS constant (used instead of u64::MAX), replaces an unwrap with expect("magic context should have been created"), and explicitly sets the account owner on the created magic context. Function signatures are unchanged. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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).
magicblock-api/src/fund_account.rs
Outdated
| @@ -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, for validator authority we ideally need 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.
There was a problem hiding this comment.
Makes sense, done
There was a problem hiding this comment.
♻️ Duplicate comments (1)
magicblock-api/src/fund_account.rs (1)
78-85:⚠️ Potential issue | 🟠 Major
.expect()in production code remains a MAJOR issue — changing from.unwrap()to.expect()is insufficient.Although the
.expect()is logically unreachable after the newfund_account_with_data(the account is guaranteed to exist), the panic is still a silent discard of any storage failure frominsert_accounton line 35. If that insert fails,get_accountreturnsNoneand this panics at startup. The function should returnApiResult<()>and propagate errors.As per coding guidelines: "Treat any usage of
.unwrap()or.expect()in production Rust code as a MAJOR issue."🤖 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 78 - 85, The code in fund_account.rs panics via accountsdb.get_account(...).expect(...) which must be replaced with proper error propagation: change the surrounding function (e.g., fund_account_with_data or its caller) to return ApiResult<()>, replace the expect on accountsdb.get_account(&magic_program::MAGIC_CONTEXT_PUBKEY) with pattern matching or .ok_or(...) to return a descriptive ApiError if the account is missing, and handle the result of accountsdb.insert_account(...) by propagating any insertion error instead of discarding it; update uses of magic_context.set_delegated and set_owner accordingly after successful retrieval and ensure failures return Err(...) rather than panicking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@magicblock-api/src/fund_account.rs`:
- Around line 78-85: The code in fund_account.rs panics via
accountsdb.get_account(...).expect(...) which must be replaced with proper error
propagation: change the surrounding function (e.g., fund_account_with_data or
its caller) to return ApiResult<()>, replace the expect on
accountsdb.get_account(&magic_program::MAGIC_CONTEXT_PUBKEY) with pattern
matching or .ok_or(...) to return a descriptive ApiError if the account is
missing, and handle the result of accountsdb.insert_account(...) by propagating
any insertion error instead of discarding it; update uses of
magic_context.set_delegated and set_owner accordingly after successful retrieval
and ensure failures return Err(...) rather than panicking.
3 participants
Summary
Fixes MagicContext reset on restart.
Compatibility
Testing
Checklist
Summary by CodeRabbit