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 |
![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.
| } | ||
|
|
||
| pub(crate) fn fund_magic_context(accountsdb: &AccountsDb) { | ||
| fund_account_with_data( |
There was a problem hiding this comment.
I think it's better to tweak the fund_account_with_data method to prevent any mutation if account already exists, all use cases assume that anyway.
There was a problem hiding this comment.
If that is the case, then it is better. Do we want to refill lamports for all of them tho?
There was a problem hiding this comment.
no, in case of EPHEMERAL VAULT account that would be a logic error, so we want to keep the state intact between restarts
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).
| let mut magic_context = accountsdb | ||
| .get_account(&magic_program::MAGIC_CONTEXT_PUBKEY) | ||
| .unwrap(); | ||
| .expect("magic context should have been created"); |
There was a problem hiding this comment.
.expect() in production code is a MAJOR issue per project coding guidelines.
After fund_account_with_data, the account must exist — but only if the underlying insert_account succeeded. Because the result of insert_account is silently discarded on line 37 (let _ = …), a storage failure would cause this expect to panic rather than propagate a meaningful error. The function signature fn fund_magic_context(accountsdb: &AccountsDb) should return ApiResult<()> (or equivalent) so both the insertion and this retrieval can surface errors.
As per coding guidelines: "Treat any usage of .unwrap() or .expect() in production Rust code as a MAJOR issue."
🔧 Sketch of error-propagating refactor
-pub(crate) fn fund_magic_context(accountsdb: &AccountsDb) {
+pub(crate) fn fund_magic_context(accountsdb: &AccountsDb) -> ApiResult<()> {
const CONTEXT_LAMPORTS: u64 = u64::MAX;
- fund_account_with_data(
+ fund_account_with_data_result(
accountsdb,
&magic_program::MAGIC_CONTEXT_PUBKEY,
CONTEXT_LAMPORTS,
MagicContext::SIZE,
- );
+ )?;
let mut magic_context = accountsdb
.get_account(&magic_program::MAGIC_CONTEXT_PUBKEY)
- .expect("magic context should have been created");
+ .ok_or_else(|| /* appropriate ApiError variant */ ...)?;
magic_context.set_delegated(true);
magic_context.set_owner(magic_program::ID);
- let _ = accountsdb
+ accountsdb
.insert_account(&magic_program::MAGIC_CONTEXT_PUBKEY, &magic_context);
+ Ok(())
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-api/src/fund_account.rs` at 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).
2 participants
Summary
Fixes MagicContext reset on restart.
Compatibility
Testing
Checklist
Summary by CodeRabbit