feat: add transaction context crate - #19
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. |
8f1a777 to
ccac233
Compare
ccac233 to
c4029f9
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 |
|---|---|
Workspace and crate integration Cargo.toml, solana/transaction-context/Cargo.toml, solana/transaction-context/README.md, solana/transaction-context/src/lib.rs |
The workspace registers the crate and updates Solana and Agave dependencies. Crate metadata, features, documentation, and lint settings now use workspace-compatible values. |
Unified account storage and views solana/transaction-context/src/transaction_accounts.rs |
TransactionAccounts stores AccountSharedData directly. Immutable and mutable views use dereferencing and copy-on-write borrowing. Deconstruction preserves account metadata and dirty markers. |
Account access and resize rules solana/transaction-context/src/instruction.rs, solana/transaction-context/src/instruction_accounts.rs |
Program ownership uses direct account borrowing. Borrowed CoW accounts count as shared. Ephemeral-account resizing returns InvalidRealloc. |
Instruction trace and memory execution solana/transaction-context/src/transaction.rs, solana/transaction-context/src/vm_slice.rs |
Instruction execution uses placeholder frames, sequential trace indices, explicit CPI and total-trace limits, active-instruction counting, and direct remapping after account growth. Tests cover the revised trace and CPI behavior. |
Estimated code review effort: 4 (Complex) | ~60 minutes
Possibly related issues
- Issue 32 — The PR modifies the same imported
solana/transaction-contextcrate described by the issue.
Possibly related PRs
- magicblock-labs/magicblock-engine#31 — Both PRs modify
solana/transaction-context; this PR further changes its account, instruction, memory, and transaction-context implementations.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 58.54% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly identifies the primary change: adding the customized transaction context crate. |
| Description check | ✅ Passed | The description explains the crate fork, engine account model changes, mapped writes, and workspace integration. |
| Linked Issues check | ✅ Passed | The changes fork and adapt solana-transaction-context for engine execution and its account representation as required by issue [#10]. |
| Out of Scope Changes check | ✅ Passed | The workspace, documentation, dependency, account model, and transaction context changes support the linked issue objectives. |
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
transaction-context
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.
c4029f9 to
e4a023f
Compare
27ca11d to
ff1197d
Compare
7f5babf to
b1bb9fd
Compare
b1bb9fd to
f7c2bac
Compare
f7c2bac to
881f449
Compare
881f449 to
59ad79e
Compare
f8d3173 to
0ef24b5
Compare
0ef24b5 to
a1c175f
Compare
a1c175f to
cd9fd68
Compare
cd9fd68 to
80378f1
Compare
80378f1 to
e041528
Compare
e041528 to
e7b59e5
Compare
e7b59e5 to
f78da53
Compare
f78da53 to
7125534
Compare
7125534 to
2e0b538
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
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)
solana/transaction-context/src/instruction_accounts.rs (1)
226-243: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
is_shared()with borrowed accounts and update the reserve behavior.
make_data_mut()currently skips borrowed accounts, soget_data_mut()andextend_from_slice()can trigger borrowed-to-owned promotion later.CoWAccount::reserve()keeps the borrowed image whenspare()is enough, soreplace AccountSharedData::is_shared()withself.is_shared()and update the stale “second-copy” comment unless the reserve path always promotes.🔧 Proposed fix if borrowed images need the reserve
- if self.account.is_shared() { + if self.is_shared() { self.account.reserve(MAX_ACCOUNT_DATA_GROWTH_PER_INSTRUCTION); }🤖 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 `@solana/transaction-context/src/instruction_accounts.rs` around lines 226 - 243, Update make_data_mut() to use the InstructionAccount::is_shared() helper, so both shared buffers and borrowed CoWAccount variants enter the reserve path. Revise the nearby comment to accurately describe the reserve behavior and remove the stale claim about avoiding a second copy unless CoWAccount::reserve() is guaranteed to promote borrowed images.
🧹 Nitpick comments (1)
solana/transaction-context/src/transaction_accounts.rs (1)
188-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth deconstruct methods drain the same storage.
deconstruct_into_keyed_account_shared_dataanddeconstruct_into_account_shared_databoth callstd::mem::take(&mut self.accounts). After the first call the array is empty, so the second call returns an emptyVecwithout any error.borrow_countersandtouched_flagskeep their original length, solen()and index-based access become inconsistent with the storage.Consider making the drain explicit at the type level, or add a debug assertion that the storage is still populated.
🤖 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 `@solana/transaction-context/src/transaction_accounts.rs` around lines 188 - 199, Update deconstruct_into_keyed_account_shared_data and deconstruct_into_account_shared_data to make draining self.accounts explicit and prevent silent repeated deconstruction; ensure the second call cannot return an empty result while borrow_counters and touched_flags retain stale lengths, either through a consumed-state representation or a debug assertion before draining.
🤖 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 `@solana/transaction-context/src/transaction_accounts.rs`:
- Around line 152-156: Update try_borrow_mut so it projects directly to the
KeyedAccountSharedData tuple’s account field (.1) from the UnsafeCell pointer,
rather than first creating a mutable reference to the whole keyed account. Keep
AccountRefMut and borrow-counter behavior unchanged while ensuring the mutable
borrow excludes the key field.
---
Outside diff comments:
In `@solana/transaction-context/src/instruction_accounts.rs`:
- Around line 226-243: Update make_data_mut() to use the
InstructionAccount::is_shared() helper, so both shared buffers and borrowed
CoWAccount variants enter the reserve path. Revise the nearby comment to
accurately describe the reserve behavior and remove the stale claim about
avoiding a second copy unless CoWAccount::reserve() is guaranteed to promote
borrowed images.
---
Nitpick comments:
In `@solana/transaction-context/src/transaction_accounts.rs`:
- Around line 188-199: Update deconstruct_into_keyed_account_shared_data and
deconstruct_into_account_shared_data to make draining self.accounts explicit and
prevent silent repeated deconstruction; ensure the second call cannot return an
empty result while borrow_counters and touched_flags retain stale lengths,
either through a consumed-state representation or a debug assertion before
draining.
🪄 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: cb022939-1ae9-4679-8c8f-61d29221d71e
📒 Files selected for processing (10)
Cargo.tomlsolana/transaction-context/Cargo.tomlsolana/transaction-context/README.mdsolana/transaction-context/src/instruction.rssolana/transaction-context/src/instruction_accounts.rssolana/transaction-context/src/lib.rssolana/transaction-context/src/transaction.rssolana/transaction-context/src/transaction_accounts.rssolana/transaction-context/src/vm_addresses.rssolana/transaction-context/src/vm_slice.rs
💤 Files with no reviewable changes (1)
- solana/transaction-context/src/vm_addresses.rs

What changed
Customized the imported
solana-transaction-contextbaseline for the engine'saccount representation and patched the crate into the workspace.
Why
VM handlers need to remap account data on writes through a mapped region instead
of copying through a serialized input buffer.
Closes #10.
Impact
TransactionContext,InstructionContext, andTransactionAccountsaroundAccountRefandAccountRefMutviews.UnsafeCellbehind explicit borrow counters so borrows staylocal to the context.
through
ExecutionRecord.Reviewer notes
Borrow counters are released on drop; outstanding references must be gone before
a context is deconstructed. The mapped-account divergences are documented in
solana/README.md.Follow-up
program-runtimeandsvmconsume this customized context upstack.