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
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Warning
|
| Layer / File(s) | Summary |
|---|---|
Instruction contract and composition Cargo.toml, programs/magic-root-interface/... |
Defines MagicRootInstruction, wincode serialization, instruction composition, account sequencing, and forwarded account metadata handling. |
Authorized processing and dispatch programs/magic-root-program/Cargo.toml, programs/magic-root-program/src/lib.rs, programs/magic-root-program/src/processor.rs, programs/magic-root-program/src/tests.rs |
Adds the program entrypoint, authorization checks, instruction decoding, variant dispatch, and authorization coverage. |
Account lifecycle and post-finalize execution programs/magic-root-program/src/account.rs, programs/magic-root-program/src/post_finalize.rs, programs/magic-root-program/README.md |
Adds validated patching, lamport balancing, finalization, executable program-cache loading, deletion, post-finalize CPI restrictions, and lifecycle documentation. |
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
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check | MagicRoot requirements are covered, but the required v42 interface, builder, and program crates are not present in the listed changes [#30]. |
Add the missing v42 calculator interface, builder, and program crates, or link the PR that implements them. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly identifies the primary change: adding the internal built-in MagicRoot program. |
| Description check | ✅ Passed | The description directly explains the MagicRoot and v42 program crates, their purpose, behavior, and follow-up engine wiring. |
| Out of Scope Changes check | ✅ Passed | The listed manifests, implementation files, documentation, and tests support the MagicRoot and workspace objectives without unrelated code changes. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
programs
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 help to get the list of available commands.
|
@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
| pub(crate) fn authorize(ctx: &InvokeContext<'_, '_>) -> Result<(), InstructionError> { | ||
| let height = ctx.get_stack_height(); | ||
| if height != TRANSACTION_LEVEL_STACK_HEIGHT { | ||
| let instruction = ctx.transaction_context.get_current_instruction_context()?; | ||
| let caller = ctx | ||
| .transaction_context | ||
| .get_instruction_context_at_index_in_trace(instruction.get_index_of_caller())?; | ||
| let caller_id = caller.get_program_key()?; | ||
| let is_builtin = ctx | ||
| .program_cache_for_tx_batch | ||
| .find(caller_id) | ||
| .filter(|e| matches!(e.program, ProgramCacheEntryType::Builtin(_))) | ||
| .is_some(); | ||
| if !is_builtin { | ||
| ic_msg!(ctx, "MagicRoot: non-builtin caller {}", caller_id); | ||
| return Err(InstructionError::CallDepth); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
authorize does not enforce the documented "MagicRoot itself cannot be the caller" rule.
programs/magic-root-program/README.md Line 10 states that MagicRoot cannot be the caller. authorize only requires the immediate caller to be a builtin in the program cache. MagicRoot is itself registered as a builtin, so a MagicRoot-to-MagicRoot CPI passes this check. Today post_finalize::process blocks actions that target magic_root_interface::ID, so the recursion is blocked one layer up. The invariant is therefore documented in authorize and in the README, but enforced somewhere else.
Add the explicit self-caller rejection so the check matches the documentation and survives future changes to post_finalize.
🛡️ Proposed fix
let caller_id = caller.get_program_key()?;
+ if *caller_id == magic_root_interface::ID {
+ ic_msg!(ctx, "MagicRoot: recursive caller");
+ return Err(InstructionError::CallDepth);
+ }
let is_builtin = ctx
.program_cache_for_tx_batch
.find(caller_id)
.filter(|e| matches!(e.program, ProgramCacheEntryType::Builtin(_)))
.is_some();📝 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.
| pub(crate) fn authorize(ctx: &InvokeContext<'_, '_>) -> Result<(), InstructionError> { | |
| let height = ctx.get_stack_height(); | |
| if height != TRANSACTION_LEVEL_STACK_HEIGHT { | |
| let instruction = ctx.transaction_context.get_current_instruction_context()?; | |
| let caller = ctx | |
| .transaction_context | |
| .get_instruction_context_at_index_in_trace(instruction.get_index_of_caller())?; | |
| let caller_id = caller.get_program_key()?; | |
| let is_builtin = ctx | |
| .program_cache_for_tx_batch | |
| .find(caller_id) | |
| .filter(|e| matches!(e.program, ProgramCacheEntryType::Builtin(_))) | |
| .is_some(); | |
| if !is_builtin { | |
| ic_msg!(ctx, "MagicRoot: non-builtin caller {}", caller_id); | |
| return Err(InstructionError::CallDepth); | |
| } | |
| } | |
| pub(crate) fn authorize(ctx: &InvokeContext<'_, '_>) -> Result<(), InstructionError> { | |
| let height = ctx.get_stack_height(); | |
| if height != TRANSACTION_LEVEL_STACK_HEIGHT { | |
| let instruction = ctx.transaction_context.get_current_instruction_context()?; | |
| let caller = ctx | |
| .transaction_context | |
| .get_instruction_context_at_index_in_trace(instruction.get_index_of_caller())?; | |
| let caller_id = caller.get_program_key()?; | |
| if *caller_id == magic_root_interface::ID { | |
| ic_msg!(ctx, "MagicRoot: recursive caller"); | |
| return Err(InstructionError::CallDepth); | |
| } | |
| let is_builtin = ctx | |
| .program_cache_for_tx_batch | |
| .find(caller_id) | |
| .filter(|e| matches!(e.program, ProgramCacheEntryType::Builtin(_))) | |
| .is_some(); | |
| if !is_builtin { | |
| ic_msg!(ctx, "MagicRoot: non-builtin caller {}", caller_id); | |
| return Err(InstructionError::CallDepth); | |
| } | |
| } |
🤖 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/processor.rs` around lines 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.
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.