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
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 |
![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.
| AccountSharedData::new( | ||
| CONTEXT_LAMPORTS, | ||
| MagicContext::SIZE, | ||
| &Default::default(), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== MagicContext struct and SIZE definition ==="
rg -n "struct MagicContext" -A 25 --type rust magicblock-api/
echo ""
echo "=== MagicContext SIZE constant ==="
rg -n "const SIZE" --type rust magicblock-api/
echo ""
echo "=== Git diff to see previous implementation ==="
git diff HEAD -- magicblock-api/src/fund_account.rs | head -100
echo ""
echo "=== How account owner is set in similar contexts ==="
rg -n "AccountSharedData::new" -B 2 -A 2 --type rust magicblock-api/ | head -50
echo ""
echo "=== Check MAGIC_CONTEXT_PUBKEY usage and owner validation ==="
rg -n "MAGIC_CONTEXT_PUBKEY" -C 5 --type rust magicblock-api/Repository: magicblock-labs/magicblock-validator
Length of output: 4237
🏁 Script executed:
#!/bin/bash
echo "=== Search for MagicContext definition across all crates ==="
find . -type f -name "*.rs" | xargs rg "struct MagicContext" -A 20 2>/dev/null | head -100
echo ""
echo "=== Search for SIZE constant in MagicContext ==="
rg "MagicContext::SIZE" --type rust -B 5 -A 2
echo ""
echo "=== Check if magic_program crate defines MagicContext ==="
find . -path "*/magic_program*" -name "*.rs" | head -20
echo ""
echo "=== Look for explicit owner validation on MagicContext account ==="
rg "MAGIC_CONTEXT_PUBKEY" --type rust -A 10 | rg -i "owner|is_signer|delegated"
echo ""
echo "=== Check MagicContext implementation and serialization ==="
rg "impl MagicContext" -A 30 --type rust | head -80Repository: magicblock-labs/magicblock-validator
Length of output: 7139
🏁 Script executed:
#!/bin/bash
echo "=== MAGIC_CONTEXT_SIZE value ==="
rg "MAGIC_CONTEXT_SIZE" --type rust -B 2 -A 2 | head -40
echo ""
echo "=== Test setup pattern - MagicContext account owner ==="
rg "MAGIC_CONTEXT_PUBKEY" --type rust -B 5 -A 5 | grep -A 8 "AccountSharedData::new.*MagicContext::SIZE"
echo ""
echo "=== fund_ephemeral_vault - shows expected owner pattern ==="
rg -n "set_owner\|magic_program::ID" --type rust magicblock-api/src/fund_account.rs
echo ""
echo "=== Check all locations where MagicContext account is created ==="
rg "AccountSharedData::new.*MagicContext::SIZE" -B 3 -A 3 --type rustRepository: magicblock-labs/magicblock-validator
Length of output: 5083
🏁 Script executed:
#!/bin/bash
echo "=== Check program initialization to see how MagicContext owner should be set ==="
rg "magic_program::ID\|magic_program::id()" --type rust magicblock-api/src/ -B 3 -A 3
echo ""
echo "=== Check if there's any comment or documentation about MagicContext ownership ==="
rg -n "fund_magic_context\|MagicContext" magicblock-api/src/fund_account.rs | head -30
echo ""
echo "=== Verify the full fund_magic_context function ==="
sed -n '72,94p' magicblock-api/src/fund_account.rsRepository: magicblock-labs/magicblock-validator
Length of output: 1101
Fix the MagicContext account owner—it should be the magic program, not the system program.
The account is currently created with &Default::default() (system program) as owner. However, test code in process_schedule_commit_tests.rs creates the same account with &crate::id() (magic_program), and the ephemeral vault account is explicitly set to magic_program::ID. Change line 88 to use &magic_program::id() instead of &Default::default().
Current code (incorrect)
AccountSharedData::new(
CONTEXT_LAMPORTS,
MagicContext::SIZE,
&Default::default(),
)
🤖 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 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.
| } | ||
|
|
||
| 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?
2 participants
Summary
Fixes MagicContext reset on restart.
Compatibility
Testing
Checklist
Summary by CodeRabbit