add permissioned signer module and update existing modules to use dot… #211
Conversation
WalkthroughThis update introduces several new modules and features to the Move standard library and Initia framework, including a scalable B+ tree-based ordered map, a permissioned signer abstraction, and granular gas parameters for new native operations. Numerous modules are refactored to use method-style signatures, and new native functions for memory, comparison, and vector manipulation are added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MoveModule
participant NativeContext
participant GasMeter
participant Storage
%% Permissioned Signer Creation and Use
User->>MoveModule: create_permissioned_handle(master_signer)
MoveModule->>NativeContext: native_signer_from_permissioned_handle(master_addr, perm_addr)
NativeContext->>GasMeter: charge(signer_from_permissioned_handle_base)
NativeContext-->>MoveModule: permissioned_signer
User->>MoveModule: is_permissioned_signer(signer)
MoveModule->>NativeContext: native_is_permissioned_signer(signer)
NativeContext->>GasMeter: charge(is_permissioned_signer_base)
NativeContext-->>MoveModule: bool
User->>MoveModule: permission_address(permissioned_signer)
MoveModule->>NativeContext: native_permission_address(permissioned_signer)
NativeContext->>GasMeter: charge(permission_address_base)
NativeContext-->>MoveModule: address
%% BigOrderedMap operations
User->>MoveModule: add(key, value) to BigOrderedMap
MoveModule->>Storage: allocate_slot_if_needed()
MoveModule->>Storage: update B+ tree nodes
Storage-->>MoveModule: confirmation
%% Vector move_range operation
User->>MoveModule: vector.move_range(from, pos, len, to, insert_pos)
MoveModule->>NativeContext: native_move_range(...)
NativeContext->>GasMeter: charge(vector_move_range_base + per_index_moved * count)
NativeContext-->>MoveModule: done
%% BCS constant_serialized_size
User->>MoveModule: bcs.constant_serialized_size<T>()
MoveModule->>NativeContext: native_constant_serialized_size(type_layout)
NativeContext->>GasMeter: charge(bcs_constant_serialized_size_base + per_type_node * count)
NativeContext-->>MoveModule: Option<u64>
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (19)
precompile/modules/move_stdlib/sources/signer.move (1)
2-22: Nit: spelling / grammar polish in newly-added docsMinor typos that can be fixed without code changes:
-/// signer is a builtin move type that represents an address that has been verfied by the VM. +/// `signer` is a builtin Move type that represents an address that has been verified by the VM. -/// ^ The discrepency is needed to maintain backwards compatibility of signer serialization +/// ^ The discrepancy is needed to maintain backwards compatibility of signer serializationAlso consider renaming the field
permissions_address→permission_addressfor
consistency with the public function namepermission_address(...).crates/natives/src/move_stdlib/mod.rs (2)
8-14: Keeppub mod …list alphabetically sorted for readabilityCurrent order:
bcs, cmp, hash, mem, signer, string, vector. Movingsignerafterstring
(or vice-versa in the table) keeps the list trivially greppable.No functional impact, purely readability.
40-42: Likewise, sort theadd_natives!callsadd_natives!("bcs", bcs::make_all(builder)); +add_natives!("cmp", cmp::make_all(builder)); add_natives!("hash", hash::make_all(builder)); -add_natives!("signer", signer::make_all(builder)); -add_natives!("string", string::make_all(builder)); +add_natives!("mem", mem::make_all(builder)); +add_natives!("signer", signer::make_all(builder)); +add_natives!("string", string::make_all(builder)); add_natives!("vector", vector::make_all(builder));Keeps registration order in step with the module declarations.
crates/natives/src/move_stdlib/cmp.rs (1)
38-46: Redundant length check
debug_assert!(args.len() == 2)already guards the invariant in debug builds.
The subsequentif args.len() != 2 { … }path is therefore dead in release builds and
duplicated in debug builds. Consider removing the redundant branch to reduce bytecode size.precompile/modules/move_stdlib/sources/mem.move (1)
15-18: Potential borrow checker complaint inreplaceBorrowing
&mut newand then movingnew(return) is valid only because the
borrow ends after theswapcall. While this compiles today, an explicit
let old = swap(...);-style pattern would make the intent clearer and prevent
future refactoring mistakes:friend fun replace<T>(ref: &mut T, mut new: T): T { swap(ref, &mut new); new }(added
mutto make it explicit thatnewis mutated).crates/natives/src/permissioned_signer.rs (1)
55-60: Hard-coded abort code lacks contextReturning
Abort { abort_code: 3 }exposes a magic number.
Define a constant (e.g.ENOT_PERMISSIONED) so Move code can match on a meaningful value and so future refactors don’t silently break the contract.precompile/modules/move_nursery/sources/acl.move (1)
42-45: Minor micro-optimisation opportunity
self.contains(addr)inassert_containsperforms a fullindex_ofscan twice (once here, once inremove).
Caching the result locally avoids redundant work, though impact is negligible for small ACLs.precompile/modules/move_stdlib/sources/bcs.move (1)
44-47: Address the TODO comment for the specification.The specification for
constant_serialized_sizeis marked as a temporary mockup. Please complete the formal specification to ensure proper verification coverage.Would you like me to help generate a complete specification for this function or open an issue to track this task?
crates/natives/src/move_stdlib/bcs.rs (1)
187-256: Comprehensive type size calculation with one consideration.The recursive implementation correctly handles all type layouts and properly tracks visited nodes for gas charging. However, for the
Signercase (line 203), while returningNoneis correct given that signer size may change, consider enhancing the comment to explain why signers are treated as variable-size despite appearing to have a fixed representation.Consider expanding the comment on line 202-203:
- // signer's size is VM implementation detail, and can change at will. - MoveTypeLayout::Signer => Ok(None), + // Signer's size is a VM implementation detail that may change across versions. + // To maintain forward compatibility, we treat it as having no constant size. + MoveTypeLayout::Signer => Ok(None),precompile/modules/move_stdlib/sources/cmp.move (1)
80-113: Duplicated assertions – consider dropping redundant blockThe block starting at line 95 re-tests exactly the same
(1,5)/(5,5)/(7,5)scenarios already covered in lines 82-94, only addingis_gt/is_ge.
Either merge the extra predicates into the first block or remove the duplicate check to keep the test light-weight.precompile/modules/move_stdlib/sources/bit_vector.move (1)
117-119: Verification disabled - needs follow-up.The pragma disabling verification mentions that it's due to inline function limitations. This should be tracked to re-enable verification once the Move language supports the necessary features.
Would you like me to create an issue to track re-enabling verification for
shift_leftonce inline function support is available?precompile/modules/initia_stdlib/sources/table.move (1)
253-260: Document safety requirements fordestroy_known_empty_unsafe.While the comment explains that the table cannot know if it's empty, it would be helpful to add more explicit documentation about:
- What invariants the caller must maintain
- What happens if called on a non-empty table
- Why this optimization is necessary
Consider expanding the documentation:
- /// Table cannot know if it is empty or not, so this method is not public, - /// and can be used only in modules that know by themselves that table is empty. + /// Table cannot know if it is empty or not, so this method is not public, + /// and can be used only in modules that know by themselves that table is empty. + /// + /// # Safety + /// Caller MUST ensure the table is empty. Calling this on a non-empty table + /// will leak resources. This optimization avoids the length check in destroy_empty + /// for modules that track emptiness externally (e.g., table_with_length). friend fun destroy_known_empty_unsafe<K: copy + drop, V>(precompile/modules/initia_stdlib/sources/permissioned_signer.move (1)
55-56: Consider using a named constant from a common module.The
U256_MAXconstant is defined inline. Consider importing this from a common constants module if available, or document why this specific value is used.+ /// Maximum value for u256, used to represent unlimited permission capacity const U256_MAX: u256 = 115792089237316195423570985008687907853269984665640564039457584007913129639935;precompile/modules/initia_stdlib/sources/ordered_map.move (1)
21-22: Consider making iterator API public in the future.The TODO comment indicates that iterator functions are friend-only pending Move language improvements. This limits the utility of the ordered map for external users who need efficient iteration.
Consider providing a stable subset of iterator operations that can be made public without waiting for language improvements, even if they're not as ergonomic as the ideal API.
precompile/modules/move_stdlib/sources/vector.move (2)
29-33: Consider documenting the performance implications of USE_MOVE_RANGE.While hardcoding this to
trueis reasonable given the cyclic dependency constraint, it would be helpful to document the performance characteristics when this feature is enabled vs disabled, so future maintainers understand the trade-offs.
284-305: Smart optimization for different insertion scenarios.The implementation intelligently chooses between direct swapping (for near-end insertions) and
move_range(for other positions) to optimize performance. Consider adding a comment explaining whyi + 2 >= lenis used as the threshold.if (USE_MOVE_RANGE) { + // When inserting within 2 positions of the end, direct swapping is more efficient + // than creating a temporary vector and using move_range if (i + 2 >= len) {precompile/modules/initia_stdlib/sources/big_ordered_map.move (3)
152-161: Potential inefficiency in constant-size type handling.The function asserts that types have constant serialized size but then passes 0 for both degree parameters, causing them to be dynamically initialized on first insert. Consider calculating the optimal degrees immediately for constant-size types.
public fun new<K: store, V: store>(): BigOrderedMap<K, V> { // Use new_with_type_size_hints or new_with_config if your types have variable sizes. - assert!( - bcs::constant_serialized_size<K>().is_some() - && bcs::constant_serialized_size<V>().is_some(), - error::invalid_argument(EINVALID_CONFIG_PARAMETER) - ); - - new_with_config(0, 0, false) + let key_size = bcs::constant_serialized_size<K>(); + let value_size = bcs::constant_serialized_size<V>(); + assert!( + key_size.is_some() && value_size.is_some(), + error::invalid_argument(EINVALID_CONFIG_PARAMETER) + ); + + let k_size = key_size.destroy_some(); + let v_size = value_size.destroy_some(); + new_with_type_size_hints(k_size, k_size, v_size, v_size) }
1270-1357: Complex but correct node splitting implementation.The node splitting logic correctly handles all cases including root splitting. Consider adding more inline comments explaining the key invariants being maintained, particularly around the index management when promoting the old root.
2561-2672: Consider periodic execution of commented extensive tests.The test suite is comprehensive, but several "long/extensive" tests are commented out. Consider running these periodically (e.g., nightly builds) to ensure continued correctness across all tree configurations.
You could use test attributes or environment variables to conditionally run these tests rather than commenting them out.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
crates/gas/src/initia_stdlib.rs(1 hunks)crates/gas/src/lib.rs(1 hunks)crates/gas/src/move_stdlib.rs(3 hunks)crates/natives/src/interface/context.rs(1 hunks)crates/natives/src/lib.rs(2 hunks)crates/natives/src/move_stdlib/bcs.rs(7 hunks)crates/natives/src/move_stdlib/cmp.rs(1 hunks)crates/natives/src/move_stdlib/mem.rs(1 hunks)crates/natives/src/move_stdlib/mod.rs(2 hunks)crates/natives/src/move_stdlib/vector.rs(1 hunks)crates/natives/src/permissioned_signer.rs(1 hunks)precompile/modules/initia_stdlib/sources/account.move(1 hunks)precompile/modules/initia_stdlib/sources/big_ordered_map.move(1 hunks)precompile/modules/initia_stdlib/sources/bigdecimal.move(3 hunks)precompile/modules/initia_stdlib/sources/ordered_map.move(1 hunks)precompile/modules/initia_stdlib/sources/permissioned_signer.move(1 hunks)precompile/modules/initia_stdlib/sources/simple_map.move(11 hunks)precompile/modules/initia_stdlib/sources/storage_slots_allocator.move(1 hunks)precompile/modules/initia_stdlib/sources/table.move(10 hunks)precompile/modules/initia_stdlib/sources/table_with_length.move(1 hunks)precompile/modules/move_nursery/sources/acl.move(2 hunks)precompile/modules/move_nursery/sources/capability.move(4 hunks)precompile/modules/move_stdlib/sources/bcs.move(2 hunks)precompile/modules/move_stdlib/sources/bit_vector.move(3 hunks)precompile/modules/move_stdlib/sources/cmp.move(1 hunks)precompile/modules/move_stdlib/sources/error.move(1 hunks)precompile/modules/move_stdlib/sources/fixed_point32.move(3 hunks)precompile/modules/move_stdlib/sources/mem.move(1 hunks)precompile/modules/move_stdlib/sources/option.move(3 hunks)precompile/modules/move_stdlib/sources/signer.move(1 hunks)precompile/modules/move_stdlib/sources/string.move(2 hunks)precompile/modules/move_stdlib/sources/vector.move(14 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
crates/natives/src/move_stdlib/mod.rs (3)
crates/natives/src/move_stdlib/cmp.rs (1)
make_all(68-74)crates/natives/src/move_stdlib/mem.rs (1)
make_all(47-53)crates/natives/src/move_stdlib/vector.rs (1)
make_all(96-102)
crates/natives/src/move_stdlib/mem.rs (4)
crates/natives/src/move_stdlib/cmp.rs (1)
make_all(68-74)crates/natives/src/move_stdlib/vector.rs (1)
make_all(96-102)crates/natives/src/move_stdlib/bcs.rs (1)
make_all(261-271)crates/natives/src/permissioned_signer.rs (1)
make_all(91-111)
crates/natives/src/permissioned_signer.rs (5)
types/bcs.go (1)
AccountAddress(102-102)crates/natives/src/move_stdlib/cmp.rs (1)
make_all(68-74)crates/natives/src/move_stdlib/mem.rs (1)
make_all(47-53)crates/natives/src/move_stdlib/vector.rs (1)
make_all(96-102)crates/natives/src/move_stdlib/bcs.rs (1)
make_all(261-271)
🪛 GitHub Actions: Lint
crates/natives/src/move_stdlib/vector.rs
[error] 26-26: Clippy lint 'precedence': operator precedence might not be obvious. Expression '0x1 << 16 + 1' should be parenthesized as '0x1 << (16 + 1)'.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Rust libmovevm
🔇 Additional comments (48)
crates/gas/src/lib.rs (1)
40-40: LGTM!Removing the trailing comma improves code style consistency.
precompile/modules/initia_stdlib/sources/account.move (1)
12-12: Friend module addition looks good.Adding
initia_std::permissioned_signeras a friend module is appropriate to enable the new permissioned signer abstraction to access account creation and signer management functions.crates/natives/src/interface/context.rs (1)
64-66: New method for dereferenced value size calculation looks good.The
abs_val_size_dereferencedmethod follows the same pattern as the existingabs_val_sizemethod and provides necessary functionality for gas charging in native functions that work with dereferenced values.precompile/modules/move_stdlib/sources/error.move (1)
68-74: Specification for canonical function is well-defined.The spec correctly formalizes the error code canonicalization behavior with appropriate concrete and abstract ensures clauses. This enhances the module's formal verification capabilities.
crates/natives/src/lib.rs (1)
27-27: Permissioned signer module integration follows established patterns.The new
permissioned_signermodule is properly declared and integrated into the native function table consistently with other native modules.Also applies to: 90-90
crates/gas/src/initia_stdlib.rs (1)
17-20: Add unit-tests / integration checks for the new gas parametersThe three new entries look consistent with the existing pattern, but there is no compile-time guarantee that
permission_address_base,is_permissioned_signer_base, andsigner_from_permissioned_handle_base
have been:
- Added to the
move_stdlibgas param struct (NativeGasParameters::move_stdlib), and- Consumed by the native implementations in
crates/natives/src/permissioned_signer.rs.A missing field will compile (the macro emits
Default::default()), but the runtime charge will always be zero,
silently under-pricing these ops.Please add an integration/constructor test that walks the generated struct and asserts the three fields are non-zero.
crates/natives/src/move_stdlib/mem.rs (1)
34-38: Confirm argument pop order matches Move ABI
leftis popped first andrightsecond. In Aptos/Move natives the arguments are usually pushed onto theVecDequein source-order and then popped from the front, which means they are consumed in the same order they appear in the Move signature.
If the Move declaration isswap<T>(left: &mut T, right: &mut T)this is correct; if it is the reverse, the swap will silently do the wrong thing at runtime. Please double-check the signature instd::mem.move.crates/gas/src/move_stdlib.rs (1)
38-45: Document and calibrate newly added gas parametersA dozen base / per-unit parameters are introduced with guesswork comments.
Without calibration these values can distort economic incentives.
Ensure a follow-up task exists to benchmark and tune:
cmp_compare_*vector_move_range_*mem_swap_base- new BCS constants
crates/natives/src/permissioned_signer.rs (1)
76-84: Argument order may be reversed
permission_addris popped beforemaster_addr.
If the Move native friend is declaredsigner_from_permissioned_handle_impl(master_addr: address, permission_addr: address), this will swap the two addresses and return an invalid signer.- let permission_addr = safely_pop_arg!(arguments, AccountAddress); - let master_addr = safely_pop_arg!(arguments, AccountAddress); + let master_addr = safely_pop_arg!(arguments, AccountAddress); + let permission_addr = safely_pop_arg!(arguments, AccountAddress);Please verify against the
.movedeclaration.precompile/modules/move_nursery/sources/acl.move (1)
24-35: Good move to method-style APIRefactoring to
selfreceivers cleans up call-sites and aligns with the standard library’s direction.
Implementation remains correct and no additional borrow checker constraints are introduced.precompile/modules/move_stdlib/sources/bcs.move (3)
6-7: LGTM!The import is correctly added to support the
Option<u64>return type of the newconstant_serialized_sizefunction.
8-9: Good addition of important safety documentation.This warning about permissioned signers causing native function failures is crucial information for users of the BCS module.
14-28: Well-designed native functions with comprehensive documentation.The addition of
serialized_sizeandconstant_serialized_sizeprovides useful BCS introspection capabilities. The documentation clearly explains:
- Abort conditions with specific error codes
- The distinction between runtime and constant sizes
- Important notes about type evolution and guarantees
The
Option<u64>return type forconstant_serialized_sizeis appropriate for representing types that may or may not have constant sizes.precompile/modules/initia_stdlib/sources/bigdecimal.move (1)
65-343: Excellent API consistency improvements through method-style refactoring.The systematic refactoring of all public functions to use
self(andotherfor binary operations) improves API ergonomics and aligns with modern Move conventions. The changes are purely stylistic with no logic modifications, making the API more intuitive for users who can now use method-style calls likenum1.add(num2)instead ofadd(num1, num2).precompile/modules/move_stdlib/sources/fixed_point32.move (2)
45-45: Good code cleanup.Removing unnecessary parentheses improves readability.
164-303: Consistent method-style refactoring with appropriate verification pragmas.The refactoring to use
selfparameters is consistent with the broader codebase modernization. The addition ofpragma verify_duration_estimate = 120forceilandroundfunctions appropriately indicates their verification complexity due to the mathematical operations involved.crates/natives/src/move_stdlib/bcs.rs (3)
26-29: Well-implemented helper for Option construction.The helper correctly creates a Move
Option<u64>value using the standard representation with a vector of 0 or 1 elements.
64-64: Good practice: using named constants for error codes.Replacing magic numbers with the
NFE_BCS_SERIALIZATION_FAILUREconstant improves code maintainability and consistency across the BCS module.Also applies to: 82-82, 125-125, 179-179
154-185: Well-structured native function with proper gas accounting.The implementation correctly:
- Charges base gas before computation
- Tracks visited nodes for proportional gas charging
- Handles errors appropriately with consistent abort codes
- Returns the proper Option type
precompile/modules/move_stdlib/sources/option.move (2)
38-38: Minor style improvement.Using vector literal syntax
vector[]is more concise than the function call.
57-381: Excellent API modernization with method-style signatures.The comprehensive refactoring to use
selfparameters and method-style vector calls significantly improves API ergonomics. Users can now write more intuitive code likeoption.is_some()andoption.borrow()instead ofis_some(&option)andborrow(&option). The changes are consistent throughout the module including all specifications.precompile/modules/initia_stdlib/sources/simple_map.move (1)
73-81:⚠️ Potential issue
keyused after move inborrow_mut_with_default
self.inner.add(key, default);(line 78) moveskey.
The subsequentself.inner.borrow_mut(key)re-uses the moved value → compile-time error.- self.inner.add(key, default); + self.inner.add(copy key, default);([or pass
copy keytoborrow_mutas well])Likely an incorrect or invalid review comment.
precompile/modules/initia_stdlib/sources/table_with_length.move (1)
87-95:upsertdoes not updatelengthwhen inserting new key viaadd
self.add(copy key, value)incrementslengthinternally, so the branch
is correct.
Nonetheless, theelsebranch mutates an existing value without touching
length– OK.
No action required; just confirming logic is sound.precompile/modules/move_stdlib/sources/bit_vector.move (3)
13-16: Good addition of struct invariant.The invariant
length == len(bit_field)properly enforces that the cached length always matches the actual vector length, which helps maintain data consistency.
95-97: Nice use offor_each_mutfor clarity.The refactoring to use
for_each_mutwhen zeroing out the entire bit vector is more idiomatic and clearer than the previous loop-based approach.
188-257: Comprehensive verification support added.The verification-only version of
shift_leftwith detailed loop invariants is excellent for formal verification. The invariants properly capture the state transitions and ensure correctness.precompile/modules/initia_stdlib/sources/table.move (1)
3-5: Appropriate friend module declarations.The friend declarations for
table_with_lengthandstorage_slots_allocatorenable proper encapsulation while allowing these tightly coupled modules to access internal functionality.precompile/modules/initia_stdlib/sources/storage_slots_allocator.move (1)
260-262: Re-enable formal verification.The module has verification disabled. Given that this is a foundational storage management module, formal verification would provide additional safety guarantees.
Is there a specific blocker preventing verification? If it's a tooling limitation, consider adding a TODO comment with the specific issue.
precompile/modules/initia_stdlib/sources/permissioned_signer.move (2)
287-296: Good defensive programming in cleanup.The
destroy_permissions_storage_addressfunction properly checks for existence before attempting to destroy, preventing potential aborts. The empty map destruction with a no-op lambda is also correct.
432-454:✅ Verification successful
Verify permission authorization logic.
The authorization functions check that the permissioned signer is actually permissioned AND the master is not permissioned AND they have the same address. This logic ensures only the original signer can grant permissions.
Let me verify that the permission model prevents privilege escalation:
🏁 Script executed:
#!/bin/bash # Description: Check for any bypasses of the master signer requirement in permission granting # Search for functions that modify permissions without master signer checks ast-grep --pattern $'fun $FUNC($$$) { $$$ insert_or($_, $_, $_, $_) $$$ }' # Also check for direct PermissionStorage modifications rg -A 5 'PermissionStorage\[.*\]\.perms'Length of output: 725
🏁 Script executed:
#!/bin/bash # Inspect the revoke function to ensure it enforces the same master signer checks rg -n -C5 'public(package) fun revoke<' precompile/modules/initia_stdlib/sources/permissioned_signer.moveLength of output: 107
🏁 Script executed:
#!/bin/bash # Locate the function handling permission removal to inspect its authorization checks rg -n -B5 -A5 'let perm_storage = &mut PermissionStorage' precompile/modules/initia_stdlib/sources/permissioned_signer.moveLength of output: 583
🏁 Script executed:
#!/bin/bash # Display lines 550-650 to inspect the full permission removal logic and its authorization checks sed -n '550,650p' precompile/modules/initia_stdlib/sources/permissioned_signer.moveLength of output: 4051
Permission Authorization Logic Confirmed
The
authorize_increasefunction enforces
is_permissioned_signer(permissioned)!is_permissioned_signer(master)- matching addresses between
masterandpermissionedNo other
insert_orcalls were found outside this guard.
Therevoke_permissionfunction similarly checksis_permissioned_signer(permissioned)before mutatingPermissionStorage, preventing any unauthorized removals.All permission-granting and ‑revocation paths properly enforce the master-signer constraint. No bypasses detected—LGTM.
precompile/modules/initia_stdlib/sources/ordered_map.move (4)
728-746: Correct binary search implementation.The binary search implementation correctly handles all cases and uses proper bounds. The use of bit shift for division (
(r - l) >> 1) is a nice optimization.
246-253: Smart optimization for sorted append.The optimization to check if all elements in
otherare larger than all inselfenables O(1) append in the best case. This is particularly useful for bulk operations on non-overlapping key ranges.
255-295: Efficient in-place merge algorithm.The reverse traversal merge algorithm is clever - building the result in reverse to avoid additional allocations. The handling of equal keys (collecting overwritten entries) is correct.
1382-1419: Comprehensive test data sets.The large dataset tests with both ordered and shuffled data provide good coverage for performance characteristics and edge cases.
precompile/modules/move_stdlib/sources/vector.move (7)
12-12: LGTM! Good use of the new mem module.The import of
std::memis appropriate for the newreplacefunction implementation.
40-75: Excellent refactoring to method-style signatures!The systematic update of all vector functions to use
selfas the first parameter creates a more intuitive and consistent API. This aligns well with modern programming practices and improves code readability.
77-92: Well-designed native function for efficient range operations.The
move_rangefunction provides an efficient primitive for moving contiguous elements between vectors. The documentation clearly explains the semantics, and the function signature is intuitive.
138-147: Efficient implementation using move_range.The updated
appendfunction correctly leverages the newmove_rangeprimitive when available, which should provide better performance than the reverse/reverse_append approach.
319-344: Consistent optimization pattern with insert.The
removefunction follows the same optimization strategy asinsert, choosing between direct swapping andmove_rangebased on position. The implementation is correct and efficient.
387-403: Valuable addition to the vector API.The new
replacefunction provides an efficient way to swap out an element and retrieve the old value. The use ofmem::replacewhen available is optimal, and the fallback implementation is correct.
782-811: Spec functions properly updated for consistency.All specification helper functions have been correctly updated to use the method-style
selfparameter, maintaining consistency with the main API changes.precompile/modules/initia_stdlib/sources/big_ordered_map.move (7)
1-21: Well-documented module with clear design rationale.The module documentation clearly explains why B+ tree was chosen and its benefits for this use case. The TODO comment about iterator APIs indicates awareness of current limitations and future improvements.
61-69: Verify MAX_NODE_BYTES against actual resource limits.The comment mentions 400KB is below the max resource limit, but it would be good to verify this against the actual platform limits and document the specific limit being referenced.
Can you confirm what the actual maximum resource size limit is in the Move environment to ensure this constant has appropriate headroom?
80-93: Excellent B+ tree node design.The node structure is well-designed with:
- Clear distinction between leaf and inner nodes
- Efficient ordered storage using OrderedMap
- Linked list pointers for sequential access at each level
890-943: Robust implementation of add/upsert with good optimizations.The function handles edge cases well:
- Optimizes for single-node (root-only) trees
- Correctly handles keys larger than all existing keys
- Properly maintains tree invariants
1453-1615: Thorough implementation of B+ tree rebalancing.The remove operation correctly implements the standard B+ tree rebalancing algorithm:
- Attempts to borrow from siblings when they have spare capacity
- Merges nodes when borrowing isn't possible
- Properly maintains all invariants and linked list pointers
486-496: Appropriate safety restriction on mutable borrows.The requirement for constant-size key/value types for
borrow_mutis a good safety measure. The error message clearly explains the restriction and suggests alternatives.Also applies to: 766-781
234-236: Good design for gas predictability with slot reuse.The ability to preallocate spare slots and reuse them is excellent for applications requiring predictable gas costs. This addresses a common concern with dynamic data structures in blockchain environments.
Also applies to: 306-317
| let cost = gas_params.cmp_compare_base | ||
| + gas_params.cmp_compare_per_abs_val_unit | ||
| * (context.abs_val_size_dereferenced(&args[0]) | ||
| + context.abs_val_size_dereferenced(&args[1])); | ||
| context.charge(cost)?; | ||
|
|
||
| let ordering = args[0].compare(&args[1])?; | ||
| let ordering_move_variant = match ordering { | ||
| std::cmp::Ordering::Less => ORDERING_LESS_THAN_VARIANT, | ||
| std::cmp::Ordering::Equal => ORDERING_EQUAL_VARIANT, | ||
| std::cmp::Ordering::Greater => ORDERING_GREATER_THAN_VARIANT, | ||
| }; |
There was a problem hiding this comment.
& references are off by one level – will not compile
VecDeque::index returns &Value.
Passing &args[0] therefore yields &&Value. The same applies to &args[1].
- * (context.abs_val_size_dereferenced(&args[0])
- + context.abs_val_size_dereferenced(&args[1]));
+ * (context.abs_val_size_dereferenced(args.get(0).unwrap())
+ + context.abs_val_size_dereferenced(args.get(1).unwrap()));
…
- let ordering = args[0].compare(&args[1])?;
+ let ordering = args.get(0).unwrap().compare(args.get(1).unwrap())?;Without this fix the crate will fail to compile with a “mismatched types: expected &Value, found &&Value” error.
📝 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 cost = gas_params.cmp_compare_base | |
| + gas_params.cmp_compare_per_abs_val_unit | |
| * (context.abs_val_size_dereferenced(&args[0]) | |
| + context.abs_val_size_dereferenced(&args[1])); | |
| context.charge(cost)?; | |
| let ordering = args[0].compare(&args[1])?; | |
| let ordering_move_variant = match ordering { | |
| std::cmp::Ordering::Less => ORDERING_LESS_THAN_VARIANT, | |
| std::cmp::Ordering::Equal => ORDERING_EQUAL_VARIANT, | |
| std::cmp::Ordering::Greater => ORDERING_GREATER_THAN_VARIANT, | |
| }; | |
| let cost = gas_params.cmp_compare_base | |
| + gas_params.cmp_compare_per_abs_val_unit | |
| * (context.abs_val_size_dereferenced(args.get(0).unwrap()) | |
| + context.abs_val_size_dereferenced(args.get(1).unwrap())); | |
| context.charge(cost)?; | |
| let ordering = args.get(0).unwrap().compare(args.get(1).unwrap())?; | |
| let ordering_move_variant = match ordering { | |
| std::cmp::Ordering::Less => ORDERING_LESS_THAN_VARIANT, | |
| std::cmp::Ordering::Equal => ORDERING_EQUAL_VARIANT, | |
| std::cmp::Ordering::Greater => ORDERING_GREATER_THAN_VARIANT, | |
| }; |
🤖 Prompt for AI Agents
In crates/natives/src/move_stdlib/cmp.rs around lines 47 to 58, the code
incorrectly takes references of args elements which are already references,
resulting in &&Value instead of &Value. To fix this, remove the extra &
operators before args[0] and args[1] when passing them to functions expecting
&Value, ensuring the types match and the code compiles correctly.
| gas_params.vector_move_range_per_index_moved | ||
| * NumArgs::new( | ||
| (from_len - removal_position) | ||
| .checked_add(to_len - insert_position) | ||
| .and_then(|v| v.checked_add(length)) | ||
| .ok_or_else(|| SafeNativeError::Abort { | ||
| abort_code: EINDEX_OUT_OF_BOUNDS, | ||
| })? as u64, | ||
| ), | ||
| )?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential usize → u64 overflow when charging gas
NumArgs::new(...) receives a u64, but the intermediate addition is done in usize.
On 64-bit builds usize may silently overflow before the cast, leading to under-charging.
Consider calculating in u128 or checking each addition separately before the final cast.
🤖 Prompt for AI Agents
In crates/natives/src/move_stdlib/vector.rs around lines 70 to 79, the gas
calculation uses usize arithmetic before casting to u64, risking overflow on
64-bit systems. To fix this, perform the additions using a wider integer type
like u128 or individually check each addition for overflow before casting to
u64. This ensures accurate gas charging without silent overflow.
| /// Given input positions/lengths are outside of vector boundaries. | ||
| pub const EINDEX_OUT_OF_BOUNDS: u64 = 0x1 << 16 + 1; | ||
|
|
There was a problem hiding this comment.
Fix Clippy precedence warning and ensure intended constant value
0x1 << 16 + 1 is parsed as 0x1 << (16 + 1) (shift by 17).
If the intended value is (0x1 << 16) + 1 ( 0x1_0001 ), parenthesise accordingly to silence the precedence lint and avoid confusion.
-pub const EINDEX_OUT_OF_BOUNDS: u64 = 0x1 << 16 + 1;
+pub const EINDEX_OUT_OF_BOUNDS: u64 = (0x1 << 16) + 1;🤖 Prompt for AI Agents
In crates/natives/src/move_stdlib/vector.rs around lines 23 to 25, the constant
EINDEX_OUT_OF_BOUNDS uses the expression 0x1 << 16 + 1 which is parsed as 0x1 <<
(16 + 1) due to operator precedence. To fix this and match the intended value
(0x1 << 16) + 1, add parentheses around the shift operation like (0x1 << 16) + 1
to clarify precedence and silence the Clippy warning.
| if removal_position | ||
| .checked_add(length) | ||
| .is_none_or(|end| end > from_len) | ||
| || insert_position > to_len | ||
| { |
There was a problem hiding this comment.
Option::is_none_or does not exist – code will not compile
checked_add(length).is_none_or(|end| end > from_len) relies on an unstable/non-existent helper.
Replace with explicit logic or map_or / is_some_and:
- if removal_position
- .checked_add(length)
- .is_none_or(|end| end > from_len)
- || insert_position > to_len
+ let end = removal_position.checked_add(length);
+ if end.is_none() || end.unwrap() > from_len || insert_position > to_len
{
return Err(SafeNativeError::Abort {
abort_code: EINDEX_OUT_OF_BOUNDS,
});
}Compilation will fail until this is addressed.
📝 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.
| if removal_position | |
| .checked_add(length) | |
| .is_none_or(|end| end > from_len) | |
| || insert_position > to_len | |
| { | |
| let end = removal_position.checked_add(length); | |
| if end.is_none() || end.unwrap() > from_len || insert_position > to_len { | |
| return Err(SafeNativeError::Abort { | |
| abort_code: EINDEX_OUT_OF_BOUNDS, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In crates/natives/src/move_stdlib/vector.rs around lines 56 to 60, the code uses
the non-existent method Option::is_none_or, causing compilation failure. Replace
the call to is_none_or with equivalent logic using stable Option methods such as
map_or or is_some_and to check if checked_add(length) is None or if the result
exceeds from_len. This will restore compatibility and allow the code to compile.
| for (i in 0..len) { | ||
| let element = data.borrow(i); | ||
| if (&element.key == &key) { | ||
| vector::push_back(data, Element { key, value }); | ||
| vector::swap(data, i, len); | ||
| let Element { key, value } = vector::pop_back(data); | ||
| data.push_back(Element { key, value }); | ||
| data.swap(i, len); | ||
| let Element { key, value } = data.pop_back(); | ||
| return (std::option::some(key), std::option::some(value)) | ||
| }; | ||
| i = i + 1; | ||
| }; | ||
| vector::push_back(&mut self.data, Element { key, value }); | ||
| self.data.push_back(Element { key, value }); | ||
| (std::option::none(), std::option::none()) |
There was a problem hiding this comment.
Illegal mutation while an immutable reference to the same vector is live
element is an immutable reference obtained via data.borrow(i) (line 124).
data.push_back(..) (line 126) mutates the vector while that reference is still in scope, violating Move’s borrow rules and preventing the file from compiling.
- let element = data.borrow(i);
- if (&element.key == &key) {
- data.push_back(Element { key, value });
- data.swap(i, len);
- let Element { key, value } = data.pop_back();
- return (std::option::some(key), std::option::some(value))
- };
+ if (&data.borrow(i).key == &key) {
+ // Stage new value at the end, then swap-remove without
+ // keeping any live refs to `data`.
+ data.push_back(Element { key, value });
+ data.swap(i, len);
+ let Element { key: old_k, value: old_v } = data.pop_back();
+ return (std::option::some(old_k), std::option::some(old_v));
+ };This drops the immutable reference before the mutation and also avoids shadowing key/value.
📝 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.
| for (i in 0..len) { | |
| let element = data.borrow(i); | |
| if (&element.key == &key) { | |
| vector::push_back(data, Element { key, value }); | |
| vector::swap(data, i, len); | |
| let Element { key, value } = vector::pop_back(data); | |
| data.push_back(Element { key, value }); | |
| data.swap(i, len); | |
| let Element { key, value } = data.pop_back(); | |
| return (std::option::some(key), std::option::some(value)) | |
| }; | |
| i = i + 1; | |
| }; | |
| vector::push_back(&mut self.data, Element { key, value }); | |
| self.data.push_back(Element { key, value }); | |
| (std::option::none(), std::option::none()) | |
| for (i in 0..len) { | |
| if (&data.borrow(i).key == &key) { | |
| // Stage new value at the end, then swap-remove without | |
| // keeping any live refs to `data`. | |
| data.push_back(Element { key, value }); | |
| data.swap(i, len); | |
| let Element { key: old_k, value: old_v } = data.pop_back(); | |
| return (std::option::some(old_k), std::option::some(old_v)); | |
| }; | |
| }; | |
| self.data.push_back(Element { key, value }); | |
| (std::option::none(), std::option::none()) |
🤖 Prompt for AI Agents
In precompile/modules/initia_stdlib/sources/simple_map.move around lines 123 to
133, the code holds an immutable reference to an element from the vector using
data.borrow(i) and then mutates the vector with data.push_back while that
reference is still live, violating borrow rules. To fix this, restructure the
code to ensure the immutable reference is dropped before any mutation occurs on
the vector. Avoid shadowing the key and value variables by using distinct names
or extracting the element's key and value before the mutation, then perform the
push_back and other mutations after the reference is no longer in scope.
| self: &mut TableWithLength<K, V>, key: K, default: V | ||
| ): &mut V { | ||
| if (self.inner.contains(key)) { | ||
| self.inner.borrow_mut(key) | ||
| } else { | ||
| self.inner.add(key, default); | ||
| self.length += 1; | ||
| self.inner.borrow_mut(key) | ||
| } |
There was a problem hiding this comment.
key reused after move; triggers compile error
Inside borrow_mut_with_default, key is moved into add, then
immediately reused:
self.inner.add(key, default); // moves `key`
self.inner.borrow_mut(key); // use-after-moveSimple fix:
- self.inner.add(key, default);
+ self.inner.add(copy key, default);🤖 Prompt for AI Agents
In precompile/modules/initia_stdlib/sources/table_with_length.move around lines
73 to 81, the variable `key` is moved when passed to `self.inner.add(key,
default)`, causing a compile error when it is reused in
`self.inner.borrow_mut(key)`. To fix this, clone or copy `key` before passing it
to `add` so that the original `key` remains valid for the subsequent
`borrow_mut` call.
| const NULL_INDEX: u64 = 0; | ||
| const FIRST_INDEX: u64 = 10; // keeping space for usecase-specific values |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document the rationale for special index values.
The constants NULL_INDEX and FIRST_INDEX use specific values (0 and 10) but the reason for reserving indices 1-9 is not documented.
const NULL_INDEX: u64 = 0;
- const FIRST_INDEX: u64 = 10; // keeping space for usecase-specific values
+ const FIRST_INDEX: u64 = 10; // Indices 1-9 are reserved for future use-case specific values (e.g., inline small values)📝 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.
| const NULL_INDEX: u64 = 0; | |
| const FIRST_INDEX: u64 = 10; // keeping space for usecase-specific values | |
| const NULL_INDEX: u64 = 0; | |
| const FIRST_INDEX: u64 = 10; // Indices 1-9 are reserved for future use-case specific values (e.g., inline small values) |
🤖 Prompt for AI Agents
In precompile/modules/initia_stdlib/sources/storage_slots_allocator.move around
lines 27 to 28, the constants NULL_INDEX and FIRST_INDEX are assigned values 0
and 10 respectively, but there is no explanation for reserving indices 1 through
9. Add comments explaining why indices 1-9 are reserved for usecase-specific
values to clarify the rationale behind these special index values.
| assert!( | ||
| self.should_reuse, | ||
| error::invalid_argument(ECANNOT_HAVE_SPARES_WITHOUT_REUSE) | ||
| ); | ||
| for (i in 0..num_to_allocate) { | ||
| let slot_index = self.next_slot_index(); | ||
| self.maybe_push_to_reuse_queue(slot_index); | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for slot pre-allocation.
The allocate_spare_slots function could benefit from an upper bound check to prevent excessive pre-allocation that could lead to resource exhaustion.
public fun allocate_spare_slots<T: store>(
self: &mut StorageSlotsAllocator<T>, num_to_allocate: u64
) {
assert!(
self.should_reuse,
error::invalid_argument(ECANNOT_HAVE_SPARES_WITHOUT_REUSE)
);
+ // Prevent excessive pre-allocation
+ assert!(
+ num_to_allocate <= 1000,
+ error::invalid_argument(EINVALID_ARGUMENT)
+ );
for (i in 0..num_to_allocate) {🤖 Prompt for AI Agents
In precompile/modules/initia_stdlib/sources/storage_slots_allocator.move around
lines 83 to 91, add a validation check before the loop in allocate_spare_slots
to enforce an upper limit on num_to_allocate. This prevents excessive
pre-allocation that could cause resource exhaustion. Define a reasonable maximum
allowed value and assert that num_to_allocate does not exceed this limit before
proceeding with slot allocation.
| inline fun initialize_permission_address( | ||
| permissions_storage_addr: address | ||
| ) { | ||
| move_to( | ||
| &create_signer(permissions_storage_addr), | ||
| // Each key is ~100bytes, the value is 12 bytes. | ||
| PermissionStorage::V1 { | ||
| perms: big_ordered_map::new_with_config(40, 35, false) | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document the BigOrderedMap configuration parameters.
The initialization uses specific parameters (40, 35, false) for the BigOrderedMap without explanation of what these values represent or why they were chosen.
move_to(
&create_signer(permissions_storage_addr),
- // Each key is ~100bytes, the value is 12 bytes.
+ // Each key is ~100bytes, the value is 12 bytes.
+ // Parameters: (node_capacity: 40, reuse_slots: 35, inline_values: false)
+ // These values are optimized for typical permission usage patterns
PermissionStorage::V1 {
perms: big_ordered_map::new_with_config(40, 35, false)
}
);🤖 Prompt for AI Agents
In precompile/modules/initia_stdlib/sources/permissioned_signer.move around
lines 212 to 222, the BigOrderedMap is initialized with parameters (40, 35,
false) without any explanation. Add comments explaining what each parameter
represents (e.g., initial capacity, load factor, or ordering flag) and why these
specific values were chosen to clarify their purpose for future maintainers.
| enum IteratorPtr<K> has copy, drop { | ||
| End, | ||
| Some { | ||
| /// The node index of the iterator pointing to. | ||
| node_index: u64, | ||
|
|
||
| /// Child iter it is pointing to | ||
| child_iter: ordered_map::IteratorPtr, | ||
|
|
||
| /// `key` to which `(node_index, child_iter)` are pointing to | ||
| /// cache to not require borrowing global resources to fetch again | ||
| key: K | ||
| } | ||
| } |
There was a problem hiding this comment.
Document iterator invalidation behavior.
The iterator implementation doesn't appear to handle the case where the map is modified while iterators exist. This could lead to incorrect behavior if an iterator is used after the map structure changes.
Consider adding documentation warning users that iterators are invalidated by any modification to the map, or implement a generation/version counter to detect such cases.
Also applies to: 742-782
🤖 Prompt for AI Agents
In precompile/modules/initia_stdlib/sources/big_ordered_map.move around lines
110 to 123 and also lines 742 to 782, the iterator implementation lacks
documentation about invalidation when the map is modified. Add clear comments to
the iterator code explaining that any modification to the map invalidates
existing iterators, warning users about potential incorrect behavior if
iterators are used after changes. Optionally, consider implementing a generation
or version counter mechanism to detect and prevent use of invalidated iterators.
Summary by CodeRabbit
New Features
BigOrderedMapand a sorted-vectorOrderedMapfor efficient ordered key-value storage and iteration.cmp) and memory (mem) modules with native functions for value comparison and safe memory operations.move_rangefunction for efficient element movement.Improvements
vector,option,string,bit_vector,table, etc.) to useselfas the receiver, improving API consistency and readability.signer.SimpleMapin favor of new map implementations.Bug Fixes
Documentation
Style