Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
73c6092 to
3c15947
Compare
367c7dc to
5f260c4
Compare
e26900b to
7921abb
Compare
11b31fc to
ffefa44
Compare
d5a0037 to
ff94259
Compare
48f5d00 to
a80df02
Compare
219a70d to
dddc008
Compare
|
Warning Review limit reached
Next review available in: 59 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds the MagicRoot instruction interface and program. It supports authority checks, account patching, finalization, deletion, executable loading, and restricted post-finalize CPI execution. ChangesMagicRoot program
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant MagicRootEntrypoint
participant processor
participant account
participant post_finalize
Caller->>MagicRootEntrypoint: Submit serialized MagicRootInstruction
MagicRootEntrypoint->>processor: Process InvokeContext
processor->>processor: Authorize caller and AUTHORITY
processor->>processor: Decode and dispatch instruction
processor->>account: Apply Patch, Finalize, or Delete
processor->>post_finalize: Execute PostFinalize instructions
post_finalize->>Caller: Invoke forwarded CPI actions
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@programs/magic-root-program/src/account.rs`:
- Around line 15-40: Update the account patch flow around the mutable target
borrow so it is released before borrowing authority account index 0. Keep the
target borrow alive through patch application and balance calculations, then
explicitly drop or scope it before
ctx.transaction_context.accounts().try_borrow_mut(0), allowing target 0 without
AccountBorrowFailed.
- Around line 50-70: Update finalize to copy the account data into an owned
value and explicitly release the account RefMut before constructing the
ProgramCacheEntry or storing it through ctx.program_cache_for_tx_batch. Preserve
the installed flags on successful loading, and either restore the previous flags
on the ProgramCacheEntry::new error path using the existing previous binding or
revise the README wording to describe runtime instruction-level rollback rather
than program-level behavior.
In `@programs/magic-root-program/src/processor.rs`:
- Around line 30-47: Update authorize to explicitly reject calls whose caller_id
equals the MagicRoot program ID before accepting builtin callers, returning
InstructionError::CallDepth and preserving the existing diagnostic pattern. Keep
the existing builtin validation for all other callers so MagicRoot cannot
authorize itself regardless of post_finalize behavior.
- Around line 48-52: Update authorize to validate that the authority account is
an actual transaction signer, not merely the account at index 0. Use the
transaction context’s signer-checking API for the AUTHORITY key and reject
unauthorized or unsigned callers before allowing MagicRoot account mutations.
In `@programs/magic-root-program/src/tests.rs`:
- Around line 43-44: Update the imports in the tests module to bring
ProgramCacheForTxBatch and ProgramRuntimeEnvironments into scope, first
confirming each type’s module path in the vendored runtime crate. Keep the
existing cache initialization in the test unchanged and use the verified
explicit paths or imports.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a02f1198-b204-49a6-9173-9850441942d2
📒 Files selected for processing (11)
Cargo.tomlprograms/magic-root-interface/Cargo.tomlprograms/magic-root-interface/README.mdprograms/magic-root-interface/src/lib.rsprograms/magic-root-program/Cargo.tomlprograms/magic-root-program/README.mdprograms/magic-root-program/src/account.rsprograms/magic-root-program/src/lib.rsprograms/magic-root-program/src/post_finalize.rsprograms/magic-root-program/src/processor.rsprograms/magic-root-program/src/tests.rs
| let mut account = ctx.transaction_context.accounts().try_borrow_mut(target)?; | ||
| account.set_flags(flags); | ||
| if !account.executable() { | ||
| return Ok(()); | ||
| } | ||
| let pubkey = ctx.transaction_context.get_key_of_account_at_index(target)?; | ||
| let entry = ProgramCacheEntry::new( | ||
| ctx.environment_config | ||
| .program_runtime_environments_for_execution | ||
| .get_env_for_execution() | ||
| .clone(), | ||
| account.data(), | ||
| ) | ||
| .map_err(|_| { | ||
| ic_msg!(ctx, "MagicRoot: program load failed {}", pubkey); | ||
| InstructionError::ProgramEnvironmentSetupFailure | ||
| })? | ||
| .into(); | ||
| ctx.program_cache_for_tx_batch.store_modified_entry(*pubkey, entry); | ||
| ic_msg!(ctx, "MagicRoot: finalized program load"); | ||
| Ok(()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
finalize holds the account guard across a mutable use of ctx, and does not roll back flags.
Two problems in this function:
- The
RefMutguard bound at Line 50 has aDropimplementation, so its borrow ofctx.transaction_contextlives until the end of the function. Line 68 needs&mut ctx.program_cache_for_tx_batch. Confirm this compiles; if it does, the guard still holds the account borrow across the cache store for no reason. Copy the data and drop the guard before Line 55. programs/magic-root-program/README.mdLine 28 states that "failed executable loading rolls back the installed flags". Line 51 installs the flags. The error path at Lines 63-66 returns without restoring the previous flags. If the intended rollback is the runtime's instruction-level account rollback, state that in the README instead of describing it as program behavior.
♻️ Proposed fix for the borrow scope
let mut account = ctx.transaction_context.accounts().try_borrow_mut(target)?;
+ let previous = account.flags();
account.set_flags(flags);
if !account.executable() {
return Ok(());
}
+ let data = account.data().to_vec();
+ drop(account);
let pubkey = ctx.transaction_context.get_key_of_account_at_index(target)?;
let entry = ProgramCacheEntry::new(
ctx.environment_config
.program_runtime_environments_for_execution
.get_env_for_execution()
.clone(),
- account.data(),
+ &data,
)The previous binding above supports an explicit flag restore if you choose to implement the rollback in the program rather than document the runtime behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@programs/magic-root-program/src/account.rs` around lines 50 - 70, Update
finalize to copy the account data into an owned value and explicitly release the
account RefMut before constructing the ProgramCacheEntry or storing it through
ctx.program_cache_for_tx_batch. Preserve the installed flags on successful
loading, and either restore the previous flags on the ProgramCacheEntry::new
error path using the existing previous binding or revise the README wording to
describe runtime instruction-level rollback rather than program-level behavior.
Source: Path instructions
| let mut cache = ProgramCacheForTxBatch::default(); | ||
| let environments = ProgramRuntimeEnvironments::default(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
ProgramCacheForTxBatch and ProgramRuntimeEnvironments are not imported.
The use block at Lines 1-14 imports only ProgramCacheEntry and ProgramCacheEntryType from loaded_programs. Lines 43 and 44 name ProgramCacheForTxBatch and ProgramRuntimeEnvironments without a path and without a glob import. The test module does not compile.
🐛 Proposed fix
solana_program_runtime::{
- loaded_programs::{ProgramCacheEntry, ProgramCacheEntryType},
+ loaded_programs::{
+ ProgramCacheEntry, ProgramCacheEntryType, ProgramCacheForTxBatch,
+ ProgramRuntimeEnvironments,
+ },
solana_sbpf::program::BuiltinFunctionDefinition,
with_mock_invoke_context,
},Confirm the module path of each type in the vendored runtime crate before applying the fix.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut cache = ProgramCacheForTxBatch::default(); | |
| let environments = ProgramRuntimeEnvironments::default(); | |
| use solana_program_runtime::{ | |
| loaded_programs::{ | |
| ProgramCacheEntry, ProgramCacheEntryType, ProgramCacheForTxBatch, | |
| ProgramRuntimeEnvironments, | |
| }, | |
| solana_sbpf::program::BuiltinFunctionDefinition, | |
| with_mock_invoke_context, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@programs/magic-root-program/src/tests.rs` around lines 43 - 44, Update the
imports in the tests module to bring ProgramCacheForTxBatch and
ProgramRuntimeEnvironments into scope, first confirming each type’s module path
in the vendored runtime crate. Keep the existing cache initialization in the
test unchanged and use the verified explicit paths or imports.
1 participant
What changed
Added the engine's internal program crates: MagicRoot and v42 calculator
implementations with separate interface crates.
Why
Engine account CRUD needs privileged operations ordinary programs cannot perform,
while callers and tests need instruction schemas without depending on execution
implementations.
Part of #4.
Closes #30.
Impact
magic-root-interfacedefinesMagicRootInstructionand instructioncomposition;
magic-root-programprovides the native entrypoint.Patchapplies field changes and balances lamport deltas against the authority;Finalizeloads executable targets into the transaction program cache.Deletemarks accounts closed, whilePostFinalizeinvokes follow-upinstructions after rejecting immutable writable accounts.
runtime and integration tests.
Reviewer notes
MagicRoot is authority-gated: every invocation must be top-level and signed by
the thread-local
AUTHORITY. Authorization and decoding complete before thetarget account is borrowed.
Follow-up
The engine crate registers MagicRoot and exposes account CRUD through these
instructions upstack.