feat: account abstraction #212
Conversation
WalkthroughThis update introduces a comprehensive account abstraction and authentication framework, including Move modules, Rust and Go interfaces, and extensive test coverage. It adds new Move modules for account abstraction, permissioned signers, ordered and big ordered maps, and storage slot allocators. The VM, native functions, and type system are extended to support authenticators, derivable accounts (Ethereum/Solana), and authentication message execution. Method signatures across Move standard library modules are refactored for consistency, and new utilities and error handling improvements are integrated throughout the codebase. Changes
Sequence Diagram(s)Account Abstraction Authentication FlowsequenceDiagram
participant User
participant GoAPI
participant GoVM
participant RustVM
participant MoveVM
participant MoveModule
User->>GoAPI: Call ExecuteAuthenticate(env, sender, signature)
GoAPI->>GoVM: Serialize inputs, call C FFI execute_authenticate
GoVM->>RustVM: Pass env, sender, signature
RustVM->>MoveVM: Deserialize, create AuthenticateMessage
MoveVM->>MoveModule: Call account_abstraction::authenticate(signer, func_info, auth_data)
MoveModule->>MoveModule: Verify function enabled, call native dispatchable_authenticate
MoveModule->>MoveModule: Run authentication logic (e.g., signature check)
MoveModule-->>MoveVM: Return signer (if valid) or error
MoveVM-->>RustVM: Return result or error
RustVM-->>GoVM: Marshal result
GoVM-->>GoAPI: Return authentication result
GoAPI-->>User: Return result (success or error)
Derivable Account Address Flow (Ethereum/Solana)sequenceDiagram
participant User
participant MoveVM
participant DerivableModule
User->>MoveVM: Call derive_account_address(func_info, abstract_public_key)
MoveVM->>DerivableModule: Serialize inputs, hash with SHA3-256
DerivableModule-->>MoveVM: Return derived address
MoveVM-->>User: Return address
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: 36
🧹 Nitpick comments (55)
Cargo.toml (1)
120-120: Pinningbs58 = 0.5.1– verify you need this exact patch-level
bs580.5.1 is a few patch versions behind the latest 0.5.y line. Patch releases often carry bug & security fixes while keeping the public API intact.
Consider bumping to the latest 0.5.*, or justify the strict pin.crates/types/Cargo.toml (1)
23-23:serde_jsonadded – consider gating behind a non-std feature
serde_jsonpulls instdand alloc-heavy code. Ifinitia-move-typesis ever compiled forno_stdtargets (e.g. light-client, wasm), guard this dep with:serde_json = { workspace = true, optional = true } [features] default = ["serde_json"]and let downstream crates opt-in explicitly.
crates/e2e-move-tests/src/tests/output.rs (1)
37-38: MagicNoneargument hurts readabilityThe extra
None(signatures) argument is non-self-describing and easy to mis-order. Consider a builder or named struct instead of widening the positional API:h.run_entry_function(EntryCall { senders: vec![sender.unwrap()], function: str::parse(entry).unwrap(), ty_args, args, signatures: None, });precompile/modules/initia_stdlib/sources/function_info.move (1)
8-8: Friend exposure – double-check thataccount_abstractionreally needs full accessAdding
initia_std::account_abstractionas a friend grants it visibility to everyfriend/public(friend)item in this module, includingcheck_dispatch_type_compatibilityand the native helpers.
If the new module only needs a subset of this surface, consider exposing a narrower API instead of blanket friend access to keep the trusted-base as small as possible.
Otherwise, please add a short comment explaining the security rationale for future maintainers.precompile/modules/initia_stdlib/sources/account.move (1)
12-14: Two new friends widen the attack surface – audit required
permissioned_signerandaccount_abstractioncan now bypass the normal visibility barriers ofaccount.
• Verify that both modules only call the minimal set of internal functions they truly need.
• Pay extra attention to native functions likeset_account_infothat can mutate critical state.If all usages are intentional, a comment near the friend list describing why each friend is necessary will save time for future reviewers.
crates/gas/src/lib.rs (1)
21-21: Public module added but not re-exported – consistency check
pub mod abstract_algebra;exposes the sub-module but, unlikealgebra, its symbols are not re-exported withpub use.
If consumers are expected to writegas::abstract_algebra::Expr, this is fine.
If the intent is to mirror the existingpub use algebra::*;pattern, add:+pub use abstract_algebra::*;Otherwise, no action needed.
crates/e2e-move-tests/Cargo.toml (1)
26-30: Pin versions / feature flags for new crypto & encoding crates
ed25519-consensus,rand_core,libsecp256k1,tiny-keccak, andbs58are added as workspace deps without explicit versions or reduced feature sets.
This can:
- Make the build non-deterministic if different crates in the workspace pull incompatible versions.
- Inflate the binary because default features often enable
std, test vectors, etc.Recommendations:
ed25519-consensus = { version = "2.2", default-features = false, features = ["std"] } rand_core = { version = "0.6", default-features = false } libsecp256k1 = { version = "0.7", default-features = false } tiny-keccak = { version = "2.0", default-features = false } bs58 = { version = "0.5", default-features = false }• Explicit versions aid reproducibility.
• Turning off unneeded default features saves compile time and code size.Please verify the exact feature set the tests need and pin accordingly.
crates/e2e-move-tests/src/tests/public_key_authenticator.data/pack/Move.toml (1)
1-10: Minor manifest nit – duplicate address aliasesBoth
stdandinitia_stdare mapped to0x1.
Ifinitia_stdmerely re-exportsstd, the second alias is redundant and can confusemove packageaddress resolution in the future.
Keep only the alias that is actually imported by the sources.crates/e2e-move-tests/src/tests/move_unit.rs (1)
42-42:"test"chain-id hard-coded – expose a constantHard-coding the chain id string in multiple call-sites makes future changes error-prone.
Extract aconst TEST_CHAIN_ID: &str = "test";near the top and use it here and elsewhere.crates/natives/src/lib.rs (2)
60-64: Order-agnostic macro helps, but duplicate names will silently overwrite
add_natives_from_module!simply pushes tuples; if another module accidentally re-uses the same$module_name, the earlier natives remain but will be shadowed inmake_table_from_iter, leading to hard-to-trace dispatch errors.
Consider detecting duplicates at build-time:- natives.extend( - $natives.map(|(func_name, func)| ($module_name.to_string(), func_name, func)), - ); + for (func_name, func) in $natives { + let key = ($module_name, &func_name); + debug_assert!( + !natives.iter().any(|(m, f, _)| m == key.0 && f == key.1), + "native `{}` already registered in module `{}`", + func_name, + $module_name + ); + natives.push(($module_name.to_string(), func_name, func)); + }
95-98: Consistent placement of new modules
permissioned_signernatives are added near the end while thepub moddeclaration appears much earlier. Keeping declaration and registration blocks in the same relative order (alphabetical or logical) simplifies grepping and reduces merge-conflict frequency.crates/e2e-move-tests/src/tests/args.rs (1)
51-57: RepeatedNoneliterals hurt readabilityAfter the new
signatures: Option<Vec<Vec<u8>>>parameter, every invocation now ends withNone. This is noisy and prone to drift if the default ever changes. Consider:
- Give the parameter a default of
Option::Nonevia a builder pattern; or- Provide helper wrappers (
run_entry_function_no_sig, etc.) for the common case.This keeps call-sites concise and isolates future refactors.
Also applies to: 97-103, 778-784, 832-842, 860-874
crates/types/src/lib.rs (1)
3-24: Public API surface expanded – add crate-level docsFour new top-level modules are exposed. A short description in
lib.rs(doc comment) summarising their purpose and stability expectations helps downstream users and prevents accidental breaking changes.crates/e2e-move-tests/src/tests/std_coin.rs (2)
9-17: Expose symbol/metadata as public constants for broader reuse
pub(crate)is fine for the current crate, butSTD_COIN_SYMBOLandstd_coin_metadata()will likely be useful in downstream crates (e.g. external integration tests or helper libraries). Consider promoting them topuband adding an#[inline(always)]on the function to encourage LLVM inlining:-pub(crate) const STD_COIN_SYMBOL: &[u8] = b"STDC"; +pub const STD_COIN_SYMBOL: &[u8; 4] = b"STDC"; - pub(crate) fn std_coin_metadata() -> AccountAddress { +#[inline(always)] +pub fn std_coin_metadata() -> AccountAddress {
43-49: Avoid repeated parsing/cloning inside the test loop
str::parse(entry)andty_args.clone()are executed for every test case although both values are never mutated.
Pre-parsing once and passing references cuts a small amount of CPU and allocation overhead and makes the intent explicit.-for (senders, entry, ty_args, args, exp_output) in tests { - ... - let exec_output = h.run_entry_function( - senders, - str::parse(entry).unwrap(), - ty_args.clone(), - args, - None, - ); +for (senders, entry, ty_args, args, exp_output) in tests { + let entry_fun = str::parse::<move_core_types::language_storage::FunctionId>(entry) + .expect("valid function id"); + + let exec_output = h.run_entry_function( + senders, + entry_fun, + &ty_args, // borrow instead of clone + args, + None, + );precompile/modules/move_stdlib/sources/signer.move (1)
2-23: Minor spelling fixes in documentation
verfied→verified
discrepency→discrepancyKeeping the docs typo-free helps external users copy-paste without propagating mistakes.
crates/compiler/src/extensions.rs (1)
26-31: Avoid magic constants – move"test"andNoneinto descriptive helpers
NativeBlockContext::new("test".to_string(), 0, 0)and the matching transaction context appear in several places.
Creating a small helper such asNativeBlockContext::test()/NativeTransactionContext::test()keeps the signature changes local and removes repetition.No functional impact, but improves maintainability.
precompile/modules/move_stdlib/sources/mem.move (1)
15-18: Minor naming / readability nitUsing the identifier
refcan be confusing becauserefis also used as
a pattern keyword in Move. Consider renaming it toplaceorslot
for clarity.crates/natives/src/permissioned_signer.rs (1)
99-107: Duplicate native mapping – risk of name collision.Both
"is_permissioned_signer_impl"and"is_permissioned_signer"map to the same Rust function.
If the Move module exports only one of these names the other entry is dead weight; if it exports both the duplication is fine but worth a comment explaining why two aliases are required.crates/types/src/move_utils/mod.rs (1)
17-33: MissingEq,PartialEq,HashandDisplayonMemberId.
MemberIdis likely to be used as a map key and to appear in log / error messages.
Deriving the standard traits and implementingDisplaykeeps boiler-plate out of downstream code.-#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct MemberId { @@ impl fmt::Display for MemberId { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{}::{}::{}", self.module_id.address(), self.module_id.name(), self.member_id) } }precompile/modules/initia_stdlib/tests/account_abstraction_tests.move (1)
7-13: Avoid silently duplicatingsignerresources in tests
invalid_authenticateforges a new signer but leaves the originalaccountsigner to be implicitly dropped.
Even in#[test_only]code this can hide linear-resource violations that the production logic must never allow.Consider explicitly consuming or destroying the incoming signer (or renaming it to
_accountto make the discard intentional) before returning the forged one:let addr = signer::address_of(&account); let cref = object::create_object(addr, true); let forged = object::generate_signer(&cref); // Explicitly mark the original as unused to avoid accidental leaks account; forgedcrates/types/src/move_utils/as_move_value.rs (1)
22-29: Avoid unnecessaryStringcloning inas_move_value
String::as_move_valuecurrently clones the whole string:self.clone().into_bytes()You can eliminate the extra allocation by borrowing the bytes slice and converting it once:
- MoveValue::Struct(MoveStruct::Runtime(vec![self - .clone() - .into_bytes() - .as_move_value()])) + MoveValue::Struct(MoveStruct::Runtime(vec![self.as_bytes().to_vec().as_move_value()]))Minor, but saves an allocation in hot code paths such as event serialization.
precompile/modules/move_nursery/sources/acl.move (1)
38-45: Error-code hygiene
ECONTAIN = 0collides with the Move VM’s generic “unknown error” convention; consider starting module-specific codes at0x1.crates/gas/src/initia_stdlib.rs (1)
17-20: Keep variable naming consistentExisting entries use both
_baseand_base_cost. Pick one style (prefer_base) to avoid confusion.crates/types/src/user_transaction_context.rs (2)
24-30: Avoid unnecessary clone
entry_function_payload(&self)returnsOption<EntryFunctionPayload>and clones every call. Return&Option<EntryFunctionPayload>(or anOption<&EntryFunctionPayload>) to eliminate heap copies.- pub fn entry_function_payload(&self) -> Option<EntryFunctionPayload> { - self.entry_function_payload.clone() + pub fn entry_function_payload(&self) -> Option<&EntryFunctionPayload> { + self.entry_function_payload.as_ref() }
59-64: Derive Serde forMultisigPayloadDown-stream components likely need to (de)serialize this struct just like the others.
-#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize, Deserialize)]precompile/modules/move_stdlib/sources/bcs.move (1)
44-47: Finish the spec or drop the TODO
spec constant_serialized_size { … }is currently a stub withpragma opaque;. Either supply the real post-condition or remove the block to avoid misleading verifiers.crates/e2e-move-tests/src/tests/solana_derivable_account_abstraction.rs (1)
71-74: Comment driftThe comment says “derive account address from ethereum public key” but the test is Solana-specific. Update for clarity.
types/bcs.go (1)
1966-1986: Option helpers leak zero-length slicesWhen
value != nilbutlen(*value)==0, serialisation writes an empty vector tag.
Upon round-trip the deserialised slice is[]which is ≠nil.
If the option’s semantic meaning is “absent vs present”, treat empty slice as absent
or document the distinction to avoid subtle bugs.Also applies to: 2181-2197
precompile/modules/move_stdlib/sources/cmp.move (2)
95-106: Redundant assertions – keep tests minimalLines 95-106 duplicate checks already performed at 81-91.
Removing the repetition shortens compile time and keeps the test focused.
43-46:spec compareis still a stubThe specification is marked
pragma opaque;with a TODO.
Consider adding at least a high-level post-condition (exists o: Ordering …) so
formal-verification users get value immediately.crates/types/src/function_info.rs (2)
33-41:Displayuses hex without0x-prefix
AccountAddress::to_hex()already includes0x; concatenating with::is fine, but if
to_hex_literal()is ever substituted you could lose the prefix.
Consider delegating toformat!("{:#x}", self.module_address)to lock this in.
64-72:as_move_valuecreates runtime struct without layout checkIf the Move side changes the field ordering, serialization will break silently.
Embedding aStaticMoveStructTagconstant and usingMoveStruct::with_fieldswould give an
early error instead of runtime corruption.crates/e2e-move-tests/src/harness.rs (3)
75-83: Avoid hard-coding the test chain-id in multiple places
"test".to_string()now appears in at least four call-sites. Consider storing it in aconst TEST_CHAIN_ID: &stror letting the caller pass the chain-id toMoveHarness::new()to reduce repetition and future drift.
127-132: Minor duplication – build & send once
run_entry_function{,_with_json}both recreate an identicalMessagethen immediately forward torun_message. A small helper likeself.exec(message, signatures)would keep these wrappers truly paper-thin.Also applies to: 140-145
313-327: Factor the Env construction into a single helper
run_messageandrun_message_with_statebuild almost identicalEnvobjects. Pulling this intofn make_env(&self, sigs: Option<Vec<Vec<u8>>>)would eliminate boilerplate and guarantee the two paths never diverge.Also applies to: 349-357
crates/natives/src/block.rs (1)
50-63: Return the standardStringstruct, not a naked vectorThe Move layer expects
0x1::string::String. Packing a one-field struct manually works but is brittle—consider using the helper incrate::helpers::make_string_value(&chain_id)(or similar) to guarantee field ordering ifStringever changes.precompile/modules/move_nursery/sources/capability.move (1)
152-153: Vector helper logic is now O(n²) in worst-caseRepeated
contains()+push_back/index_ofcauses a linear scan twice. For large delegate lists consider keeping atable<address, bool>or sort & binary-search. Not critical now, but worth noting.Also applies to: 210-220
crates/e2e-move-tests/src/tests/account_abstraction.rs (1)
116-120: Redundantas_bytes().to_vec()before BCS serialization
bcs::to_bytesaccepts&str; the intermediateVec<u8>allocation is unnecessary.- bcs::to_bytes(&module_name.as_bytes().to_vec()).unwrap(), + bcs::to_bytes(&module_name).unwrap(),precompile/modules/initia_stdlib/sources/table_with_length.move (1)
75-81: Avoid double lookup inborrow_mut_with_default
contains(key)followed byborrow_mut(key)incurs two table searches.
Usingself.inner.borrow_mut_with_default(key, default)(if available) or callingaddand directly returning the fresh reference avoids the extra scan.precompile/modules/move_stdlib/sources/bit_vector.move (1)
94-113: Mixed use oflengthfield vslength()accessor
shift_leftuses the rawself.lengthfield, whereas other routines rely onlength().
Sticking to the accessor everywhere keeps the invariant obvious and reduces chances of future divergence if the representation changes.precompile/modules/initia_stdlib/sources/table.move (2)
67-75: First arg still namedtable– breaks the new method-style conventionEvery public API in this PR was migrated to
self-receiver style, yetborrow_with_defaultstill uses the legacytableparameter and callscontains(table, …)/borrow(table, …).
This is only cosmetic, but keeping a single outlier will confuse users and hurts mechanical refactors down the road.- public fun borrow_with_default<K: copy + drop, V>( - table: &Table<K, V>, key: K, default: &V + public fun borrow_with_default<K: copy + drop, V>( + self: &Table<K, V>, key: K, default: &V ): &V { - if (!contains(table, copy key)) { + if (!contains(self, copy key)) { default } else { - borrow(table, copy key) + borrow(self, copy key) } }
253-260:destroy_known_empty_unsafeis powerful – gate it strongerThe new friend-only helper bypasses the emptiness check and drops storage unconditionally.
Consider:
- Adding an inline doc comment clearly stating the invariants the caller MUST uphold.
- Renaming to
destroy_unchecked(consistent withdrop_unchecked) or adding an_internalsuffix to discourage casual use.No code change required if you are comfortable with current risk surface.
precompile/modules/move_stdlib/sources/vector.move (2)
29-33: Constant toggle can be aconst bool = false;in prod
USE_MOVE_RANGEis permanently set totrue; theelsebranches are now dead code that the byte-code verifier still keeps around.
If the intention is a compile-time switch, consider:
- Removing the flag and deleting
elsepaths, or- Guarding with a feature flag at build time instead of a run-time constant.
This keeps byte-code smaller and avoids two divergent code paths to maintain.
319-343:removebranch mutates loop index in the RHS expression – fragileThe idiom
self.swap(i, { i += 1; i });relies on subtle evaluation order and is hard to read. Using
move_rangehere as well would both speed up mid-vector removals and make the code clearer.Minor, but worth tidying while the function is being touched.
precompile/modules/initia_stdlib/sources/account_abstraction/account_abstraction.move (1)
131-135: Vector growth inside address derivation can allocate twice
bytes.append(...)followed bybytes.push_back(...)may reallocate twice for largeFunctionInfo.
You can pre-reserve the final capacity:let mut bytes = bcs::to_bytes(&derivable_func_info); bytes.reserve(abstract_public_key.length() + 1); bytes.append(bcs::to_bytes(abstract_public_key)); bytes.push_back(DERIVABLE_ABSTRACTION_DERIVED_SCHEME);Not critical, but gives a small gas win on hot paths.
precompile/modules/initia_stdlib/sources/ordered_map.move (4)
50-56: Consider simplifying to a struct instead of enum with single variant.The
OrderedMapenum currently has only one variantSortedVectorMap. Unless there are concrete plans for additional implementations, this adds unnecessary complexity.Consider simplifying to:
-enum OrderedMap<K, V> has drop, copy, store { - /// sorted-vector based implementation of OrderedMap - SortedVectorMap { - /// List of entries, sorted by key. - entries: vector<Entry<K, V>> - } -} +struct OrderedMap<K, V> has drop, copy, store { + /// List of entries, sorted by key. + entries: vector<Entry<K, V>> +}
188-194: Track optimization opportunity for batch operations.The TODO indicates a known optimization: sorting keys and values before bulk insertion could improve performance from O(n²) to O(n log n).
Would you like me to implement the optimized version or create an issue to track this enhancement?
630-649: Known performance limitation: O(n log n) iteration.The current public API forces an inefficient implementation. The TODO comments indicate this will be improved when iterator APIs become public or when Move supports private functions in inline functions.
Consider exposing a limited public iterator API to enable O(n) iteration for common use cases while maintaining backward compatibility.
20-24: Iterator API accessibility limitation.The iterator functions are restricted to friend modules, limiting their utility. The TODO indicates this is temporary pending Move language improvements for reference support in structs.
Document the friend module requirements clearly in the module documentation to help developers understand which modules can use the iterator functionality.
precompile/modules/initia_stdlib/sources/bigdecimal.move (5)
64-67: Consider borrowing instead of moving for trivial getters
get_scaledcurrently consumes the wholeBigDecimal, even though it only needs an immutable view on the innerscaledfield.
Changing the signature topublic fun get_scaled(self: &BigDecimal): &BigUint(or returningBigUintafter an internalcopy) would:
- avoid a potentially expensive copy of large
BigUintvalues,- make the function usable without forcing the caller to copy / re-initialise
BigDecimal.Same observation applies to the downstream call‐sites that use this getter.
74-76: Same move-vs-borrow concern forget_scaled_le_bytesFor consistency with the getter above and to limit unnecessary copies, consider:
public fun get_scaled_le_bytes(self: &BigDecimal): vector<u8> { biguint::to_le_bytes(copy self.scaled) }(The
copyonself.scaledis cheap compared with duplicating the wholeBigDecimal.)
141-159: Comparators could take&BigDecimalto eliminate large copiesAll five comparator helpers (
eq/lt/le/gt/ge) take full-value parameters. GivenBigDecimalwraps an arbitrary-precisionBigUint, each call duplicates two potentially large values.Borrowing would preserve semantics while improving performance:
public fun eq(self: &BigDecimal, other: &BigDecimal): bool { biguint::eq(self.scaled, other.scaled) }Same pattern applies to the other four functions.
161-167:is_zero/is_one– same borrowing suggestionThese predicates only need read access; switching to
&BigDecimalremoves an otherwise dead copy.
297-343: Cast helpers work but can avoid copies
truncate/round_up/ceiland their*_u*wrappers are correct; however each of these getters consumes the value. As withget_scaled, consider taking&BigDecimalto avoid moving large values around.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockapi/libcompiler.dylibis excluded by!**/*.dylibapi/libmovevm.dylibis excluded by!**/*.dylib
📒 Files selected for processing (83)
.cargo/config.toml(1 hunks)Cargo.toml(2 hunks)api/compiler_test.go(0 hunks)crates/compiler/src/extensions.rs(1 hunks)crates/e2e-move-tests/Cargo.toml(1 hunks)crates/e2e-move-tests/src/harness.rs(7 hunks)crates/e2e-move-tests/src/tests/account_abstraction.rs(1 hunks)crates/e2e-move-tests/src/tests/args.rs(7 hunks)crates/e2e-move-tests/src/tests/cache.rs(1 hunks)crates/e2e-move-tests/src/tests/cosmos.rs(1 hunks)crates/e2e-move-tests/src/tests/ethereum_derivable_account_abstraction.rs(1 hunks)crates/e2e-move-tests/src/tests/infinite_loop.rs(1 hunks)crates/e2e-move-tests/src/tests/memory_quota.rs(4 hunks)crates/e2e-move-tests/src/tests/mod.rs(1 hunks)crates/e2e-move-tests/src/tests/move_unit.rs(1 hunks)crates/e2e-move-tests/src/tests/output.rs(1 hunks)crates/e2e-move-tests/src/tests/public_key_authenticator.data/pack/Move.toml(1 hunks)crates/e2e-move-tests/src/tests/public_key_authenticator.data/pack/sources/PublicKeyAuthenticator.move(1 hunks)crates/e2e-move-tests/src/tests/simple_authenticator.data/pack/Move.toml(1 hunks)crates/e2e-move-tests/src/tests/simple_authenticator.data/pack/sources/SimpleAuthenticator.move(1 hunks)crates/e2e-move-tests/src/tests/solana_derivable_account_abstraction.rs(1 hunks)crates/e2e-move-tests/src/tests/staking.rs(1 hunks)crates/e2e-move-tests/src/tests/std_coin.data/pack/sources/StdCoin.move(1 hunks)crates/e2e-move-tests/src/tests/std_coin.rs(3 hunks)crates/e2e-move-tests/src/tests/table.rs(1 hunks)crates/gas/Cargo.toml(1 hunks)crates/gas/src/abstract_algebra.rs(1 hunks)crates/gas/src/initia_stdlib.rs(4 hunks)crates/gas/src/lib.rs(1 hunks)crates/gas/src/move_stdlib.rs(3 hunks)crates/natives/src/account_abstraction.rs(1 hunks)crates/natives/src/block.rs(5 hunks)crates/natives/src/interface/context.rs(1 hunks)crates/natives/src/lib.rs(4 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)crates/natives/src/transaction_context.rs(4 hunks)crates/types/Cargo.toml(1 hunks)crates/types/src/authenticator.rs(1 hunks)crates/types/src/env.rs(3 hunks)crates/types/src/function_info.rs(1 hunks)crates/types/src/lib.rs(1 hunks)crates/types/src/move_utils/as_move_value.rs(1 hunks)crates/types/src/move_utils/mod.rs(1 hunks)crates/types/src/user_transaction_context.rs(1 hunks)crates/vm/src/initia_vm.rs(12 hunks)precompile/modules/initia_stdlib/sources/account.move(1 hunks)precompile/modules/initia_stdlib/sources/account_abstraction/account_abstraction.move(1 hunks)precompile/modules/initia_stdlib/sources/account_abstraction/auth_data.move(1 hunks)precompile/modules/initia_stdlib/sources/account_abstraction/base16.move(1 hunks)precompile/modules/initia_stdlib/sources/account_abstraction/common_account_abstractions_utils.move(1 hunks)precompile/modules/initia_stdlib/sources/account_abstraction/ethereum_derivable_account.move(1 hunks)precompile/modules/initia_stdlib/sources/account_abstraction/solana_derivable_account.move(1 hunks)precompile/modules/initia_stdlib/sources/bcs_stream.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/block.move(1 hunks)precompile/modules/initia_stdlib/sources/function_info.move(1 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/initia_stdlib/sources/transaction_context.move(2 hunks)precompile/modules/initia_stdlib/tests/account_abstraction_tests.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)types/bcs.go(4 hunks)
💤 Files with no reviewable changes (1)
- api/compiler_test.go
🔇 Additional comments (51)
Cargo.toml (1)
81-81: Confirmeitheris required at the workspace scope
eitheris lightweight and harmless, but adding it to[workspace.dependencies]makes it a hard transitive dep for every crate unless each crate opts-out withdefault-features = false, optional = true.
Double-check that (a) only the crates that truly need it enable it, and (b) there is no version clash with indirect deps already pullingeitherin.crates/gas/Cargo.toml (1)
14-14:initia-move-gasnow depends oneither– audit binary sizeGas-metering code is part of the VM’s critical path. Although
eitheris tiny, every extra dep is compiled into the deterministic‐execution footprint.
Runcargo bloat --release -p initia-move-gasto ensure the new generic wrappers don’t inflate code size or change hashing.crates/e2e-move-tests/src/tests/std_coin.data/pack/sources/StdCoin.move (1)
24-25: Trailing comma removal LGTMMatches current Move parser expectations; no functional impact.
crates/e2e-move-tests/src/tests/memory_quota.rs (1)
24-25: Signature placeholder looks goodThe additional
Noneargument aligns with the updatedrun_entry_functionsignature (Option<Vec<Vec<u8>>>). No further changes required.Also applies to: 34-35, 60-61, 73-74
crates/e2e-move-tests/src/tests/cache.rs (1)
36-37: Call site updated correctly
run_message(msg, None)matches the new signature. No issues spotted..cargo/config.toml (1)
6-6: Rationale for 8 MiBRUST_MIN_STACKshould be documentedBumping the minimum thread stack size → 8 MiB affects every binary that uses this workspace, including production builds spawned by other crates.
If the increase is only needed for a handful of deeply-recursive tests, consider:# .cargo/config.toml [target.'cfg(test)'.env] RUST_MIN_STACK = "8388608"or exporting the variable only in CI scripts.
Please double-check that the larger stack is absolutely required; otherwise it silently masks runaway recursion and increases per-thread memory consumption.crates/natives/src/interface/context.rs (1)
64-68: Helper addition looks goodThe delegating method is concise and matches the existing
abs_val_sizepattern. 📌
No further issues noticed.crates/e2e-move-tests/src/tests/infinite_loop.rs (1)
27-28: Signature parameter integration acknowledgedPassing
Nonefor the optionalsignaturesargument keeps the original behaviour.
No changes requested.crates/e2e-move-tests/src/tests/table.rs (1)
33-39: Entry-function call updated correctlyThe extra
Nonealigns with the revised harness API; loop logic and error handling remain intact.crates/e2e-move-tests/src/tests/staking.rs (1)
49-55: Let’s dump the full call context for eachrun_entry_functioninvocation so we can verify argument counts and ensureNonevsSome(sig_vec)is used correctly:#!/bin/bash set -eo pipefail # For each run_entry_function call, print its file, line, and the next 20 lines rg -n 'run_entry_function' --glob '*.rs' | while IFS=: read -r file line _; do echo "=== $file:$line ===" sed -n "${line},$((line+20))p" "$file" echo donecrates/e2e-move-tests/src/tests/move_unit.rs (1)
47-47: EnsureNoneis intentional for tx-level signatures
NativeTransactionContext::new([0; 32], [0; 32], None)disables account-abstraction signature checks in unit tests.
If any unit test is meant to cover signature validation this will silently bypass it.
Double-check upcoming tests.crates/e2e-move-tests/src/tests/cosmos.rs (1)
79-80: LGTM – new argument aligns with harness API
No functional issues spotted in this hunk.crates/natives/src/lib.rs (1)
8-9: Publicly exporting new native modules – verify feature gating &make_allavailability
pub mod account_abstraction;is exported unconditionally. If these natives rely on features (e.g.crypto,stdversions) that are not always compiled, consider guarding thepub modwith the samecfgused in its implementation to prevent compilation failures for downstream crates building with a reduced feature set.crates/e2e-move-tests/src/tests/mod.rs (1)
1-14: Module list grows – ensure CI timeouts stay reasonableThree heavy e2e test suites were added. Watch the total execution time on CI; you may need to bump
RUST_TEST_THREADS, mark lengthy tests#[ignore], or split them behind a feature flag to keep pipelines green.precompile/modules/initia_stdlib/sources/block.move (1)
2-10: Document test-only setter and clarify chain-ID invariants
set_chain_id_for_testmutates a global that production code relies on.
Please:
- Add a short doc comment explaining why the function exists and under what feature flag it is compiled.
- Explicitly state in the docs for
get_chain_idthat the value is immutable in production and only overridable under#[test_only].This prevents accidental use in on-chain Move or unrestricted frameworks.
crates/natives/src/move_stdlib/mem.rs (1)
47-53: Add unit tests for the nativeswap
swapis security-sensitive (aliasing, gas accounting). A small Rust test exercising:
- identical references (should abort)
- distinct refs with different types (compile-time fail)
- gas metering
would prevent regressions.
crates/natives/src/move_stdlib/cmp.rs (1)
54-59: Hard-coding Move-side variant indices is brittle
Ordering’s variant order (Less, Equal, Greater) is duplicated here as
rawu16s. A future change on the Move side will silently desynchronise
the native implementation.Either:
- Generate / include the variant index at build-time from the compiled
.mvmetadata, or- Add an assertion in e2e tests that
cmp::compare(…, …)round-trips
the enum correctly.crates/natives/src/move_stdlib/mod.rs (1)
8-16: Module registration looks goodThe new
cmp,mem, andvectornatives are wired in correctly and
will be visible at address0x1. No further action required.crates/e2e-move-tests/src/tests/simple_authenticator.data/pack/Move.toml (1)
5-9: Relative path correctnessEnsure the relative path
../../../../../../precompile/modules/initia_stdlibresolves from the
test harness working directory; otherwise package publishing will fail at
runtime.crates/gas/src/move_stdlib.rs (1)
17-18: ```shell
#!/bin/bashSearch for any occurrence of GasParameters across the repo
echo "1) Searching for GasParameters references:"
rg -n "GasParameters" .List files/folders containing "parameters" under crates/gas/src
echo -e "\n2) Listing parameters-related files under crates/gas/src:"
fd parameters crates/gas/srcSearch for module declarations named parameters
echo -e "\n3) Searching for
mod parametersin crates/gas/src:"
rg -n "mod parameters" crates/gas/srcSearch for struct definition of GasParameters under crates/gas/src
echo -e "\n4) Searching for
struct GasParametersin crates/gas/src:"
rg -n "struct\s+GasParameters" crates/gas/src</details> <details> <summary>crates/types/src/env.rs (1)</summary> `3-3`: **`Eq`, `PartialEq`, `Hash` derives were removed – verify no callers rely on value comparison** Dropping these derives changes the public API semantics of `Env`. Anything that previously stored `Env` in a `HashMap`, used `==`, or expected deterministic hashing will now fail to compile. Please grep the codebase for `Env` equality / hashing usage to confirm nothing breaks. </details> <details> <summary>precompile/modules/move_nursery/sources/acl.move (2)</summary> `25-28`: **Looks good – idiomatic `self` usage adopted** The switch to method-style calls (`self.list.contains`, `push_back`) is correct and matches the broader refactor. No functional issues spotted. --- `31-35`: **Minor perf note on `remove`** `index_of` already scans the vector; calling `contains` beforehand would avoid the second scan, but you guard with `assert!(found, …)` so only one pass is performed. All good. </details> <details> <summary>crates/gas/src/initia_stdlib.rs (2)</summary> `77-79`: **Parameter label mismatch** Variable `transaction_context_entry_function_payload_per_byte_in_str` implies “per byte”, but the on-chain key reads `.per_abstract_memory_unit`. Pick one term for both to prevent mis-configuration downstream. --- `103-104`: **Chain-ID gas key added – good** Entry looks correct and follows the SDK-scaled pattern. </details> <details> <summary>crates/e2e-move-tests/src/tests/solana_derivable_account_abstraction.rs (1)</summary> `158-167`: **Message construction includes raw digest** You pass the digest hex string into the user-visible message but sign the *plain* UTF-8 bytes (`digest`). Confirm this is the intended domain-separation scheme; otherwise signing the hex may be safer. </details> <details> <summary>crates/e2e-move-tests/src/tests/public_key_authenticator.data/pack/sources/PublicKeyAuthenticator.move (1)</summary> `43-51`: **Race between stored key format and lookup** `permit_public_key` stores the *raw* `public_key` bytes while `authenticate` looks up `ed25519::public_key_to_bytes(&public_key)`. Make sure the two representations are identical (endianness / leading zeros) or store/look-up the same canonicalised form to avoid false negatives. </details> <details> <summary>types/bcs.go (1)</summary> `656-675`: **Breaking BCS layout change** `ChainId` and `Signatures` were prepended to `Env`. All existing consumers must be recompiled; otherwise deserialisation will fail silently. Confirm no external systems rely on the previous layout before merging. </details> <details> <summary>precompile/modules/initia_stdlib/sources/account_abstraction/auth_data.move (1)</summary> `1-17`: **LGTM!** The module structure and enum definition are well-designed with appropriate variants for regular and derivable authentication data. </details> <details> <summary>crates/natives/src/transaction_context.rs (2)</summary> `33-50`: **LGTM!** The extension of `NativeTransactionContext` with optional user transaction context is well-structured and maintains backward compatibility. --- `112-140`: **Well-implemented native function for entry function payload retrieval.** The function properly handles gas charging proportional to payload size and correctly returns an Option-wrapped result. </details> <details> <summary>precompile/modules/initia_stdlib/sources/account_abstraction/ethereum_derivable_account.move (3)</summary> `1-39`: **Excellent documentation and module structure.** The comprehensive documentation clearly explains the SIWE message format and lists tested wallet compatibility. --- `121-165`: **Correctly implements SIWE message construction.** The function properly constructs a SIWE-compliant message with the Ethereum prefix, ensuring compatibility with standard Ethereum wallets. --- `167-237`: **Robust signature verification implementation.** The signature recovery and address verification correctly follow Ethereum standards: - Validates v values (27/28) - Properly derives Ethereum address from recovered public key - Includes appropriate error handling </details> <details> <summary>crates/natives/src/move_stdlib/bcs.rs (2)</summary> `70-70`: **Good error code standardization.** Consistent use of `NFE_BCS_SERIALIZATION_FAILURE` improves error handling uniformity. Also applies to: 88-88, 131-131 --- `195-264`: **Well-designed constant size calculation.** The recursive implementation correctly identifies constant-sized types and properly tracks visited nodes for accurate gas charging. </details> <details> <summary>crates/e2e-move-tests/src/harness.rs (1)</summary> `118-119`: **Pass the signatures you already have** `publish_package` currently forces `signatures = None`. If package publication must be signed later, this helper will need another overload or a default parameter instead of suppressing the caller’s intent here. </details> <details> <summary>precompile/modules/move_stdlib/sources/string.move (1)</summary> `32-70`: **Refactor looks good** Method-receiver rewrite is consistent and compiles with the updated `vector` API. </details> <details> <summary>precompile/modules/initia_stdlib/sources/storage_slots_allocator.move (1)</summary> `223-236`: **`reuse_spare_count` arithmetic uses mismatched integer types** `reuse_spare_count` is `u32`, but `+= 1` / `-= 1` default to `u64`, causing a type error. ```diff -self.reuse_spare_count += 1; +self.reuse_spare_count = self.reuse_spare_count + 1u32;Apply the same fix for the decrement.
Likely an incorrect or invalid review comment.
precompile/modules/initia_stdlib/sources/permissioned_signer.move (1)
478-492:grant_unlimited_with_permissioned_signercircumvents master approvalThe helper is documented, but giving any module the power to escalate its own permissions to unlimited without master oversight is dangerous.
Double-check all call-sites; consider restricting this to framework-only modules viapublic(friend)visibility.precompile/modules/initia_stdlib/sources/ordered_map.move (5)
92-104: LGTM! Efficient insertion with duplicate detection.The
addfunction correctly uses binary search to find the insertion position and properly validates that the key doesn't already exist before inserting.
124-133: Excellent defensive programming with double validation.The function performs both index bounds checking and key equality validation, which provides robust error handling against edge cases.
1382-1459: Excellent test coverage including large dataset validation.The test suite is comprehensive, covering edge cases, error conditions, and performance validation with large datasets. The large dataset test helps ensure the implementation scales correctly.
526-532: Suboptimal value replacement due to missing stdlib function.The current implementation reconstructs the entire Entry instead of just replacing the value. This is less efficient but necessary until
mem::replacebecomes public.Is mem::replace available in the latest Move standard library?
269-271: Note: Pending stdlib enhancement for mem::swap.The TODO indicates waiting for
mem::swapto become public. This would improve code clarity but doesn't affect correctness.Is mem::swap available in the latest Move standard library?precompile/modules/initia_stdlib/sources/big_ordered_map.move (1)
328-335: Possible logic bug incompute_length
sizeis captured and mutated inside a closure.
If Move captures by value (current behaviour for function values), the outersizenever changes and the function will always return0.Please confirm with a quick unit test or switch to an explicit accumulator:
let mut total = 0; self.for_each_leaf_node_ref(|node| total = total + node.children.length()); totalprecompile/modules/initia_stdlib/sources/bigdecimal.move (5)
119-129:revimplementation looks correct – guard is in placeLogic and error handling (division-by-zero check) are sound, and the arithmetic preserves scale (
F * F / scaled). No further issues spotted.
171-191: Arithmetic helpers look good
addand the threeadd_by_*helpers correctly preserve scale and return new values.
Implementation is straightforward, no logic errors detected.
193-214: Early negative-result guard is appreciatedThe explicit
geassertion prevents underflow – good defensive programming. Implementation otherwise mirrors the add helpers.
216-264: Multiplication family – scale handling is correct
mulrescales by dividing byF, and the truncate / ceil helpers delegate appropriately. No arithmetic issues spotted.
266-293: Division family – zero checks are in placeZero-denominator assertions avoid traps; scaling formula (
self * F / other) is correct. Implementation LGTM.
| spec canonical { | ||
| pragma opaque = true; | ||
| let shl_res = category << 16; | ||
| ensures [concrete] result == shl_res + reason; | ||
| aborts_if [abstract] false; | ||
| ensures [abstract] result == category; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Spec is sound but missing range guarantees for category / reason
canonical promises the 1-byte / 2-byte layout in the doc-comment, yet neither the function body nor the spec constrains
category < 1 << 8 and reason < 1 << 16.
Without those u64 may overflow or collide with non-canonical codes.
Consider tightening the spec (and optionally adding assert!s in code):
requires category < 0x100 && reason < 0x10000;
ensures result == (category << 16) + reason;🤖 Prompt for AI Agents
In precompile/modules/move_stdlib/sources/error.move around lines 68 to 74, the
spec for the canonical function lacks constraints ensuring category is less than
256 and reason is less than 65536, which can cause overflow or collisions. Fix
this by adding requires clauses to the spec that enforce category < 0x100 and
reason < 0x10000, and optionally add assert statements in the function body to
enforce these bounds at runtime.
| module 0xcafe::simple_authenticator { | ||
| use std::auth_data::{Self, AbstractionAuthData}; | ||
|
|
||
| public fun authenticate( | ||
| account: signer, auth_data: AbstractionAuthData | ||
| ): signer { | ||
| let authenticator = *auth_data::authenticator(&auth_data); | ||
| assert!(authenticator == b"hello world", 1); | ||
| account | ||
| } | ||
| } |
There was a problem hiding this comment.
auth_data reference out of scope – compilation will fail
The module imports std::auth_data::{Self, AbstractionAuthData} but then calls auth_data::authenticator.
auth_data is not in scope; the intended reference is the alias Self (or the fully-qualified path).
Move will raise an “unbound module” error here.
- let authenticator = *auth_data::authenticator(&auth_data);
+ // `Self` is the alias for `std::auth_data`
+ let authenticator = *Self::authenticator(&auth_data);Optional: to avoid the slightly confusing Self alias, rename the module on import:
-use std::auth_data::{Self, AbstractionAuthData};
+use std::auth_data as AuthData;
+use AuthData::AbstractionAuthData;
...
- let authenticator = *auth_data::authenticator(&auth_data);
+ let authenticator = *AuthData::authenticator(&auth_data);Without this fix the package containing this Move module cannot be published or compiled.
📝 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.
| module 0xcafe::simple_authenticator { | |
| use std::auth_data::{Self, AbstractionAuthData}; | |
| public fun authenticate( | |
| account: signer, auth_data: AbstractionAuthData | |
| ): signer { | |
| let authenticator = *auth_data::authenticator(&auth_data); | |
| assert!(authenticator == b"hello world", 1); | |
| account | |
| } | |
| } | |
| module 0xcafe::simple_authenticator { | |
| use std::auth_data::{Self, AbstractionAuthData}; | |
| public fun authenticate( | |
| account: signer, auth_data: AbstractionAuthData | |
| ): signer { | |
| // `Self` is the alias for `std::auth_data` | |
| let authenticator = *Self::authenticator(&auth_data); | |
| assert!(authenticator == b"hello world", 1); | |
| account | |
| } | |
| } |
🤖 Prompt for AI Agents
In
crates/e2e-move-tests/src/tests/simple_authenticator.data/pack/sources/SimpleAuthenticator.move
lines 1 to 11, the code imports std::auth_data::{Self, AbstractionAuthData} but
incorrectly references auth_data::authenticator, which is out of scope and
causes a compilation error. Fix this by replacing auth_data::authenticator with
Self::authenticator or by renaming the imported Self alias to a clearer name and
using that instead. This will ensure the module reference is valid and the code
compiles successfully.
| #[allow(clippy::result_large_err)] | ||
| fn native_swap( | ||
| context: &mut SafeNativeContext, | ||
| _ty_args: Vec<Type>, | ||
| mut args: VecDeque<Value>, | ||
| ) -> SafeNativeResult<SmallVec<[Value; 1]>> { | ||
| let gas_params = &context.native_gas_params.move_stdlib; | ||
|
|
||
| debug_assert!(args.len() == 2); | ||
|
|
||
| context.charge(gas_params.mem_swap_base)?; | ||
|
|
||
| let left = safely_pop_arg!(args, Reference); | ||
| let right = safely_pop_arg!(args, Reference); | ||
|
|
||
| left.swap_values(right)?; | ||
|
|
||
| Ok(smallvec![]) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fail fast instead of relying on debug_assert!(args.len() == 2)
debug_assert! is stripped in release builds. If callers pass the wrong arity the native will panic at pop_arg! with an opaque error.
Prefer explicit runtime validation so mis-use surfaces as a well-defined Move abort:
- debug_assert!(args.len() == 2);
+ if args.len() != 2 {
+ // E0 = invalid argument count (define a domain-specific error code)
+ return context.abort("E0_invalid_argument_count");
+ }(Or use an existing errors::NArity pattern if one exists in your native helpers.)
📝 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.
| #[allow(clippy::result_large_err)] | |
| fn native_swap( | |
| context: &mut SafeNativeContext, | |
| _ty_args: Vec<Type>, | |
| mut args: VecDeque<Value>, | |
| ) -> SafeNativeResult<SmallVec<[Value; 1]>> { | |
| let gas_params = &context.native_gas_params.move_stdlib; | |
| debug_assert!(args.len() == 2); | |
| context.charge(gas_params.mem_swap_base)?; | |
| let left = safely_pop_arg!(args, Reference); | |
| let right = safely_pop_arg!(args, Reference); | |
| left.swap_values(right)?; | |
| Ok(smallvec![]) | |
| } | |
| #[allow(clippy::result_large_err)] | |
| fn native_swap( | |
| context: &mut SafeNativeContext, | |
| _ty_args: Vec<Type>, | |
| mut args: VecDeque<Value>, | |
| ) -> SafeNativeResult<SmallVec<[Value; 1]>> { | |
| let gas_params = &context.native_gas_params.move_stdlib; | |
| - debug_assert!(args.len() == 2); | |
| + if args.len() != 2 { | |
| + // E0 = invalid argument count (define a domain-specific error code) | |
| + return context.abort("E0_invalid_argument_count"); | |
| + } | |
| context.charge(gas_params.mem_swap_base)?; | |
| let left = safely_pop_arg!(args, Reference); | |
| let right = safely_pop_arg!(args, Reference); | |
| left.swap_values(right)?; | |
| Ok(smallvec![]) | |
| } |
🤖 Prompt for AI Agents
In crates/natives/src/move_stdlib/mem.rs around lines 24 to 42, replace the
debug_assert checking args length with explicit runtime validation that returns
a clear Move abort error if the argument count is incorrect. This prevents
panics in release builds by validating args.len() == 2 before popping arguments,
and returning a well-defined error such as errors::NArity if the check fails.
| fn native_compare( | ||
| context: &mut SafeNativeContext, | ||
| _ty_args: Vec<Type>, | ||
| args: VecDeque<Value>, | ||
| ) -> SafeNativeResult<SmallVec<[Value; 1]>> { | ||
| debug_assert!(args.len() == 2); | ||
| let gas_params = &context.native_gas_params.move_stdlib; | ||
|
|
||
| if args.len() != 2 { | ||
| return Err(SafeNativeError::InvariantViolation(PartialVMError::new( | ||
| StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR, | ||
| ))); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing type-argument arity check
compare<T> is generic, so VM calls will always supply exactly one type
argument.
Failing to assert this silently accepts malformed invocations and masks
potential VM ↔︎ native mismatches.
fn native_compare(
context: &mut SafeNativeContext,
_ty_args: Vec<Type>,
args: VecDeque<Value>,
) -> SafeNativeResult<SmallVec<[Value; 1]>> {
+ // `compare<T>` must receive exactly one type argument.
+ if _ty_args.len() != 1 {
+ return Err(SafeNativeError::InvariantViolation(
+ PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_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.
| fn native_compare( | |
| context: &mut SafeNativeContext, | |
| _ty_args: Vec<Type>, | |
| args: VecDeque<Value>, | |
| ) -> SafeNativeResult<SmallVec<[Value; 1]>> { | |
| debug_assert!(args.len() == 2); | |
| let gas_params = &context.native_gas_params.move_stdlib; | |
| if args.len() != 2 { | |
| return Err(SafeNativeError::InvariantViolation(PartialVMError::new( | |
| StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR, | |
| ))); | |
| } | |
| fn native_compare( | |
| context: &mut SafeNativeContext, | |
| _ty_args: Vec<Type>, | |
| args: VecDeque<Value>, | |
| ) -> SafeNativeResult<SmallVec<[Value; 1]>> { | |
| // `compare<T>` must receive exactly one type argument. | |
| if _ty_args.len() != 1 { | |
| return Err(SafeNativeError::InvariantViolation( | |
| PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR), | |
| )); | |
| } | |
| debug_assert!(args.len() == 2); | |
| let gas_params = &context.native_gas_params.move_stdlib; | |
| if args.len() != 2 { | |
| return Err(SafeNativeError::InvariantViolation(PartialVMError::new( | |
| StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR, | |
| ))); | |
| } |
🤖 Prompt for AI Agents
In crates/natives/src/move_stdlib/cmp.rs around lines 34 to 46, add a check to
assert that the length of _ty_args is exactly one before proceeding. If the
length is not one, return an appropriate error indicating the type argument
arity violation. This ensures that the generic function compare<T> is called
with exactly one type argument, preventing silent acceptance of malformed
invocations.
| public(friend) fun base16_utf8_to_vec_u8(str: vector<u8>): vector<u8> { | ||
| let result = vector::empty<u8>(); | ||
| let i = 0; | ||
| while (i < vector::length(&str)) { | ||
| let c1 = vector::borrow(&str, i); | ||
| let c2 = vector::borrow(&str, i + 1); | ||
| let byte = hex_char_to_u8(*c1) << 4 | hex_char_to_u8(*c2); | ||
| vector::push_back(&mut result, byte); | ||
| i = i + 2; | ||
| }; | ||
| result | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Compilation blocker: i must be mutable and input length must be even
i is reassigned inside the while loop but is declared immutable.
Moreover, borrowing i + 1 without checking for an odd-length input can
panic. Fix both issues:
- let i = 0;
- while (i < vector::length(&str)) {
+ let mut i = 0;
+ assert!(vector::length(&str) % 2 == 0, 0); // abort code 0 = invalid length
+ while (i < vector::length(&str)) {📝 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.
| public(friend) fun base16_utf8_to_vec_u8(str: vector<u8>): vector<u8> { | |
| let result = vector::empty<u8>(); | |
| let i = 0; | |
| while (i < vector::length(&str)) { | |
| let c1 = vector::borrow(&str, i); | |
| let c2 = vector::borrow(&str, i + 1); | |
| let byte = hex_char_to_u8(*c1) << 4 | hex_char_to_u8(*c2); | |
| vector::push_back(&mut result, byte); | |
| i = i + 2; | |
| }; | |
| result | |
| } | |
| public(friend) fun base16_utf8_to_vec_u8(str: vector<u8>): vector<u8> { | |
| let result = vector::empty<u8>(); | |
| let mut i = 0; | |
| assert!(vector::length(&str) % 2 == 0, 0); // abort code 0 = invalid length | |
| while (i < vector::length(&str)) { | |
| let c1 = vector::borrow(&str, i); | |
| let c2 = vector::borrow(&str, i + 1); | |
| let byte = hex_char_to_u8(*c1) << 4 | hex_char_to_u8(*c2); | |
| vector::push_back(&mut result, byte); | |
| i = i + 2; | |
| }; | |
| result | |
| } |
🤖 Prompt for AI Agents
In precompile/modules/initia_stdlib/sources/account_abstraction/base16.move
lines 21 to 32, the variable i is declared immutable but reassigned inside the
while loop, causing a compilation error. Also, the code assumes the input vector
length is even without validation, which can cause a panic when accessing i + 1.
Fix this by declaring i as mutable and adding a check to ensure the input length
is even before the loop to prevent out-of-bounds access.
| account: signer, func_info: FunctionInfo, signing_data: AbstractionAuthData | ||
| ): signer acquires DispatchableAuthenticator, DerivableDispatchableAuthenticator { | ||
| let master_signer_addr = signer::address_of(&account); | ||
|
|
||
| if (signing_data.is_derivable()) { | ||
| assert!( | ||
| master_signer_addr | ||
| == derive_account_address( | ||
| func_info, signing_data.derivable_abstract_public_key() | ||
| ), | ||
| error::invalid_state(EINCONSISTENT_SIGNER_ADDRESS) | ||
| ); | ||
|
|
||
| let func_infos = dispatchable_derivable_authenticator_internal(); | ||
| assert!( | ||
| func_infos.contains(&func_info), | ||
| error::not_found(EFUNCTION_INFO_EXISTENCE) | ||
| ); | ||
| } else { | ||
| let func_infos = dispatchable_authenticator_internal(master_signer_addr); | ||
| assert!( | ||
| func_infos.contains(&func_info), | ||
| error::not_found(EFUNCTION_INFO_EXISTENCE) | ||
| ); | ||
| }; | ||
|
|
||
| function_info::load_module_from_function(&func_info); | ||
| let returned_signer = dispatchable_authenticate( | ||
| account, signing_data, &func_info | ||
| ); | ||
| // Returned signer MUST represent the same account address. Otherwise, it may break the invariant of Aptos blockchain! | ||
| assert!( | ||
| master_signer_addr == signer::address_of(&returned_signer), | ||
| error::invalid_state(EINCONSISTENT_SIGNER_ADDRESS) | ||
| ); | ||
| returned_signer | ||
| } |
There was a problem hiding this comment.
authenticate leaks resource if user function aborts
After the user-supplied dispatchable_authenticate is invoked, if it aborts the local account signer has already been consumed (moved), and the original signer resource is lost, violating safety.
Consider wrapping the call in let result = abort::catch_unwind(|| { … }) once such helper exists, or splitting logic:
- Pass a reference to
accountto the user function. - Only
movethe signer once the returned signer passes address equality check.
This preserves linearity even on abort paths.
🤖 Prompt for AI Agents
In
precompile/modules/initia_stdlib/sources/account_abstraction/account_abstraction.move
around lines 317 to 353, the function authenticate moves the account signer
before calling dispatchable_authenticate, which can cause resource leaks if
dispatchable_authenticate aborts. To fix this, modify the code to pass a
reference to the account signer to dispatchable_authenticate instead of moving
it, and only move the signer after verifying the returned signer’s address
matches. This ensures the original signer resource is preserved if the user
function aborts, maintaining resource safety.
| fun increase_capacity(perm: &mut StoredPermission, threshold: u256) { | ||
| match(perm) { | ||
| StoredPermission::Capacity(current_capacity) => { | ||
| *current_capacity = *current_capacity + threshold; | ||
| } | ||
| StoredPermission::Unlimited => () | ||
| } | ||
| } |
There was a problem hiding this comment.
increase_capacity can silently overflow u256
*current_capacity = *current_capacity + threshold; has no upper-bound check.
Adding 1 to U256_MAX will wrap to 0, effectively granting unlimited permission.
Add a guard:
assert!(
*current_capacity <= U256_MAX - threshold,
error::invalid_state(EOVERFLOW) // define new code
);
*current_capacity = *current_capacity + threshold;Same issue exists in merge when two finite capacities are combined.
🤖 Prompt for AI Agents
In precompile/modules/initia_stdlib/sources/permissioned_signer.move around
lines 354 to 361, the increase_capacity function adds threshold to
current_capacity without checking for overflow, which can wrap around and
incorrectly grant unlimited permission. Fix this by adding an assertion before
the addition to ensure current_capacity plus threshold does not exceed U256_MAX,
using an appropriate error code like error::invalid_state(EOVERFLOW). Also,
apply the same overflow check in the merge function when combining two finite
capacities.
| public inline fun for_each<Element>( | ||
| self: Option<Element>, f: |Element| | ||
| ) { | ||
| if (self.is_some()) { | ||
| f(self.destroy_some()) | ||
| } else { | ||
| destroy_none(o) | ||
| self.destroy_none() | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
for_each destroys self after potential abort in f
If the callback f aborts, the option resource is leaked because neither branch calls destroy_some/none.
Safer pattern:
match self {
Option { vec } if vec.is_empty() => { vec.destroy_empty(); },
Option { vec } => {
let val = vec.pop_back();
vec.destroy_empty();
f(val); // run after cleanup
}
}Decide if stronger guarantees are needed; otherwise add a note in the doc comment.
🤖 Prompt for AI Agents
In precompile/modules/move_stdlib/sources/option.move around lines 297 to 305,
the for_each function currently destroys the Option resource after calling the
callback f, which can cause resource leaks if f aborts. To fix this, refactor
for_each to first extract and destroy the Option resource before invoking f,
ensuring no resource is leaked even if f aborts. Follow the safer pattern by
matching on self, destroying the inner resource before calling f, and consider
adding a note in the doc comment about the guarantees provided.
| public fun destroy_empty<K, V>(self: OrderedMap<K, V>) { | ||
| let OrderedMap::SortedVectorMap { entries } = self; | ||
| // assert!(entries.is_empty(), E_NOT_EMPTY); | ||
| entries.destroy_empty(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Uncomment the assertion to validate empty state.
The commented assertion should be enabled to catch programming errors where destroy_empty is called on non-empty maps.
public fun destroy_empty<K, V>(self: OrderedMap<K, V>) {
let OrderedMap::SortedVectorMap { entries } = self;
- // assert!(entries.is_empty(), E_NOT_EMPTY);
+ assert!(entries.is_empty(), error::invalid_state(ENOT_EMPTY));
entries.destroy_empty();
}Note: You'll need to add the ENOT_EMPTY error constant.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In precompile/modules/initia_stdlib/sources/ordered_map.move around lines 570 to
574, uncomment the assertion that checks if entries is empty before calling
destroy_empty to ensure the function is only used on empty maps. Additionally,
define the ENOT_EMPTY error constant used in the assertion to provide a clear
error message when the assertion fails.
| assert!( | ||
| leaf_max_degree_from_max >= (INNER_MIN_DEGREE as u64), | ||
| error::invalid_argument(EINVALID_CONFIG_PARAMETER) |
There was a problem hiding this comment.
Leaf-degree check uses the wrong constant
leaf_max_degree_from_max is validated against INNER_MIN_DEGREE, not LEAF_MIN_DEGREE.
This wrongly aborts when leaf_max_degree_from_max == 3, even though 3 is the documented minimum for leaf nodes.
-assert!(
- leaf_max_degree_from_max >= (INNER_MIN_DEGREE as u64),
+assert!(
+ leaf_max_degree_from_max >= (LEAF_MIN_DEGREE as u64),📝 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.
| assert!( | |
| leaf_max_degree_from_max >= (INNER_MIN_DEGREE as u64), | |
| error::invalid_argument(EINVALID_CONFIG_PARAMETER) | |
| assert!( | |
| leaf_max_degree_from_max >= (LEAF_MIN_DEGREE as u64), | |
| error::invalid_argument(EINVALID_CONFIG_PARAMETER) |
🤖 Prompt for AI Agents
In precompile/modules/initia_stdlib/sources/big_ordered_map.move around lines
213 to 215, the assertion incorrectly compares leaf_max_degree_from_max against
INNER_MIN_DEGREE instead of LEAF_MIN_DEGREE. To fix this, replace
INNER_MIN_DEGREE with LEAF_MIN_DEGREE in the assertion to correctly validate the
leaf node degree according to the documented minimum.
* split dispatchable authenticate from execute entry function & provide its api * cargo fmt & clippy * add execute authenticate go api * add missing minitia_stdlib update && add missing dot receiver function call style patch && improve error handling * deserialize returned value to signer move value and return address * fix account address serialize * using base64 when serializing bytes fields of abstraction auth data * clippy & fmt --------- Co-authored-by: beer-1 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (14)
crates/natives/src/move_stdlib/vector.rs (2)
56-57: Guard against missing type arguments
ty_args[0]is accessed without first assertingty_args.len() == 1. A malformed call (e.g. via fuzzing) with zero type-args will panic inside the native and crash the VM.+if unlikely!(ty_args.len() != 1) { + return Err(SafeNativeError::InvariantViolation); +} let to_len = to.length_as_usize(&ty_args[0])?; let from_len = from.length_as_usize(&ty_args[0])?;
84-92: Overlapping self-moves are undefined
VectorRef::move_rangeis invoked even whenfromandtomight alias the same underlying vector. For overlapping regions this can lead to element duplication or loss. Add an explicit check and abort iffrom.equals(&to)(or make the Move spec disallow aliasing).precompile/modules/initia_stdlib/sources/storage_slots_allocator.move (1)
239-246: Fix unqualified call totable_with_length::newcrates/natives/src/permissioned_signer.rs (1)
82-89: Potential argument order inversion remains unresolved.This appears to be the same issue flagged in the previous review. Since
VecDequeis popped from the back bysafely_pop_arg!, the arguments may be inverted compared to the Move function signature.If the Move signature is
fun signer_from_permissioned(master_addr: address, permission_addr: address), swap the assignments:- 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);precompile/modules/initia_stdlib/sources/account_abstraction/solana_derivable_account.move (4)
106-106: Immutable vars mutated – compile error
i,j, andcarryare mutated inside the loops but declared withoutmut.
Addmutto each binding.- let i = 0; + let mut i = 0;And in the inner loop:
- let j = 0; - let carry = (char_index as u16); + let mut j = 0; + let mut carry = (char_index as u16);Also applies to: 112-113
103-103: Extra leading zero insertedStarting
byteswithvector[0u8]always injects a superfluous 0 byte. Use an empty vector instead.- let bytes = vector[0u8]; + let mut bytes = vector::empty<u8>();
167-167: Unqualifieded25519reference
ed25519::verifyis used but onlySelf(alias forstd::ed25519) is imported. Either importverifyexplicitly or callstd::ed25519::verify.- ed25519::verify(message, &public_key, &signature), EINVALID_SIGNATURE + std::ed25519::verify(message, &public_key, &signature), EINVALID_SIGNATURE
134-134: Variableiredeclared withoutmutThe variable
ion line 134 is mutated in the loop but declared withoutmut.- let i = 0; + let mut i = 0;precompile/modules/minitia_stdlib/sources/account_abstraction/solana_derivable_account.move (3)
106-106: Immutable vars mutated – compile error
i,j, andcarryare mutated inside the loops but declared withoutmut.
Addmutto each binding.- let i = 0; + let mut i = 0;And in the inner loop:
- let j = 0; - let carry = (char_index as u16); + let mut j = 0; + let mut carry = (char_index as u16);Also applies to: 112-113
103-103: Extra leading zero insertedStarting
byteswithvector[0u8]always injects a superfluous 0 byte. Use an empty vector instead.- let bytes = vector[0u8]; + let mut bytes = vector::empty<u8>();
167-167: Unqualifieded25519reference
ed25519::verifyis used but onlySelf(alias forstd::ed25519) is imported. Either importverifyexplicitly or callstd::ed25519::verify.- ed25519::verify(message, &public_key, &signature), EINVALID_SIGNATURE + std::ed25519::verify(message, &public_key, &signature), EINVALID_SIGNATUREcrates/e2e-move-tests/src/tests/ethereum_derivable_account_abstraction.rs (1)
218-218: Digest mismatch inAbstractionAuthData::DerivableV1
signing_message_digestis set to the raw stringb"hello world"while the signature is produced over the Keccak-256 hash stored insigning_message_digestvariable.
Passing inconsistent data will cause authenticators that actually validate the digest to fail.- signing_message_digest: digest.as_bytes().to_vec(), + signing_message_digest: signing_message_digest.clone(),precompile/modules/initia_stdlib/sources/permissioned_signer.move (2)
354-361:increase_capacitycan silently overflowu256
*current_capacity = *current_capacity + threshold;has no upper-bound check.
Adding 1 toU256_MAXwill wrap to 0, effectively granting unlimited permission.Add a guard:
assert!( *current_capacity <= U256_MAX - threshold, error::invalid_state(EOVERFLOW) // define new code ); *current_capacity = *current_capacity + threshold;Same issue exists in
mergewhen two finite capacities are combined.
363-376:mergefunction also has u256 overflow issueWhen merging two finite capacities, the addition at line 369 can overflow without detection.
Apply overflow check before addition:
StoredPermission::Capacity(current_capacity) => { + assert!( + *current_capacity <= U256_MAX - new_capacity, + error::invalid_state(EOVERFLOW) + ); *current_capacity = *current_capacity + new_capacity; }
🧹 Nitpick comments (21)
crates/natives/src/move_stdlib/vector.rs (1)
72-82: Review gas calculation complexity for potential overflowThe gas calculation involves multiple arithmetic operations that could potentially overflow, especially the complex chain of
checked_addoperations. While the code does handle overflow by returning an error, the logic is quite intricate.Consider simplifying the calculation or adding comments to clarify the reasoning:
// We are moving all elements in the range, all elements after range, and all elements after insertion point. // We are counting "length" of moving block twice, as it both gets moved out and moved in. // From calibration testing, this seems to be a reasonable approximation of the cost of the operation. +// Total cost: (from_len - removal_position) + (to_len - insert_position) + length context.charge(libmovevm/src/interface.rs (1)
210-210: Fix copy-paste error in function comment.The comment incorrectly describes this as a view function execution when it's actually for authentication.
-// exported function to execute #[view] function +// exported function to execute authenticationprecompile/modules/minitia_stdlib/sources/account_abstraction/ethereum_derivable_account.move (2)
41-41: Remove unused error constant.The
EMISSING_ENTRY_FUNCTION_PAYLOADerror constant is defined but never used in this module.- /// Entry function payload is missing. - const EMISSING_ENTRY_FUNCTION_PAYLOAD: u64 = 2;
216-216: Fix variable naming convention.The variable name
kexHashdoesn't follow Move naming conventions. It should use snake_case.- let kexHash = aptos_hash::keccak256(public_key_without_prefix); + let key_hash = aptos_hash::keccak256(public_key_without_prefix);Also update line 218:
- let recovered_addr = vector::slice(&kexHash, 12, 32); + let recovered_addr = vector::slice(&key_hash, 12, 32);precompile/modules/initia_stdlib/sources/account_abstraction/ethereum_derivable_account.move (2)
41-41: Remove unused error constant.The
EMISSING_ENTRY_FUNCTION_PAYLOADerror constant is defined but never used in this module.- /// Entry function payload is missing. - const EMISSING_ENTRY_FUNCTION_PAYLOAD: u64 = 2;
216-216: Fix variable naming convention.The variable name
kexHashdoesn't follow Move naming conventions. It should use snake_case.- let kexHash = aptos_hash::keccak256(public_key_without_prefix); + let key_hash = aptos_hash::keccak256(public_key_without_prefix);Also update line 218:
- let recovered_addr = vector::slice(&kexHash, 12, 32); + let recovered_addr = vector::slice(&key_hash, 12, 32);precompile/modules/initia_stdlib/sources/account_abstraction/solana_derivable_account.move (2)
30-31: Remove unused error constant.The
EMISSING_ENTRY_FUNCTION_PAYLOADerror constant is defined but never used in this module.- /// Entry function payload is missing. - const EMISSING_ENTRY_FUNCTION_PAYLOAD: u64 = 3;
42-42: Remove unused constant.The
HEX_ALPHABETconstant is defined but never used in this module.- const HEX_ALPHABET: vector<u8> = b"0123456789abcdef";precompile/modules/minitia_stdlib/sources/account_abstraction/solana_derivable_account.move (2)
30-31: Remove unused error constant.The
EMISSING_ENTRY_FUNCTION_PAYLOADerror constant is defined but never used in this module.- /// Entry function payload is missing. - const EMISSING_ENTRY_FUNCTION_PAYLOAD: u64 = 3;
42-42: Remove unused constant.The
HEX_ALPHABETconstant is defined but never used in this module.- const HEX_ALPHABET: vector<u8> = b"0123456789abcdef";crates/e2e-move-tests/src/tests/ethereum_derivable_account_abstraction.rs (1)
27-27: Simplify byte conversion.Converting
to_string().as_bytes().to_vec()is redundant. You can directly convert the string to bytes.- let mut msg_len_bytes = msg_len.to_string().as_bytes().to_vec(); // vector<u8> + let msg_len_bytes = msg_len.to_string().into_bytes();precompile/modules/minitia_stdlib/sources/bcs_stream.move (2)
54-54: Fix formatting: Add space before parenthesis.Minor formatting issue in the bit operation.
- res |=(val << shift); + res |= (val << shift);
242-242: Use underscore for unused loop variable.The loop variable
iis declared but never used in the loop body.- for (i in 0..len) { + for (_ in 0..len) {precompile/modules/minitia_stdlib/sources/permissioned_signer.move (2)
177-183: Consider using a more efficient data structure for active handles lookup.The
index_ofoperation on line 179 has O(n) complexity. For accounts with many active permission handles, this could become a performance bottleneck.Consider using an ordered set or map structure instead of a vector for
active_handlesto achieve O(log n) lookup complexity:struct GrantedPermissionHandles has key { - /// Each address refers to a `permissions_storage_addr` that stores the `PermissionStorage`. - active_handles: vector<address> + /// Set of addresses that refer to `permissions_storage_addr` storing the `PermissionStorage`. + active_handles: OrderedSet<address> }
215-222: Document the BigOrderedMap configuration parameters.The magic numbers
40, 35, falseon line 219 should be documented or defined as named constants to improve code maintainability.+ // BigOrderedMap config: max_leaf_size=40, max_internal_size=35, enable_pagination=false 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 ) } );precompile/modules/minitia_stdlib/sources/big_ordered_map.move (6)
1-1: Fix grammar: "an big" → "a big"-/// This module provides an implementation for an big ordered map. +/// This module provides an implementation for a big ordered map.
10-11: Fix typo: "gast" → "gas"-/// BPlusTreeMap is chosen since the biggest (performance and gast) -/// costs are reading resources, and it: +/// BPlusTreeMap is chosen since the biggest (performance and gas) +/// costs are reading resources, and it:
1037-1039: Use field shorthand syntaxfun new_leaf_child<V: store>(value: V): Child<V> { - Child::Leaf { value: value } + Child::Leaf { value } }
1041-1045: Use field shorthand syntax for all fieldsfun new_iter<K>( node_index: u64, child_iter: ordered_map::IteratorPtr, key: K ): IteratorPtr<K> { - IteratorPtr::Some { node_index: node_index, child_iter: child_iter, key: key } + IteratorPtr::Some { node_index, child_iter, key } }
2507-2507: Remove redundant import inside functionThe
vectormodule is already available through the module-level imports.- use std::vector; - let map = new_with_config(inner_max_degree, leaf_max_degree, reuse_slots);
1372-1375: Consider extracting complex condition for readabilityThe condition for checking if we're updating the largest child could be clearer.
+ let updated_key = children.new_end_iter().iter_prev(children).iter_borrow_key(children); // If we were not updating the largest child, we don't need to continue. - if (children.new_end_iter().iter_prev(children).iter_borrow_key(children) - != &new_key) { return }; + if (updated_key != &new_key) { return };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
api/bindings.h(1 hunks)api/vm.go(1 hunks)crates/e2e-move-tests/src/harness.rs(7 hunks)crates/e2e-move-tests/src/tests/account_abstraction.rs(1 hunks)crates/e2e-move-tests/src/tests/ethereum_derivable_account_abstraction.rs(1 hunks)crates/e2e-move-tests/src/tests/solana_derivable_account_abstraction.rs(1 hunks)crates/e2e-move-tests/src/tests/std_coin.rs(2 hunks)crates/natives/src/account_abstraction.rs(1 hunks)crates/natives/src/move_stdlib/cmp.rs(1 hunks)crates/natives/src/move_stdlib/mem.rs(1 hunks)crates/natives/src/move_stdlib/vector.rs(1 hunks)crates/natives/src/permissioned_signer.rs(1 hunks)crates/types/Cargo.toml(2 hunks)crates/types/src/authenticator.rs(1 hunks)crates/types/src/env.rs(3 hunks)crates/types/src/message.rs(1 hunks)crates/vm/src/initia_vm.rs(10 hunks)lib.go(1 hunks)libmovevm/bindings.h(1 hunks)libmovevm/src/interface.rs(2 hunks)libmovevm/src/vm.rs(2 hunks)precompile/modules/initia_stdlib/sources/account_abstraction/ethereum_derivable_account.move(1 hunks)precompile/modules/initia_stdlib/sources/account_abstraction/solana_derivable_account.move(1 hunks)precompile/modules/initia_stdlib/sources/bigdecimal.move(6 hunks)precompile/modules/initia_stdlib/sources/biguint.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(4 hunks)precompile/modules/minitia_stdlib/sources/account.move(1 hunks)precompile/modules/minitia_stdlib/sources/account_abstraction/account_abstraction.move(1 hunks)precompile/modules/minitia_stdlib/sources/account_abstraction/auth_data.move(1 hunks)precompile/modules/minitia_stdlib/sources/account_abstraction/base16.move(1 hunks)precompile/modules/minitia_stdlib/sources/account_abstraction/ethereum_derivable_account.move(1 hunks)precompile/modules/minitia_stdlib/sources/account_abstraction/solana_derivable_account.move(1 hunks)precompile/modules/minitia_stdlib/sources/bcs_stream.move(1 hunks)precompile/modules/minitia_stdlib/sources/big_ordered_map.move(1 hunks)precompile/modules/minitia_stdlib/sources/bigdecimal.move(6 hunks)precompile/modules/minitia_stdlib/sources/biguint.move(1 hunks)precompile/modules/minitia_stdlib/sources/block.move(1 hunks)precompile/modules/minitia_stdlib/sources/function_info.move(1 hunks)precompile/modules/minitia_stdlib/sources/ordered_map.move(1 hunks)precompile/modules/minitia_stdlib/sources/permissioned_signer.move(1 hunks)precompile/modules/minitia_stdlib/sources/simple_map.move(11 hunks)precompile/modules/minitia_stdlib/sources/storage_slots_allocator.move(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- precompile/modules/minitia_stdlib/sources/account.move
- precompile/modules/minitia_stdlib/sources/function_info.move
- crates/types/src/message.rs
- precompile/modules/minitia_stdlib/sources/biguint.move
🚧 Files skipped from review as they are similar to previous changes (11)
- crates/types/Cargo.toml
- crates/e2e-move-tests/src/tests/std_coin.rs
- crates/natives/src/move_stdlib/cmp.rs
- crates/natives/src/move_stdlib/mem.rs
- crates/natives/src/account_abstraction.rs
- crates/e2e-move-tests/src/tests/solana_derivable_account_abstraction.rs
- precompile/modules/initia_stdlib/sources/simple_map.move
- crates/types/src/env.rs
- crates/e2e-move-tests/src/tests/account_abstraction.rs
- precompile/modules/initia_stdlib/sources/bigdecimal.move
- precompile/modules/initia_stdlib/sources/table.move
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (44)
crates/natives/src/move_stdlib/vector.rs (2)
59-67: LGTM: Comprehensive bounds checkingThe bounds validation correctly handles all edge cases including integer overflow in the range calculation and ensures both removal and insertion positions are valid.
99-105: LGTM: Proper native function registrationThe module registration follows the standard pattern and correctly exports the
move_rangenative function.precompile/modules/initia_stdlib/sources/biguint.move (6)
84-87: LGTM! Consistent method calling style.The subtraction, multiplication, and division convenience methods correctly use the instance method calling style (
self.sub(num2),self.mul(num2),self.div(num2)) which is consistent with the refactor objectives.Also applies to: 89-92, 94-97, 104-107, 109-112, 114-117, 124-127, 129-132, 134-137
31-33: LGTM! Conversion methods properly refactored.The conversion methods (
to_u64,to_u128,to_u256,to_le_bytes) have been correctly updated to useselfparameter and accessself.bytesinstead of the previous parameter reference.Also applies to: 40-42, 49-51, 53-55
59-62: LGTM! Core arithmetic methods properly refactored.The primary arithmetic methods (
add,sub,mul,div) have been correctly converted to instance methods usingselfparameter while maintaining the same internal logic and native function calls.Also applies to: 79-82, 99-102, 119-122
141-143: LGTM! Comparison methods properly refactored.The comparison methods (
eq,lt,le,gt,ge) have been correctly updated to useselfparameter and accessself.bytesin the internal function calls.Also applies to: 145-147, 149-151, 153-155, 157-159
161-167: LGTM! Utility methods use consistent instance method calls.The
is_zeroandis_onemethods correctly use the instance method calling style withself.eq(), which is consistent with the refactor objectives.
196-372: Verify test compatibility with the new API.The test functions still use the old static function call style (e.g.,
add(num1, num2),to_u64(num3)), but should now use the instance method style (e.g.,num1.add(num2),num3.to_u64()). Since these functions aren't marked as changed, verify that the tests have been updated elsewhere or will compile correctly.Run the following script to check if there are any compilation issues with the test functions:
#!/bin/bash # Description: Check for potential compilation issues with BigUint tests # Search for old-style function calls in test functions that may need updating rg -A 3 -B 1 "add\(|sub\(|mul\(|div\(|to_u64\(|to_u128\(|to_u256\(|eq\(|lt\(|le\(|gt\(|ge\(" precompile/modules/initia_stdlib/sources/biguint.moveprecompile/modules/initia_stdlib/sources/storage_slots_allocator.move (2)
56-68: Well-designed safety handles for slot management.The
ReservedSlotandStoredSlothandles are well-designed to prevent misuse:
- Not copy/drop-able to ensure proper lifecycle management
ReservedSlotcannot be stored, enforcing single-transaction usageStoredSlotis store-able for persistent ownership
117-140: Robust cleanup implementation indestroy_empty.The implementation properly:
- Drains the reuse queue completely
- Verifies internal invariants with assertions
- Handles both empty and non-empty slots table cases
- Uses pattern matching for safe destruction
precompile/modules/minitia_stdlib/sources/block.move (2)
2-2: Correct import addition for String type.The import is properly added to support the new chain ID functions.
4-9: Well-designed chain ID functions.The functions follow good practices:
get_chain_id()is correctly marked as#[view]for read-only accessset_chain_id_for_test()is properly restricted with#[test_only]- Function signatures are clean and consistent with the module's pattern
api/bindings.h (1)
247-254: Consistent function signature for authentication execution.The
execute_authenticatefunction signature follows the established patterns:
- Consistent parameter ordering with other execute functions
- Proper use of
ByteSliceViewfor payload parameters- Standard error handling with
UnmanagedVector *errmsg- Appropriate return type
UnmanagedVectorlibmovevm/bindings.h (1)
247-254: Consistent header declaration across binding files.The
execute_authenticatefunction declaration is identical toapi/bindings.h, which is correct for maintaining consistency across the binding interfaces.lib.go (1)
187-224: Well-implemented authentication execution method.The
ExecuteAuthenticatemethod follows the established patterns in the codebase:
- Consistent error handling with early returns
- Proper BCS serialization of inputs
- Appropriate parameter types for authentication flow
- Clean error propagation and result handling
The implementation correctly:
- Serializes environment, signature, and sender using appropriate helpers
- Calls the underlying native function with proper parameters
- Returns meaningful string result and error
api/vm.go (1)
179-212: LGTM! Clean implementation following established patterns.The
ExecuteAuthenticatefunction correctly follows the same architectural pattern as other execute functions in the file, with proper resource management, error handling, and FFI integration.libmovevm/src/vm.rs (1)
93-114: Verify that authentication operations should not persist state changes.Unlike other execute functions (
execute_contract,execute_script), this function doesn't callpush_write_setafter VM execution. This means any state changes during authentication won't be persisted to storage.Please confirm this is the intended behavior for authentication operations. If authentication should persist state changes, add:
let output = vm.execute_authenticate( gas_meter, &api, &env, &storage, &mut table_storage, authenticate_message, )?; +// push write set to storage +push_write_set(&mut storage, output.write_set())?; to_vec(&output)libmovevm/src/interface.rs (1)
212-257: LGTM! Well-structured C interface function.The implementation correctly follows the established pattern for C-exported functions with proper parameter deserialization, error handling, and gas metering.
crates/natives/src/permissioned_signer.rs (1)
105-106: Clarify the duplicate native function registration.Both
"is_permissioned_signer_impl"and"is_permissioned_signer"are registered to the same native function. Please confirm if this is intentional for compatibility reasons.crates/e2e-move-tests/src/harness.rs (4)
120-127: LGTM: Well-structured authentication method.The
authenticatemethod follows the established pattern of delegating to helper methods for message creation and execution.
287-293: LGTM: Clean message constructor.The
create_authenticate_messagemethod provides a clean abstraction for creating authentication messages.
326-350: LGTM: Consistent execution pattern.The
run_authenticatemethod follows the same pattern asrun_messageandrun_view_function, maintaining consistency in the test harness API.
76-76: LGTM: Consistent chain ID parameter addition.All
Env::newcalls have been consistently updated to include the test chain ID parameter, maintaining uniformity across the harness.Also applies to: 178-178, 328-328, 354-354, 384-384
crates/types/src/authenticator.rs (3)
10-31: Good security fix: TryFrom prevents DoS attacks.The implementation correctly addresses the previous review feedback by using
TryFrominstead ofFrom, preventing panics on invalid JSON input from external sources.
59-93: Excellent dual-mode serialization design.The custom
serde_base64module elegantly handles both human-readable (JSON with base64) and binary serialization modes, providing efficiency where needed while maintaining readability for debugging.
95-108: LGTM: Clean accessor method with pattern matching.The
signing_message_digestmethod provides a unified interface to access the digest field from both enum variants.precompile/modules/minitia_stdlib/sources/simple_map.move (2)
7-21: Appropriate deprecation with clear migration path.The deprecation notices clearly indicate the inefficiency of the O(N) implementation and recommend
ordered_map.moveas the replacement, providing users with a clear migration path.
34-34: Good modernization to method-call style.The conversion from free functions to method calls improves code readability and follows modern Move idioms. The logic and error handling remain unchanged.
Also applies to: 47-47, 62-64, 96-99
precompile/modules/minitia_stdlib/sources/bigdecimal.move (2)
54-54: Excellent API modernization to method-call style.The conversion from free functions to method calls significantly improves the API's ergonomics and follows modern Move conventions. The changes are purely stylistic with no behavioral modifications.
Also applies to: 64-64, 73-73, 83-83
139-141: Consistent modernization across all operations.All arithmetic, comparison, and conversion operations have been consistently updated to use the method-call style, providing a uniform and intuitive API.
Also applies to: 169-171, 295-297
precompile/modules/minitia_stdlib/sources/account_abstraction/auth_data.move (2)
7-17: Well-designed authentication data enum.The
AbstractionAuthDataenum provides a clean separation between regular and derivable authentication data with appropriate fields for each variant.
40-43: Good type safety with runtime assertions.The accessor functions correctly use runtime assertions to ensure type safety when accessing variant-specific fields.
Also applies to: 54-57
precompile/modules/minitia_stdlib/sources/account_abstraction/ethereum_derivable_account.move (3)
49-117: Well-structured signature versioning.The enum design with deprecated MessageV1 and current MessageV2 provides good backward compatibility. The deserialization functions properly handle both versions.
119-160: Correct SIWE message construction.The message construction properly follows the SIWE standard and Ethereum signed message format with correct length prefix calculation.
162-188: Proper ECDSA signature recovery implementation.The function correctly validates the v value (27 or 28), performs ECDSA recovery, and formats the public key with the 0x04 prefix for uncompressed format, matching ethers.js behavior.
crates/vm/src/initia_vm.rs (1)
447-451: Good: Returned signer address is properly verified.The implementation correctly validates that the signer returned from authentication matches the expected address, addressing the security concern raised in previous reviews.
precompile/modules/minitia_stdlib/sources/storage_slots_allocator.move (1)
1-263: Well-structured storage allocator implementation.The module provides a clean abstraction for addressable storage slots with:
- Optional slot reuse via linked list
- Proper invariant checks and error handling
- Clear separation between ephemeral and storable handles
- Comprehensive API for slot management
precompile/modules/minitia_stdlib/sources/account_abstraction/account_abstraction.move (1)
345-352: Excellent: Proper validation of returned signer address.The code correctly verifies that the signer returned by
dispatchable_authenticatematches the original account address, preventing privilege escalation attacks. This is a critical security check.precompile/modules/minitia_stdlib/sources/permissioned_signer.move (2)
589-597: Clarify the purpose of compatibility functions that always abort.The functions
address_ofandborrow_addressclaim to be kept for compatibility but unconditionally abort. This is confusing for users who might expect backward compatibility.Consider either:
- Providing actual backward-compatible implementations
- Marking these as deprecated with clear migration guidance
- Removing them entirely if they serve no purpose
1-667: Well-designed permissioned signer implementation with robust security controls.The module provides a comprehensive permission system with:
- Clear separation between ephemeral and storable permission handles
- Proper validation of master signers and permissions
- Expiration time support for storable handles
- Comprehensive test coverage
The implementation follows security best practices and provides a flexible framework for permission management.
precompile/modules/minitia_stdlib/sources/ordered_map.move (2)
230-296: Efficient O(n) append implementation with smart optimizations.The implementation includes:
- Fast path for non-overlapping ranges (lines 247-253)
- Clever reverse merge algorithm to avoid repeated insertions
- Proper handling of duplicate keys
Well-designed algorithm that achieves optimal complexity.
762-1459: Excellent test coverage with comprehensive edge case handling.The test suite includes:
- Basic operation tests
- Edge case and error condition tests
- Iterator behavior validation
- Performance tests with large datasets
- Proper use of #[expected_failure] annotations
precompile/modules/minitia_stdlib/sources/big_ordered_map.move (2)
74-145: Well-structured data model with future extensibilityThe use of enum variants (V1 for Node, BPlusTreeMap for BigOrderedMap) provides good forward compatibility for future schema changes.
967-999: Robust size validation and degree initializationExcellent implementation of dynamic degree calculation based on key/value sizes, with proper bounds checking against MAX_NODE_BYTES.
| public fun add_by_u64(self: BigUint, num2: u64): BigUint { | ||
| let num2 = from_u64(num2); | ||
| add(num1, num2) | ||
| add(self, num2) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inconsistent method calling styles.
There's an inconsistency in how the refactored methods call other BigUint methods:
- Line 66:
add(self, num2)uses static function call style - Lines 71, 76:
self.add(num2)uses instance method call style
For consistency with the refactor goals, all should use the instance method style.
Apply this diff to maintain consistent method calling style:
public fun add_by_u64(self: BigUint, num2: u64): BigUint {
let num2 = from_u64(num2);
- add(self, num2)
+ self.add(num2)
}Also applies to: 69-72, 74-77
🤖 Prompt for AI Agents
In precompile/modules/initia_stdlib/sources/biguint.move around lines 64 to 67
and also lines 69 to 72 and 74 to 77, the method calls to add other BigUint
values are inconsistent; some use static function style like add(self, num2)
while others use instance method style like self.add(num2). To fix this, change
all calls to use the instance method style, replacing static calls like
add(self, num2) with self.add(num2) for consistency.
| public(friend) fun base16_utf8_to_vec_u8(str: vector<u8>): vector<u8> { | ||
| let result = vector::empty<u8>(); | ||
| let i = 0; | ||
| while (i < vector::length(&str)) { | ||
| let c1 = vector::borrow(&str, i); | ||
| let c2 = vector::borrow(&str, i + 1); | ||
| let byte = hex_char_to_u8(*c1) << 4 | hex_char_to_u8(*c2); | ||
| vector::push_back(&mut result, byte); | ||
| i = i + 2; | ||
| }; | ||
| result | ||
| } |
There was a problem hiding this comment.
Add input length validation to prevent out-of-bounds access.
The function doesn't validate that the input string has even length. If an odd-length string is passed, line 26 will attempt to access i + 1 beyond the vector bounds, causing a runtime abort.
Add length validation at the beginning:
public(friend) fun base16_utf8_to_vec_u8(str: vector<u8>): vector<u8> {
+ assert!(vector::length(&str) % 2 == 0, 2); // Error code 2 for invalid length
let result = vector::empty<u8>();
let i = 0;📝 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.
| public(friend) fun base16_utf8_to_vec_u8(str: vector<u8>): vector<u8> { | |
| let result = vector::empty<u8>(); | |
| let i = 0; | |
| while (i < vector::length(&str)) { | |
| let c1 = vector::borrow(&str, i); | |
| let c2 = vector::borrow(&str, i + 1); | |
| let byte = hex_char_to_u8(*c1) << 4 | hex_char_to_u8(*c2); | |
| vector::push_back(&mut result, byte); | |
| i = i + 2; | |
| }; | |
| result | |
| } | |
| public(friend) fun base16_utf8_to_vec_u8(str: vector<u8>): vector<u8> { | |
| assert!(vector::length(&str) % 2 == 0, 2); // Error code 2 for invalid length | |
| let result = vector::empty<u8>(); | |
| let i = 0; | |
| while (i < vector::length(&str)) { | |
| let c1 = vector::borrow(&str, i); | |
| let c2 = vector::borrow(&str, i + 1); | |
| let byte = hex_char_to_u8(*c1) << 4 | hex_char_to_u8(*c2); | |
| vector::push_back(&mut result, byte); | |
| i = i + 2; | |
| }; | |
| result | |
| } |
🤖 Prompt for AI Agents
In precompile/modules/minitia_stdlib/sources/account_abstraction/base16.move
around lines 21 to 32, the function base16_utf8_to_vec_u8 does not check if the
input vector length is even, which can cause out-of-bounds access when accessing
i + 1. Add a validation at the start of the function to ensure the input vector
length is even, and handle the error appropriately (e.g., abort or return an
error) before proceeding with the conversion loop.
| public fun derivable_abstract_signature(self: &AbstractionAuthData): &vector<u8> { | ||
| assert!(self is DerivableV1, error::invalid_argument(ENOT_REGULAR_AUTH_DATA)); | ||
| &self.abstract_signature | ||
| } |
There was a problem hiding this comment.
Fix inconsistent error code.
The error code should be ENOT_DERIVABLE_AUTH_DATA since this function requires DerivableV1 variant data.
- assert!(self is DerivableV1, error::invalid_argument(ENOT_REGULAR_AUTH_DATA));
+ assert!(self is DerivableV1, error::invalid_argument(ENOT_DERIVABLE_AUTH_DATA));📝 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.
| public fun derivable_abstract_signature(self: &AbstractionAuthData): &vector<u8> { | |
| assert!(self is DerivableV1, error::invalid_argument(ENOT_REGULAR_AUTH_DATA)); | |
| &self.abstract_signature | |
| } | |
| public fun derivable_abstract_signature(self: &AbstractionAuthData): &vector<u8> { | |
| assert!(self is DerivableV1, error::invalid_argument(ENOT_DERIVABLE_AUTH_DATA)); | |
| &self.abstract_signature | |
| } |
🤖 Prompt for AI Agents
In precompile/modules/minitia_stdlib/sources/account_abstraction/auth_data.move
around lines 49 to 52, the error code used in the assert statement is incorrect
for the function derivable_abstract_signature. Replace the error code
ENOT_REGULAR_AUTH_DATA with ENOT_DERIVABLE_AUTH_DATA to correctly reflect that
this function requires the DerivableV1 variant.
| }; | ||
|
|
||
| // Handle leading zeros (1's in Base58) | ||
| let i = 0; |
There was a problem hiding this comment.
Variable i redeclared without mut
The variable i on line 134 is mutated in the loop but declared without mut.
- let i = 0;
+ let mut i = 0;📝 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 i = 0; | |
| let mut i = 0; |
🤖 Prompt for AI Agents
In
precompile/modules/minitia_stdlib/sources/account_abstraction/solana_derivable_account.move
at line 134, the variable `i` is declared without `mut` but is mutated later in
the loop. Fix this by adding the `mut` keyword to the declaration of `i` to
allow mutation.
| /// Apply the function to a reference of each key-value pair in the map. | ||
| /// | ||
| /// Current implementation is O(n * log(n)). After function values will be optimized | ||
| /// to O(n). | ||
| public inline fun for_each_ref<K: copy + drop, V>( | ||
| self: &OrderedMap<K, V>, f: |&K, &V| | ||
| ) { | ||
| // This implementation is innefficient: O(log(n)) for next_key / borrow lookups every time, | ||
| // but is the only one available through the public API. | ||
| if (!self.is_empty()) { | ||
| let (k, v) = self.borrow_front(); | ||
| f(k, v); | ||
|
|
||
| let cur_k = self.next_key(k); | ||
| while (cur_k.is_some()) { | ||
| let k = cur_k.destroy_some(); | ||
| f(&k, self.borrow(&k)); | ||
|
|
||
| cur_k = self.next_key(&k); | ||
| }; | ||
| }; | ||
|
|
||
| // TODO: if we make iterator api public update to: | ||
| // let iter = self.new_begin_iter(); | ||
| // while (!iter.iter_is_end(self)) { | ||
| // f(iter.iter_borrow_key(self), iter.iter_borrow(self)); | ||
| // iter = iter.iter_next(self); | ||
| // } | ||
|
|
||
| // TODO: once move supports private functions udpate to: | ||
| // vector::for_each_ref( | ||
| // &self.entries, | ||
| // |entry| { | ||
| // f(&entry.key, &entry.value) | ||
| // } | ||
| // ); | ||
| } |
There was a problem hiding this comment.
Fix the inefficient O(n log n) implementation of for_each_ref.
The current implementation performs O(log n) lookups for each element, resulting in O(n log n) complexity. The TODO comments show the correct O(n) approach using iterators.
The function should use the iterator API (even if internal) to achieve O(n) complexity. The current implementation will have poor performance on large maps. Consider making the iterator API public or using the friend function for_each_ref_friend instead.
🤖 Prompt for AI Agents
In precompile/modules/minitia_stdlib/sources/ordered_map.move between lines 628
and 664, the for_each_ref function is inefficient with O(n log n) complexity due
to repeated lookups. To fix this, replace the current implementation with one
that uses the iterator API to achieve O(n) complexity, as suggested in the TODO
comments. If the iterator API is not public, consider using the friend function
for_each_ref_friend or make the iterator API accessible to enable efficient
iteration over the map entries.
| public fun add_all<K, V>( | ||
| self: &mut OrderedMap<K, V>, | ||
| keys: vector<K>, | ||
| values: vector<V> | ||
| ) { | ||
| // TODO: Can be optimized, by sorting keys and values, and then creating map. | ||
| keys.zip( | ||
| values, |key, value| { | ||
| self.add(key, value); | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| /// Add multiple key/value pairs to the map, overwrites values if they exist already, | ||
| /// or if duplicate keys are passed in. | ||
| public fun upsert_all<K: drop, V: drop>( | ||
| self: &mut OrderedMap<K, V>, | ||
| keys: vector<K>, | ||
| values: vector<V> | ||
| ) { | ||
| // TODO: Can be optimized, by sorting keys and values, and then creating map. | ||
| keys.zip( | ||
| values, |key, value| { | ||
| self.upsert(key, value); | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement the suggested optimization for bulk operations.
The TODO comments correctly identify that add_all and upsert_all can be optimized from O(n²) to O(n log n) by sorting the input vectors first.
Consider implementing the optimization:
public fun add_all<K, V>(
self: &mut OrderedMap<K, V>,
keys: vector<K>,
values: vector<V>
) {
// Sort keys and values together by key
let pairs = keys.zip_with(values, |k, v| Entry { key: k, value: v });
pairs.sort_by(|a, b| cmp::compare(&a.key, &b.key));
// Merge sorted entries with existing map
// ... merge implementation
}🤖 Prompt for AI Agents
In precompile/modules/minitia_stdlib/sources/ordered_map.move lines 183 to 209,
the add_all and upsert_all functions currently insert key-value pairs one by
one, resulting in O(n²) complexity. To optimize, first zip keys and values into
a vector of entries, then sort this vector by keys to achieve O(n log n)
sorting. After sorting, merge these sorted entries efficiently into the existing
map, replacing or adding entries as needed. Implement this sorting and merging
logic to improve performance of bulk insertions.
| leaf_max_degree_from_max >= (INNER_MIN_DEGREE as u64), | ||
| error::invalid_argument(EINVALID_CONFIG_PARAMETER) | ||
| ); |
There was a problem hiding this comment.
Fix assertion to use correct constant for leaf validation
The assertion is checking leaf max degree against INNER_MIN_DEGREE instead of LEAF_MIN_DEGREE.
- assert!(
- leaf_max_degree_from_max >= (INNER_MIN_DEGREE as u64),
- error::invalid_argument(EINVALID_CONFIG_PARAMETER)
+ assert!(
+ leaf_max_degree_from_max >= (LEAF_MIN_DEGREE as u64),
+ error::invalid_argument(EINVALID_CONFIG_PARAMETER)📝 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.
| leaf_max_degree_from_max >= (INNER_MIN_DEGREE as u64), | |
| error::invalid_argument(EINVALID_CONFIG_PARAMETER) | |
| ); | |
| assert!( | |
| leaf_max_degree_from_max >= (LEAF_MIN_DEGREE as u64), | |
| error::invalid_argument(EINVALID_CONFIG_PARAMETER) | |
| ); |
🤖 Prompt for AI Agents
In precompile/modules/minitia_stdlib/sources/big_ordered_map.move around lines
214 to 216, the assertion incorrectly compares leaf_max_degree_from_max with
INNER_MIN_DEGREE instead of LEAF_MIN_DEGREE. Update the assertion to use
LEAF_MIN_DEGREE as the constant for validating leaf_max_degree_from_max to
ensure correct validation logic.
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Documentation
Tests
Chores