feat: add customized solana-account crate#13
Conversation
📝 WalkthroughWalkthroughThis PR adds a new Changessolana-account crate fork
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant AccountSharedData
participant CoWAccount
participant BorrowedAccount
participant OwnedAccount
Caller->>AccountSharedData: mutate account fields or data
AccountSharedData->>CoWAccount: translate and update
CoWAccount->>BorrowedAccount: check borrowed capacity
alt mutation fits
BorrowedAccount-->>CoWAccount: update borrowed image
else mutation exceeds capacity
CoWAccount->>OwnedAccount: promote storage
OwnedAccount-->>CoWAccount: provide owned backing
end
CoWAccount-->>AccountSharedData: return updated account state
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
bmuddha
force-pushed
the
solana-account
branch
3 times, most recently
from
726ddf7 to
b00291b
Compare
bmuddha
force-pushed
the
solana-account
branch
2 times, most recently
from
a0e2c91 to
7501614
Compare
bmuddha
force-pushed
the
solana-account
branch
4 times, most recently
from
9acbc90 to
2e43191
Compare
bmuddha
force-pushed
the
solana-account
branch
7 times, most recently
from
9f791a8 to
aab5a4e
Compare
bmuddha
force-pushed
the
solana-account
branch
2 times, most recently
from
2bad847 to
a7a8ce1
Compare
bmuddha
force-pushed
the
solana-account
branch
5 times, most recently
from
d353d1d to
0bf814e
Compare
bmuddha
force-pushed
the
solana-account
branch
6 times, most recently
from
12406dd to
b65cb20
Compare
![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
🧹 Nitpick comments (2)
solana/account/Cargo.toml (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
bitflags'sserdefeature is enabled unconditionally, undermining the optionalserdefeature.
bitflagsis declared withfeatures = ["serde"]unconditionally (notoptional = true, not gated behind this crate's ownserdefeature). Per bitflags' own manifest,serde"Enables: serde_core", an optional dependency of bitflags — enabling this feature always pulls that dependency and compilesStateFlags's serde impls, even when consumers disable this crate'sserdefeature (line 16).Consider routing it through the crate's own feature instead:
♻️ Proposed fix
[features] -serde = ["dep:serde", "dep:serde_bytes", "serde/rc", "solana-pubkey/serde"] +serde = ["dep:serde", "dep:serde_bytes", "serde/rc", "solana-pubkey/serde", "bitflags/serde"] [dependencies] -bitflags = { workspace = true, features = ["serde"] } +bitflags = { workspace = true }Also applies to: 21-21
🤖 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/account/Cargo.toml` at line 16, The crate’s optional serde feature is being undermined because bitflags is always built with its serde support enabled, so update the Cargo feature wiring in Cargo.toml to route bitflags’ serde through this crate’s own serde feature instead of enabling it unconditionally. Locate the dependency and feature declarations for bitflags and StateFlags, then make bitflags’ serde-related support optional and only activated when this crate’s serde feature is enabled, matching the existing serde feature list and preserving behavior for non-serde consumers.solana/account/src/state_traits.rs (1)
56-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
StateMutimpl bodies forAccountandAccountSharedData.Both impls have identical bodies. Consider a single blanket impl to avoid the duplication.
♻️ Proposed refactor
-impl<T> StateMut<T> for Account -where - T: serde::Serialize + serde::de::DeserializeOwned, -{ - fn state(&self) -> Result<T, InstructionError> { - state(self) - } - fn set_state(&mut self, state: &T) -> Result<(), InstructionError> { - set_state(self, state) - } -} - -impl<T> StateMut<T> for AccountSharedData -where - T: serde::Serialize + serde::de::DeserializeOwned, -{ - fn state(&self) -> Result<T, InstructionError> { - state(self) - } - fn set_state(&mut self, state: &T) -> Result<(), InstructionError> { - set_state(self, state) - } -} +impl<A, T> StateMut<T> for A +where + A: crate::ReadableAccount + crate::WritableAccount, + T: serde::Serialize + serde::de::DeserializeOwned, +{ + fn state(&self) -> Result<T, InstructionError> { + state(self) + } + fn set_state(&mut self, state: &T) -> Result<(), InstructionError> { + set_state(self, state) + } +}🤖 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/account/src/state_traits.rs` around lines 56 - 78, The `StateMut<T>` implementations for `Account` and `AccountSharedData` are duplicated with identical `state` and `set_state` bodies. Refactor `state_traits.rs` to remove the repeated impl blocks by introducing a shared blanket implementation or a common helper abstraction that both types can use, while preserving the existing `StateMut` API and the `state`/`set_state` behavior.Source: Path instructions
🤖 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/account/src/codec.rs`:
- Around line 7-12: The deserialize_data helper currently uses
bincode::deserialize, which rejects trailing bytes in account buffers. Update
deserialize_data<T, U> to decode through a bincode configuration that allows
trailing bytes so StateMut::state can read accounts created with
Account::new_data_with_space even when the serialized value does not fill the
entire buffer. Keep the change localized to deserialize_data and preserve the
existing Result<T, bincode::Error> behavior.
In `@solana/account/src/cow/borrowed.rs`:
- Around line 140-147: The unsafe borrowed view method reset in borrowed.rs is
missing the required rustdoc safety contract. Add a # Safety section to reset,
matching the style used by translate and rollback, and document that the header
must remain live and that the view must come from init or translate before
calling it. Keep the docs near the reset method so callers can see the
preconditions when using this unsafe API.
- Around line 1-5: Update the module rustdoc in borrowed.rs so it matches the
actual behavior of the zero-copy account view helpers: `rollback` should be
described as undoing a prior `commit` by decrementing the sequence counter,
while `reset` should be added as the operation that repoints the view to the
active image. Keep the descriptions aligned with the symbols
`AccountHeader::sequence`, `translate`, `reset`, `rollback`, and `commit` so the
docs reflect the real roles of each method.
In `@solana/account/src/state_traits.rs`:
- Around line 19-28: The public State<T> trait is currently unused and its docs
don’t match the actual shared-handle implementation, which lives on StateMut<T>.
Either move the Ref<'_, AccountSharedData> read/write behavior into State<T>
with an impl in the relevant state-handling code, or remove State<T> entirely if
StateMut<T> is the intended API; make sure the trait definition and its
documentation stay aligned with the real entry points.
In `@solana/account/src/test_utils.rs`:
- Around line 33-41: `aligned_account_buffer` should not pass a potentially null
pointer from `alloc::alloc_zeroed` into `Vec::from_raw_parts`. Add an explicit
check after the allocation in `aligned_account_buffer` and handle allocation
failure safely before constructing the Vec, and also ensure the zero-units case
is guarded or handled so the `Layout::array`/allocation path is never used with
a zero-sized layout. Keep the fix localized to `aligned_account_buffer` and
preserve the allocation/deallocation symmetry expected by `Vec::from_raw_parts`.
---
Nitpick comments:
In `@solana/account/Cargo.toml`:
- Line 16: The crate’s optional serde feature is being undermined because
bitflags is always built with its serde support enabled, so update the Cargo
feature wiring in Cargo.toml to route bitflags’ serde through this crate’s own
serde feature instead of enabling it unconditionally. Locate the dependency and
feature declarations for bitflags and StateFlags, then make bitflags’
serde-related support optional and only activated when this crate’s serde
feature is enabled, matching the existing serde feature list and preserving
behavior for non-serde consumers.
In `@solana/account/src/state_traits.rs`:
- Around line 56-78: The `StateMut<T>` implementations for `Account` and
`AccountSharedData` are duplicated with identical `state` and `set_state`
bodies. Refactor `state_traits.rs` to remove the repeated impl blocks by
introducing a shared blanket implementation or a common helper abstraction that
both types can use, while preserving the existing `StateMut` API and the
`state`/`set_state` behavior.
🪄 Autofix (Beta)
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
Run ID: 1f9f0e4c-f5dc-4562-9ab2-2724f9490359
📒 Files selected for processing (22)
.gitattributes.gitignoreCargo.tomlsolana/account/Cargo.tomlsolana/account/README.mdsolana/account/src/account.rssolana/account/src/codec.rssolana/account/src/cow/borrowed.rssolana/account/src/cow/mod.rssolana/account/src/cow/owned.rssolana/account/src/cow/tests.rssolana/account/src/lib.rssolana/account/src/patch.rssolana/account/src/state_traits.rssolana/account/src/sysvar.rssolana/account/src/test_utils.rssolana/account/src/tests/account.rssolana/account/src/tests/mod.rssolana/account/src/tests/state_traits.rssolana/account/src/tests/sysvar.rssolana/account/src/traits.rssrc/lib.rs
💤 Files with no reviewable changes (1)
- src/lib.rs
| /// Deserializes typed state from an account data slice. | ||
| pub(crate) fn deserialize_data<T: DeserializeOwned, U: ReadableAccount>( | ||
| account: &U, | ||
| ) -> Result<T, bincode::Error> { | ||
| bincode::deserialize(account.data()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== bincode version pins ==\n'
rg -n 'bincode' Cargo.toml Cargo.lock **/Cargo.toml **/Cargo.lock || true
printf '\n== codec/account/state snippets ==\n'
for f in solana/account/src/codec.rs solana/account/src/account.rs solana/account/src/state_traits.rs; do
if [[ -f "$f" ]]; then
echo "--- $f"
nl -ba "$f" | sed -n '1,240p'
fi
doneRepository: magicblock-labs/magicblock-engine
Length of output: 533
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== relevant repo snippets ==\n'
for f in solana/account/src/codec.rs solana/account/src/account.rs solana/account/src/state_traits.rs; do
if [[ -f "$f" ]]; then
echo "--- $f"
sed -n '1,240p' "$f"
fi
done
printf '\n== bincode references in repo ==\n'
rg -n 'allow_trailing_bytes|with_fixint_encoding|deserialize\(' solana/account/src Cargo.toml Cargo.lockRepository: magicblock-labs/magicblock-engine
Length of output: 11468
🌐 Web query:
bincode 1.3.3 deserialize trailing bytes default allow_trailing_bytes docs
💡 Result:
In bincode 1.3.3, the allow_trailing_bytes method is part of the bincode::config::Options trait [1]. It is used to configure the deserializer to permit extra, unused bytes in a byte slice after the deserialization process is complete [2][3]. By default, bincode's behavior regarding trailing bytes depends on how you invoke it. When using the standard deserialize function, trailing bytes are generally allowed [2]. However, when using custom configuration options, the default behavior is typically to reject trailing bytes [4]. To explicitly allow trailing bytes when using a custom configuration, you can use the allow_trailing_bytes method on a configuration object [1][4]. Example usage: use bincode::Options; let my_options = bincode::DefaultOptions::new.with_fixint_encoding.allow_trailing_bytes; let result: MyStruct = my_options.deserialize(bytes)?; The AllowTrailing struct is the underlying configuration type used by this method to specify that trailing bytes in a slice should not trigger a deserialization error [3][5].
Citations:
- 1: https://docs.rs/bincode/1/bincode/config/trait.Options.html
- 2: https://docs.rs/bincode/1.3.3/bincode/config/index.html
- 3: https://docs.rs/bincode/1.3.3/bincode/config/struct.AllowTrailing.html
- 4: https://docs.rs/bincode/1.3.2/bincode/config/trait.Options.html
- 5: https://doc.servo.org/bincode/config/trailing/index.html
🌐 Web query:
docs.rs bincode 1.3.3 bincode::deserialize trailing bytes default behavior
💡 Result:
In bincode 1.3.3, the default behavior when deserializing from a byte slice is to reject trailing bytes [1][2][3]. If there are extra bytes remaining in the slice after the deserialization process is complete, bincode will return an error [4][5]. You can explicitly configure this behavior using the bincode::config::Options trait [2][6]: * To reject trailing bytes (default): Use.reject_trailing_bytes [2][7]. * To allow trailing bytes: Use.allow_trailing_bytes [2][7]. Note that for historical reasons, the default options used by the top-level bincode::deserialize function may differ from those configured via the DefaultOptions struct [1]. If you want to ensure specific behavior, it is recommended to use the Builder pattern provided by the bincode::config module to explicitly set your desired configuration [1].
Citations:
- 1: https://docs.rs/bincode/1.3.3/bincode/config/index.html
- 2: https://docs.rs/bincode/1/bincode/config/trait.Options.html
- 3: https://doc.servo.org/bincode/config/trait.Options.html
- 4: https://docs.rs/bincode/1.3.3/bincode/config/struct.RejectTrailing.html
- 5: https://doc.servo.org/bincode/config/trailing/index.html
- 6: https://docs.rs/bincode/1.3.2/bincode/config/trait.Options.html
- 7: https://docs.rs/bincode/1.3.3/bincode/config/struct.WithOtherIntEncoding.html
Allow trailing bytes when decoding account state
bincode::deserialize rejects trailing bytes in bincode 1.3.3, so accounts created with Account::new_data_with_space cannot be read back through StateMut::state once the serialized value doesn’t consume the full buffer. Use a config that allows trailing bytes here.
🤖 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/account/src/codec.rs` around lines 7 - 12, The deserialize_data helper
currently uses bincode::deserialize, which rejects trailing bytes in account
buffers. Update deserialize_data<T, U> to decode through a bincode configuration
that allows trailing bytes so StateMut::state can read accounts created with
Account::new_data_with_space even when the serialized value does not fill the
entire buffer. Keep the change localized to deserialize_data and preserve the
existing Result<T, bincode::Error> behavior.
| //! Raw layout used by the borrowed zero-copy account view. | ||
| //! | ||
| //! The buffer is 8-byte aligned and contains a header followed by two images. | ||
| //! `AccountHeader::sequence` selects the active image; `translate` copies it to the shadow | ||
| //! image, `rollback` restores the old view, and `commit` publishes the shadow image. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Module doc conflates rollback and reset.
The header says rollback "restores the old view", but in the code (Line 154) rollback only decrements the sequence counter to undo a commit; it is reset (Line 143) that repoints the view to the active image. reset is not mentioned at all. Please align the doc with the actual roles.
📝 Suggested doc fix
-//! `AccountHeader::sequence` selects the active image; `translate` copies it to the shadow
-//! image, `rollback` restores the old view, and `commit` publishes the shadow image.
+//! `AccountHeader::sequence` selects the active image; `translate` copies it to the shadow
+//! image, `commit` publishes the shadow image by advancing the sequence, `rollback` undoes
+//! the latest `commit` by reversing the sequence, and `reset` repoints the view to the
+//! currently active image.As per path instructions: "Check docs and rustdoc for factual consistency with the code."
📝 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.
| //! Raw layout used by the borrowed zero-copy account view. | |
| //! | |
| //! The buffer is 8-byte aligned and contains a header followed by two images. | |
| //! `AccountHeader::sequence` selects the active image; `translate` copies it to the shadow | |
| //! image, `rollback` restores the old view, and `commit` publishes the shadow image. | |
| //! Raw layout used by the borrowed zero-copy account view. | |
| //! | |
| //! The buffer is 8-byte aligned and contains a header followed by two images. | |
| //! `AccountHeader::sequence` selects the active image; `translate` copies it to the shadow | |
| //! image, `commit` publishes the shadow image by advancing the sequence, `rollback` undoes | |
| //! the latest `commit` by reversing the sequence, and `reset` repoints the view to the | |
| //! currently active image. |
🤖 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/account/src/cow/borrowed.rs` around lines 1 - 5, Update the module
rustdoc in borrowed.rs so it matches the actual behavior of the zero-copy
account view helpers: `rollback` should be described as undoing a prior `commit`
by decrementing the sequence counter, while `reset` should be added as the
operation that repoints the view to the active image. Keep the descriptions
aligned with the symbols `AccountHeader::sequence`, `translate`, `reset`,
`rollback`, and `commit` so the docs reflect the real roles of each method.
Source: Path instructions
| /// Repoints this view to the currently active image without copying data. | ||
| /// | ||
| /// This is used after a sequence change or abandoned shadow write. | ||
| pub unsafe fn reset(&mut self) { | ||
| let offset = offset(self.header.as_ref(), true); | ||
| self.core = self.header.add(offset).cast(); | ||
| self.data = DataSlice::init(self.core.add(1).cast()); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
reset is unsafe but has no # Safety section.
Unlike translate and rollback, the reset rustdoc omits a # Safety section documenting its precondition (the header must be live and the view must be one previously produced by init/translate). Clippy flags this. Document the caller contract so misuse cannot silently repoint into freed/aliased storage.
📝 Suggested doc fix
/// Repoints this view to the currently active image without copying data.
///
/// This is used after a sequence change or abandoned shadow write.
+ ///
+ /// # Safety
+ ///
+ /// The borrowed buffer behind `self.header` must still be live and must be
+ /// a valid borrowed account image created by [`OwnedAccount::serialize`].
pub unsafe fn reset(&mut self) {📝 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.
| /// Repoints this view to the currently active image without copying data. | |
| /// | |
| /// This is used after a sequence change or abandoned shadow write. | |
| pub unsafe fn reset(&mut self) { | |
| let offset = offset(self.header.as_ref(), true); | |
| self.core = self.header.add(offset).cast(); | |
| self.data = DataSlice::init(self.core.add(1).cast()); | |
| } | |
| /// Repoints this view to the currently active image without copying data. | |
| /// | |
| /// This is used after a sequence change or abandoned shadow write. | |
| /// | |
| /// # Safety | |
| /// | |
| /// The borrowed buffer behind `self.header` must still be live and must be | |
| /// a valid borrowed account image created by [`OwnedAccount::serialize`]. | |
| pub unsafe fn reset(&mut self) { | |
| let offset = offset(self.header.as_ref(), true); | |
| self.core = self.header.add(offset).cast(); | |
| self.data = DataSlice::init(self.core.add(1).cast()); | |
| } |
🧰 Tools
🪛 Clippy (1.96.0)
[warning] 143-143: unsafe function's docs are missing a # Safety section
(warning)
🤖 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/account/src/cow/borrowed.rs` around lines 140 - 147, The unsafe
borrowed view method reset in borrowed.rs is missing the required rustdoc safety
contract. Add a # Safety section to reset, matching the style used by translate
and rollback, and document that the header must remain live and that the view
must come from init or translate before calling it. Keep the docs near the reset
method so callers can see the preconditions when using this unsafe API.
Source: Linters/SAST tools
| /// Reads and writes typed account state through a shared handle. | ||
| /// | ||
| /// Writing through `Ref<'_, AccountSharedData>` is rejected. | ||
| pub trait State<T> { | ||
| /// Deserializes the account data as `T`. | ||
| fn state(&self) -> Result<T, InstructionError>; | ||
|
|
||
| /// Serializes `state` into the existing account data buffer. | ||
| fn set_state(&self, state: &T) -> Result<(), InstructionError>; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file with line numbers.
sed -n '1,220p' solana/account/src/state_traits.rs | cat -n
# Search for implementations/usages of the traits.
rg -n --hidden --glob '!**/target/**' '\bimpl\b.*\bState<|\\bimpl\b.*\bStateMut<|\bStateMut<|\bState<' solana/accountRepository: magicblock-labs/magicblock-engine
Length of output: 3836
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all references to the shared-handle trait and its impls.
rg -n --hidden --glob '!**/target/**' '\bState<|::State\b|\bStateMut<' .Repository: magicblock-labs/magicblock-engine
Length of output: 541
Remove the unused State<T> trait or wire it up to the shared-handle path
State<T> is declared here but has no impls anywhere in the crate; the Ref<'_, AccountSharedData> read-only behavior is implemented on StateMut<T> instead. That leaves a dead public trait and mismatched docs. Either move the shared-handle implementation onto State<T> or drop the trait if StateMut<T> is the intended API.
🤖 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/account/src/state_traits.rs` around lines 19 - 28, The public State<T>
trait is currently unused and its docs don’t match the actual shared-handle
implementation, which lives on StateMut<T>. Either move the Ref<'_,
AccountSharedData> read/write behavior into State<T> with an impl in the
relevant state-handling code, or remove State<T> entirely if StateMut<T> is the
intended API; make sure the trait definition and its documentation stay aligned
with the real entry points.
Source: Path instructions
| fn aligned_account_buffer(units: usize) -> Vec<StorageUnit> { | ||
| let layout = Layout::array::<StorageUnit>(units).expect("account buffer layout"); | ||
| // SAFETY: `layout` has non-zero size and is the exact layout later used by | ||
| // `Vec<StorageUnit>` for deallocation. | ||
| let ptr = unsafe { alloc::alloc_zeroed(layout) }.cast(); | ||
| // SAFETY: `ptr` was allocated for `units` initialized `StorageUnit`s with | ||
| // the same layout `Vec` will use to deallocate it. | ||
| unsafe { Vec::from_raw_parts(ptr, units, units) } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing null-check after alloc_zeroed; potential UB on zero-size layout.
alloc::alloc_zeroed can return a null pointer on allocation failure, and the safety comment's non-zero-size claim isn't enforced here. Dereferencing/using a null pointer via Vec::from_raw_parts is immediate UB, and a zero-size layout (if units == 0 is ever reachable) is documented UB for alloc_zeroed.
🛡️ Proposed fix
fn aligned_account_buffer(units: usize) -> Vec<StorageUnit> {
let layout = Layout::array::<StorageUnit>(units).expect("account buffer layout");
- // SAFETY: `layout` has non-zero size and is the exact layout later used by
- // `Vec<StorageUnit>` for deallocation.
- let ptr = unsafe { alloc::alloc_zeroed(layout) }.cast();
+ assert!(units > 0, "account buffer must have at least one storage unit");
+ // SAFETY: `layout` has non-zero size (checked above) and is the exact
+ // layout later used by `Vec<StorageUnit>` for deallocation.
+ let raw_ptr = unsafe { alloc::alloc_zeroed(layout) };
+ if raw_ptr.is_null() {
+ alloc::handle_alloc_error(layout);
+ }
+ let ptr = raw_ptr.cast();
// SAFETY: `ptr` was allocated for `units` initialized `StorageUnit`s with
// the same layout `Vec` will use to deallocate it.
unsafe { Vec::from_raw_parts(ptr, units, units) }
}📝 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.
| fn aligned_account_buffer(units: usize) -> Vec<StorageUnit> { | |
| let layout = Layout::array::<StorageUnit>(units).expect("account buffer layout"); | |
| // SAFETY: `layout` has non-zero size and is the exact layout later used by | |
| // `Vec<StorageUnit>` for deallocation. | |
| let ptr = unsafe { alloc::alloc_zeroed(layout) }.cast(); | |
| // SAFETY: `ptr` was allocated for `units` initialized `StorageUnit`s with | |
| // the same layout `Vec` will use to deallocate it. | |
| unsafe { Vec::from_raw_parts(ptr, units, units) } | |
| } | |
| fn aligned_account_buffer(units: usize) -> Vec<StorageUnit> { | |
| let layout = Layout::array::<StorageUnit>(units).expect("account buffer layout"); | |
| assert!(units > 0, "account buffer must have at least one storage unit"); | |
| // SAFETY: `layout` has non-zero size (checked above) and is the exact | |
| // layout later used by `Vec<StorageUnit>` for deallocation. | |
| let raw_ptr = unsafe { alloc::alloc_zeroed(layout) }; | |
| if raw_ptr.is_null() { | |
| alloc::handle_alloc_error(layout); | |
| } | |
| let ptr = raw_ptr.cast(); | |
| // SAFETY: `ptr` was allocated for `units` initialized `StorageUnit`s with | |
| // the same layout `Vec` will use to deallocate it. | |
| unsafe { Vec::from_raw_parts(ptr, units, units) } | |
| } |
🤖 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/account/src/test_utils.rs` around lines 33 - 41,
`aligned_account_buffer` should not pass a potentially null pointer from
`alloc::alloc_zeroed` into `Vec::from_raw_parts`. Add an explicit check after
the allocation in `aligned_account_buffer` and handle allocation failure safely
before constructing the Vec, and also ensure the zero-units case is guarded or
handled so the `Layout::array`/allocation path is never used with a zero-sized
layout. Keep the fix localized to `aligned_account_buffer` and preserve the
allocation/deallocation symmetry expected by `Vec::from_raw_parts`.
Source: Path instructions
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/account/src/lib.rs`:
- Around line 13-15: Update the cfg attribute on the test_utils module to expose
it during ordinary unit-test builds as well as when the dev-context-only-utils
feature is enabled. Preserve the existing feature-gated behavior and ensure
tests/account.rs can resolve crate::test_utils under cfg(test).
In `@solana/account/src/test_utils.rs`:
- Around line 43-57: Make init_borrowed_account, borrowed_shared_data, and
active_borrowed_data unsafe or otherwise bind returned views to owned storage,
preventing arbitrary buffers from creating lifetime-free borrowed-layout views.
Document explicit caller obligations to provide a valid BorrowedAccount layout
and keep the backing buffer alive for every returned view; preserve
active_borrowed_data’s validation requirement as well.
In `@solana/account/src/tests/account.rs`:
- Around line 277-285: Update solana/account/src/tests/account.rs lines 277-285
in test_account_cow_borrowed_extend_promotes to append capacity - len + 1 bytes
and assert shared.cow() matches CoWAccount::Owned(_); update lines 342-353 in
the related set_data_at promotion test to either force and assert promotion or
rename it to describe in-capacity behavior, keeping the test names and
assertions factually consistent.
🪄 Autofix (Beta)
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
Run ID: 094db5d7-ce72-45fb-be2a-717761c2f6eb
📒 Files selected for processing (22)
.gitattributes.gitignoreCargo.tomlsolana/account/Cargo.tomlsolana/account/README.mdsolana/account/src/account.rssolana/account/src/codec.rssolana/account/src/cow/borrowed.rssolana/account/src/cow/mod.rssolana/account/src/cow/owned.rssolana/account/src/cow/tests.rssolana/account/src/lib.rssolana/account/src/patch.rssolana/account/src/state_traits.rssolana/account/src/sysvar.rssolana/account/src/test_utils.rssolana/account/src/tests/account.rssolana/account/src/tests/mod.rssolana/account/src/tests/state_traits.rssolana/account/src/tests/sysvar.rssolana/account/src/traits.rssrc/lib.rs
💤 Files with no reviewable changes (1)
- src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (17)
- solana/account/src/tests/sysvar.rs
- .gitignore
- solana/account/src/tests/state_traits.rs
- solana/account/src/tests/mod.rs
- .gitattributes
- solana/account/src/codec.rs
- solana/account/src/cow/tests.rs
- solana/account/Cargo.toml
- Cargo.toml
- solana/account/README.md
- solana/account/src/state_traits.rs
- solana/account/src/patch.rs
- solana/account/src/cow/owned.rs
- solana/account/src/cow/mod.rs
- solana/account/src/traits.rs
- solana/account/src/sysvar.rs
- solana/account/src/account.rs
| /// Test-only helpers for borrowed account buffers. | ||
| #[cfg(feature = "dev-context-only-utils")] | ||
| pub mod test_utils; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Expose these helpers when compiling unit tests.
tests/account.rs imports crate::test_utils, but cfg(test) alone does not enable this feature, causing unresolved imports during ordinary unit-test builds.
Proposed fix
-#[cfg(feature = "dev-context-only-utils")]
+#[cfg(any(test, feature = "dev-context-only-utils"))]
pub mod test_utils;As per path instructions, Rust workspace reviews must prioritize API contract correctness.
📝 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.
| /// Test-only helpers for borrowed account buffers. | |
| #[cfg(feature = "dev-context-only-utils")] | |
| pub mod test_utils; | |
| /// Test-only helpers for borrowed account buffers. | |
| #[cfg(any(test, feature = "dev-context-only-utils"))] | |
| pub mod test_utils; |
🤖 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/account/src/lib.rs` around lines 13 - 15, Update the cfg attribute on
the test_utils module to expose it during ordinary unit-test builds as well as
when the dev-context-only-utils feature is enabled. Preserve the existing
feature-gated behavior and ensure tests/account.rs can resolve crate::test_utils
under cfg(test).
Source: Path instructions
| /// Builds a borrowed account view over a test buffer. | ||
| pub fn init_borrowed_account(buf: &mut [StorageUnit]) -> BorrowedAccount { | ||
| let ptr = NonNull::from(&mut buf[..]).cast(); | ||
| // SAFETY: test buffers are created with the borrowed account layout. | ||
| unsafe { BorrowedAccount::init(ptr) } | ||
| } | ||
|
|
||
| /// Wraps a borrowed account test buffer in `AccountSharedData`. | ||
| pub fn borrowed_shared_data(buf: &mut [StorageUnit]) -> AccountSharedData { | ||
| AccountSharedData::from(init_borrowed_account(buf)) | ||
| } | ||
|
|
||
| /// Reinitializes a borrowed test buffer and returns its active data image. | ||
| pub fn active_borrowed_data(buf: &mut [StorageUnit]) -> Vec<u8> { | ||
| borrowed_shared_data(buf).data().to_vec() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Do not expose unchecked borrowed-layout construction as a safe API.
These functions accept arbitrary slices and return lifetime-free views backed by them. Safe code can pass malformed storage or drop the buffer while retaining BorrowedAccount/AccountSharedData, leading to invalid reads and undefined behavior.
Make escaping constructors unsafe with explicit layout and lifetime obligations, or return a wrapper that owns the buffer and binds the view to it. The same validation obligation applies to active_borrowed_data.
As per path instructions, Rust workspace reviews must prioritize unsafe usage and API contract correctness.
🤖 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/account/src/test_utils.rs` around lines 43 - 57, Make
init_borrowed_account, borrowed_shared_data, and active_borrowed_data unsafe or
otherwise bind returned views to owned storage, preventing arbitrary buffers
from creating lifetime-free borrowed-layout views. Document explicit caller
obligations to provide a valid BorrowedAccount layout and keep the backing
buffer alive for every returned view; preserve active_borrowed_data’s validation
requirement as well.
Source: Path instructions
| #[test] | ||
| // Writing past borrowed capacity should promote to owned storage. | ||
| fn test_account_cow_borrowed_extend_promotes() { | ||
| let (_buf, mut shared) = make_borrowed(vec![7, 8]); | ||
|
|
||
| shared.extend_from_slice(&[9]); | ||
|
|
||
| assert_eq!(shared.data(), &[7, 8, 9]); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the promotion tests actually cross capacity and assert owned storage.
Both tests currently verify only resulting bytes, so they can pass while storage remains borrowed.
solana/account/src/tests/account.rs#L277-L285: appendcapacity - len + 1bytes and assertmatches!(shared.cow(), CoWAccount::Owned(_)).solana/account/src/tests/account.rs#L342-L353: either force and assert one promotion or rename the test to describe in-capacityset_data_atbehavior.
As per path instructions, Rust tests and documentation must remain factually consistent with behavior.
📍 Affects 1 file
solana/account/src/tests/account.rs#L277-L285(this comment)solana/account/src/tests/account.rs#L342-L353
🤖 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/account/src/tests/account.rs` around lines 277 - 285, Update
solana/account/src/tests/account.rs lines 277-285 in
test_account_cow_borrowed_extend_promotes to append capacity - len + 1 bytes and
assert shared.cow() matches CoWAccount::Owned(_); update lines 342-353 in the
related set_data_at promotion test to either force and assert promotion or
rename it to describe in-capacity behavior, keeping the test names and
assertions factually consistent.
Source: Path instructions
1 participant
What changed
Added the
solana-accountcrate with owned and copy-on-write account representations, borrowed account layout helpers, codec helpers, sysvar support, and tests.Why
accountsdbneeds an account representation that can borrow from mapped storage and promote to owned storage only when writes require it.Closes #5.
Impact
Account,AccountSharedData, and the borrowed/owned copy-on-write model.Reviewer notes
The borrowed layout is layout-bound and must stay 8-byte aligned. This crate should stay storage-source agnostic.
Follow-up
nucleus,accountsdb,ledger, andkeeperbuild on this crate in later stack PRs.