Conversation
kpob
force-pushed
the
feature/storage-key-redesign
branch
from
2df400b to
177faf8
Compare
Introduces a path-based encoding scheme to increase the per-module field limit from 15 to 255 while preserving backward compatibility with deployed contracts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
9-task plan covering ContractEnv data model changes, macro updates, encoding propagation, and integration tests with upgrade scenarios. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the 4-bit-shift u32 index scheme with a path-based encoding in ContractEnv. The new design stores the full path as [u8; 8] and encodes it either as the legacy u32 (backward compatible for indices <= 15) or as a path-prefixed format [0xFF, len, path...] for larger indices or V2 encoding mode. - Add KeyEncoding enum (Legacy, V2) to control encoding strategy - Replace index: u32 with path: [u8; MAX_PATH_LEN] + path_len: u8 - Implement index_bytes() with dual encoding logic - Update child() with bounds check assertion - Add unit tests for encoding correctness and collision resistance - Re-export KeyEncoding from odra-core and odra facade crates Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace ContractEnv::new(0, backend) with ContractEnv::new(KeyEncoding::Legacy, backend) across all backends and test files to match the new constructor signature. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Raises the module field limit from 15 to 255 to support larger contracts in the storage key redesign. Adds `keys = "v2"` attribute parsing to ModuleConfiguration for future V2 key encoding propagation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When #[odra::module(keys = "v2")] is used, the macro-generated WASM entry points now create ContractEnv with KeyEncoding::V2 via WasmContractEnv::new_env_v2(). Default (Legacy) produces identical code to before. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds ManyFieldsContract with 20 Var<u32> fields and a test that verifies storage read/write works correctly across the 15-field boundary, exercising both legacy 4-bit key encoding (fields 1-15) and path-based encoding (fields 16+). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds OriginalContract (15 fields) and UpgradedContract (20 fields) with an upgrade test that deploys the original, writes data to f1/f10/f15, upgrades to the new implementation via try_upgrade, and confirms the original fields are preserved while new fields (f16+) start empty and can be written independently. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Explain the dual encoding logic: legacy 4-bit shift for backward compatibility vs path encoding for wider field indices. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…implifying ContractEnv initialization
kpob
force-pushed
the
feature/storage-key-redesign
branch
from
177faf8 to
eb4935b
Compare
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR redesigns storage key encoding to support up to 255 fields per module, introducing a breaking API change and critical collision safety considerations that require immediate attention.
📄 Documentation Diagram
This diagram documents the refactored storage key encoding workflow from field indices to hashed keys.
sequenceDiagram
participant U as User/Module
participant C as ContractEnv
participant B as Backend
U->>C: child(index: u8)
note over C: PR #35;641: Path updated; legacy or encoding based on index
C->>C: index_bytes()
note over C: Encodes path with 0xFF prefix if >15
C->>B: hash(index_bytes ++ mapping_data)
B-->>C: Hashed key
C->>U: current_key() returns StorageKey
🌟 Strengths
- Maintains backward compatibility with deployed contracts via a dual encoding strategy.
- Comprehensive integration testing validates upgrade scenarios and ensures reliability.
Findings Summary
| Priority | File | Category | Impact Summary (≤12 words) | Anchors |
|---|---|---|---|---|
| P1 | core/src/contract_env.rs | Architecture | Breaking public API change affects all downstream callers. | path:core/src/args.rs |
| P1 | core/src/contract_env.rs | Architecture | Dual encoding strategy risks critical key collisions if flawed. | local_only |
| P1 | odra-macros/src/ast/module_def.rs | Architecture | Raised field limit to 255; must sync with encoding changes. | local_only |
| P2 | examples/src/features/many_fields.rs | Testing | Integration test validates backward compatibility for upgrades. | local_only |
| P2 | odra-macros/src/ast/wasm_parts.rs | Maintainability | Centralized ContractEnv creation ensures macro consistency. | path:odra-macros/src/ast/factory/parts/wasm_parts.rs |
| P2 | docs/superpowers/plans/2026-03-25-storage-key-redesign.md | Documentation | Plan and implementation are out of sync on KeyEncoding. | path:core/src/contract_env.rs |
🔍 Notable Themes
- API Breaking Changes: The removal of the index parameter in
ContractEnv::new()disrupts multiple callers, necessitating coordinated updates across the codebase. - Collision Safety: The new encoding algorithm must guarantee no storage key overlaps, with unit tests serving as critical validation.
- Documentation Drift: Discrepancies between the implementation plan and actual code could lead to confusion or incomplete feature rollout.
📈 Risk Diagram
This diagram illustrates the risk of API breakage and storage key collisions in the new encoding strategy.
sequenceDiagram
participant A as API Caller
participant E as ContractEnv
participant S as Storage System
A->>E: new(backend) // Breaking change
note over E: R1(P1): Missing index parameter breaks existing code
E->>E: index_bytes()
note over E: R2(P1): Collision risk if encoding algorithm flawed
E->>S: store/retrieve with current_key()
note over S: R3(P1): Key mismatch could corrupt data
⚠️ **Unanchored Suggestions (Manual Review Recommended)**
The following suggestions could not be precisely anchored to a specific line in the diff. This can happen if the code is outside the changed lines, has been significantly refactored, or if the suggestion is a general observation. Please review them carefully in the context of the full file.
📁 File: examples/src/features/many_fields.rs
The integration test validates backward compatibility—the most critical requirement for the storage key redesign. It deploys a 15-field contract (using legacy encoding), writes data, then "upgrades" to a 20-field contract and verifies the original 15 fields remain accessible. This test provides strong evidence that the dual encoding strategy works correctly for real-world upgrade scenarios. The test uses try_upgrade which may involve contract replacement mechanics; ensure this matches the actual upgrade mechanism used in production.
Related Code:
#[test]
fn upgrade_preserves_existing_fields() {
let env = odra_test::env();
// ... deploy original contract
let addr = original.address();
let mut upgraded = UpgradedContract::try_upgrade(&env, addr, NoArgs).unwrap();
// Original fields must still be readable with same values
assert_eq!(upgraded.get_f1(), 100);
}📁 File: docs/superpowers/plans/2026-03-25-storage-key-redesign.md
Speculative: The implementation plan describes a KeyEncoding enum (Legacy/V2) and an encoding field in ContractEnv, but the actual implementation in contract_env.rs omits these. Instead, it uses a hybrid automatic detection (indices ≤15 → legacy, else path). This deviation simplifies the initial implementation but loses the ability to explicitly opt into V2 encoding for new contracts (as mentioned in the spec). This could be intentional for phase 1, but the plan and code are now out of sync, which may confuse future developers.
Related Code:
**Architecture:** `ContractEnv.index: u32` is replaced with `path: [u8; 8]` + `path_len: u8` + `encoding: KeyEncoding`.💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| pub const fn new(backend: Rc<RefCell<dyn ContractContext>>) -> Self { | ||
| Self { | ||
| index, | ||
| path: [0u8; MAX_PATH_LEN], | ||
| path_len: 0, | ||
| mapping_data: Vec::new(), | ||
| backend | ||
| } | ||
| } |
There was a problem hiding this comment.
P1 | Confidence: High
The ContractEnv::new constructor signature has changed from new(index: u32, backend: ...) to new(backend: ...), removing the index parameter entirely. This is a breaking public API change that affects all downstream callers. The related_context shows 5 direct usages across the codebase (args.rs, odra_vm_host.rs, contract_container.rs, wasm_contract_env.rs, livenet_host.rs) that have been updated, but any third-party code or plugins implementing ContractContext or creating ContractEnv instances will fail to compile. This change eliminates the old 4-bit shift encoding entirely, which aligns with the storage key redesign goal but requires coordinated updates across all dependent code.
Code Suggestion:
pub const fn new(encoding: KeyEncoding, backend: Rc<RefCell<dyn ContractContext>>) -> Self {
Self {
path: [0u8; MAX_PATH_LEN],
path_len: 0,
encoding,
mapping_data: Vec::new(),
backend
}
}Evidence: path:core/src/args.rs, path:odra-casper/livenet-env/src/livenet_host.rs, path:odra-vm/src/odra_vm_host.rs
| // Legacy: pack indices into u32 via 4-bit shifts (e.g. path [3, 15] → 0x3F). | ||
| // Only used when all indices fit in a nibble, preserving old storage keys. | ||
| if path.iter().all(|&idx| idx <= 15) { | ||
| let index: u32 = path.iter().fold(0u32, |acc, &idx| (acc << 4) + idx as u32); |
There was a problem hiding this comment.
P1 | Confidence: High
The dual encoding strategy (legacy u32 for indices ≤15, path encoding with 0xFF prefix for >15) introduces critical collision safety. The 0xFF prefix ensures no overlap with legacy keys (first byte ≤0x0F). The inclusion of path_len prevents collisions between paths and mapping data (e.g., path [3,16] vs path [3] with mapping key [16]). However, this algorithm assumes all callers use u8 indices ≤255. The child() method accepts u8, and MAX_PATH_LEN=8 ensures the path fits in the array. The unit tests verify the collision properties, but the correctness of the entire storage system depends on this algorithm being error-free.
| let ident_entry_points = utils::ident::entry_points(); | ||
| let new_env_expr = utils::expr::new_wasm_contract_env(); | ||
| let exec_env_stmt: syn::Stmt = parse_quote!( | ||
| let exec_env = { | ||
| let env = odra::odra_casper_wasm_env::WasmContractEnv::new_env(); | ||
| let env = #new_env_expr; | ||
| let env_rc = Rc::new(env); | ||
| odra::ExecutionEnv::new(env_rc) | ||
| }; |
There was a problem hiding this comment.
P2 | Confidence: High
The macro-generated WASM entry points now use a shared new_env_expr via utils::expr::new_wasm_contract_env(). This centralizes the ContractEnv creation logic, ensuring consistency across all generated code (regular modules and factories). The related wasm_contract_env.rs updates WasmContractEnv::new_env() to call ContractEnv::new(backend) without the index parameter. This change is correct but relies on the unseen utils::expr::new_wasm_contract_env() function being updated accordingly. A missing update there would break all WASM contract compilation.
| const MAX_FIELDS: usize = 255; | ||
|
|
||
| #[derive(syn_derive::ToTokens)] | ||
| pub struct ModuleDefItem { |
There was a problem hiding this comment.
[Contextual Comment]
This comment refers to code near real line 19. Anchored to nearest_changed(7) line 7.
P1 | Confidence: High
Raising MAX_FIELDS from 15 to 255 enables the primary feature goal (supporting >15 fields). This change is synchronized with the storage key encoding changes in ContractEnv. However, the test test_invalid_module_definition is updated to expect success for 16 fields, which could mask regressions if the macro's field validation logic were broken. The change is correct but must be paired with the encoding changes to avoid storage collisions at runtime.
Benchmark report
|
…rease field limit - Removed legacy storage key encoding and replaced it with a path-based encoding that supports up to 255 fields per module. - Updated `ContractEnv` struct to use `path` and `path_len` instead of a single `index` value. - Introduced `KeyEncoding` enum to manage legacy and new encoding modes. - Modified the `child()` and `current_key()` methods to accommodate the new encoding scheme. - Updated the `odra-macros` to raise the maximum fields limit and support the `keys` attribute for V2 encoding. - Added integration tests for contracts with more than 15 fields and an upgrade scenario to ensure backward compatibility. - Updated `Odra.toml` to include new contract definitions for testing.
Benchmark report
|
1 participant
No description provided.