std: add checked accessors to StoragePackedArray - #1541
Conversation
get/set on StoragePackedArray are deliberately unbounded: indices map arithmetically onto packed slot words, so an index past the caller's modeled domain silently spills into the next storage region instead of failing. A 16-cell board backed by StoragePackedArray<4> happily accepts set(16, v) and writes a lane into bits 64-67 of the second slot word — the footgun reported in argotorg#1538. Add get_checked/set_checked which take a const-generic modeled length and revert with Solidity's standard Panic(uint256) payload (code 0x32, IndexOutOfBounds) when the index is out of domain — the same signal the compiler emits for failed fixed-array bounds checks. Note: argotorg#1538's headline claim (fixed-array ADJACENCY[16] reading a default instead of trapping) does not reproduce: bounds-checked array indexing with an empty revert has been in place well before v26.2.0 (see checked_index_value in codegen). The silent-write behavior in that report comes from StorageMap/StoragePackedArray default-and-spill semantics, i.e. a caller-side missing guard, which these accessors now let callers express.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1748cc9523
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /// Revert with Solidity's standard `Panic(uint256)` payload carrying code | ||
| /// `0x32` (`IndexOutOfBounds`) — the same encoding the compiler emits for a | ||
| /// failed fixed-array bounds check, so callers observe one uniform signal. |
There was a problem hiding this comment.
For callers that inspect revert data, these accessors do not currently produce the uniform signal claimed here: native fixed-array indexing still calls emit_empty_revert for dynamic indices and emit_unconditional_empty_revert for constant indices in crates/codegen/src/sonatina/lower_runtime.rs:4507-4521, whereas this helper returns a 36-byte Panic(0x32) payload. Either align the compiler's array bounds lowering with this payload or stop documenting the behaviors as equivalent, since error-handling code can distinguish them.
Useful? React with 👍 / 👎.
|
Hey! Thanks for the PR, and sorry it took me so long to review it. I'm not entirely convinced by the proposed API: callers still have to supply Would it make more sense to keep What do you think? |
2 participants
get/set on StoragePackedArray are deliberately unbounded: indices map arithmetically onto packed slot words, so an index past the caller's modeled domain silently spills into the next storage region instead of failing. A 16-cell board backed by StoragePackedArray<4> happily accepts set(16, v) and writes a lane into bits 64-67 of the second slot word — the footgun reported in #1538.
Add get_checked/set_checked which take a const-generic modeled length and revert with Solidity's standard Panic(uint256) payload (code 0x32, IndexOutOfBounds) when the index is out of domain — the same signal the compiler emits for failed fixed-array bounds checks.
Note: #1538's headline claim (fixed-array ADJACENCY[16] reading a default instead of trapping) does not reproduce: bounds-checked array indexing with an empty revert has been in place well before v26.2.0 (see checked_index_value in codegen). The silent-write behavior in that report comes from StorageMap/StoragePackedArray default-and-spill semantics, i.e. a caller-side missing guard, which these accessors now let callers express.