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
Out-of-scope changes
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
# Conflicts: # test-integration/Cargo.lock
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-committor-service/CLAUDE.md`:
- Around line 24-31: The fenced code block in CLAUDE.md showing the
CommitDelivery tree is missing a language specifier; update the opening fence
(```) to include a language such as rust (e.g., ```rust) so the block has proper
syntax highlighting and consistency with other docs, ensuring only the opening
fence is changed and the block content (CommitDelivery, StateInArgs,
StateInBuffer, DiffInArgs, DiffInBuffer) remains untouched.
- Around line 10-16: The fenced code block showing the BaseTaskImpl hierarchy
lacks a language specifier; update the opening fence to include a language
(e.g., add "rust" so it becomes ```rust) so the block containing BaseTaskImpl
and its entries (Commit(CommitTask), Finalize(FinalizeTask),
Undelegate(UndelegateTask), BaseAction(BaseActionTask)) gets proper syntax
highlighting and consistent formatting.
| ``` | ||
| BaseTaskImpl | ||
| ├── Commit(CommitTask) — commit account state/diff to base layer | ||
| ├── Finalize(FinalizeTask) — finalize a committed account | ||
| ├── Undelegate(UndelegateTask) — undelegate an account | ||
| └── BaseAction(BaseActionTask) — call handler actions (V1/V2) | ||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add language specifier to fenced code block.
For consistency and proper syntax highlighting, add a language specifier to the fenced code block.
📝 Proposed fix
`BaseTaskImpl` is the central enum representing all task types:
-```
+```rust
BaseTaskImpl
├── Commit(CommitTask) — commit account state/diff to base layer
├── Finalize(FinalizeTask) — finalize a committed account🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 10-10: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-committor-service/CLAUDE.md` around lines 10 - 16, The fenced code
block showing the BaseTaskImpl hierarchy lacks a language specifier; update the
opening fence to include a language (e.g., add "rust" so it becomes ```rust) so
the block containing BaseTaskImpl and its entries (Commit(CommitTask),
Finalize(FinalizeTask), Undelegate(UndelegateTask), BaseAction(BaseActionTask))
gets proper syntax highlighting and consistent formatting.
| ``` | ||
| CommitDelivery | ||
| ├── StateInArgs — full account data in instruction args | ||
| ├── StateInBuffer { stage } — data in on-chain buffer | ||
| ├── DiffInArgs { base_account } — diff against base in args | ||
| └── DiffInBuffer { stage, base_account } — diff in buffer | ||
| ``` | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add language specifier to fenced code block.
For consistency and proper syntax highlighting, add a language specifier to the fenced code block.
📝 Proposed fix
Delivery strategy is an enum:
-```
+```rust
CommitDelivery
├── StateInArgs — full account data in instruction args
├── StateInBuffer { stage } — data in on-chain buffer🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 24-24: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 24-24: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-committor-service/CLAUDE.md` around lines 24 - 31, The fenced code
block in CLAUDE.md showing the CommitDelivery tree is missing a language
specifier; update the opening fence (```) to include a language such as rust
(e.g., ```rust) so the block has proper syntax highlighting and consistency with
other docs, ensuring only the opening fence is changed and the block content
(CommitDelivery, StateInArgs, StateInBuffer, DiffInArgs, DiffInBuffer) remains
untouched.
3 participants
Summary
Fixes MagicContext reset on restart.
Compatibility
Testing
Checklist
Summary by CodeRabbit
Refactor
Documentation