fix: metadata to denom #135
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Outside diff range and nitpick comments (2)
precompile/modules/initia_stdlib/sources/coin.move (2)
333-337: LGTM! Consider adding a comment explaining the "uinit" conversion.The addition of special handling for the "INIT" symbol is appropriate and consistent with the function's purpose. It effectively reverts the conversion done in
fungible_asset::metadata().Consider adding a brief comment explaining why "INIT" is converted to "uinit". This would improve code readability and maintainability. For example:
// Convert "INIT" to "uinit" to maintain consistency with the base unit representation if (*string::bytes(&symbol) == b"INIT") { symbol = string::utf8(b"uinit") };
Line range hint
426-443: LGTM! Consider adding more test cases for comprehensive coverage.The new test function
test_denom_metadata_converteffectively validates the conversion between metadata and denom for the "INIT" symbol, which is crucial given the recent changes tometadata_to_denom. The test appropriately covers both chain and non-chain scenarios.To enhance test coverage and robustness, consider adding the following:
- A test case with a non-"INIT" symbol to ensure the general case still works correctly.
- Explicit assertions for the intermediate
denomanddenom_values to verify they are as expected.- A test case that checks the behavior when converting an already "uinit" symbol.
Example additions:
// Test non-"INIT" symbol let (_, _, _) = initialize_coin_for_testing(¬_chain, string::utf8(b"TEST")); let test_metadata = metadata(std::signer::address_of(¬_chain), string::utf8(b"TEST")); let test_denom = metadata_to_denom(test_metadata); let test_metadata_from_denom = denom_to_metadata(test_denom); assert!(test_metadata == test_metadata_from_denom, 2); // Assert intermediate denom values assert!(denom == string::utf8(b"uinit"), 3); assert!(string::index_of(&denom_, string::utf8(b"move/")) == 0, 4); // Test "uinit" symbol let (_, _, _) = initialize_coin_for_testing(&chain, string::utf8(b"uinit")); let uinit_metadata = metadata(std::signer::address_of(&chain), string::utf8(b"uinit")); let uinit_denom = metadata_to_denom(uinit_metadata); assert!(uinit_denom == string::utf8(b"uinit"), 5);These additions would provide more comprehensive test coverage for the
metadata_to_denomanddenom_to_metadatafunctions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
precompile/modules/initia_stdlib/sources/multisig_v2.move (2)
Line range hint
958-965: Potential Issue with Index Mutation Inside ClosureIn the
create_votes_mapfunction, theindexvariable is mutated inside the closure passed tovector::for_each. Depending on how closures handle variable captures, this may lead to unexpected behavior, asindexmight not increment correctly. To ensure proper indexing, consider usingvector::zipto iterate over bothmembersandvotessimultaneously.Apply this diff to fix the issue:
-fun create_votes_map(members: vector<Member>, votes: vector<bool>): - SimpleMap<Member, bool> { - let votes_map = simple_map::create<Member, bool>(); - let index = 0; - vector::for_each( - members, - |member| { - let vote = *vector::borrow(&votes, index); - index = index + 1; - - simple_map::add(&mut votes_map, member, vote) - } - ); - - votes_map -} +fun create_votes_map(members: vector<Member>, votes: vector<bool>): + SimpleMap<Member, bool> { + let votes_map = simple_map::create<Member, bool>(); + let pairs = vector::zip(members, votes); + vector::for_each( + pairs, + |(member, vote)| { + simple_map::add(&mut votes_map, member, vote) + } + ); + + votes_map +}
Line range hint
1295-1302: Potential Issue with Index Mutation Inside ClosureIn the closure passed to
vector::for_each_ref, theindexvariable is mutated. This may result in incorrect indexing due to how variable captures work in closures. To synchronize iteration overmultisig_wallet.membersandmember_tiers, consider usingvector::zipto pair elements directly.Apply this diff to fix the issue:
-let index = 0; -vector::for_each_ref( - &multisig_wallet.members, - |member| { - let tier_name = *vector::borrow(&member_tiers, index); - index = index + 1; - - let m: &Member = member; - let tier = option::borrow(&m.tier); - assert!(tier_name == tier.name, 1) - } -); +let pairs = vector::zip(multisig_wallet.members, member_tiers); +vector::for_each( + pairs, + |(member, tier_name)| { + let m: &Member = &member; + let tier = option::borrow(&m.tier); + assert!(tier_name == tier.name, 1) + } +);precompile/modules/minitia_stdlib/sources/multisig_v2.move (6)
Line range hint
215-329: Refactorcreate_proposalfunctions to eliminate code duplicationThe functions
create_proposalandcreate_proposal_with_jsonshare a significant amount of code, differing mainly in how they handleargs_listand constructExecuteMessage. Refactoring these functions to consolidate the shared logic will enhance maintainability and reduce potential bugs.Consider combining these into a single function that accepts an additional parameter indicating whether to use JSON arguments. You can then handle the differences within this unified function.
For example:
public entry fun create_proposal_generic( account: &signer, multisig_addr: address, module_address_list: vector<address>, module_name_list: vector<String>, function_name_list: vector<String>, type_args_list: vector<vector<String>>, args_list: vector<any>, // Could be vector<vector<vector<u8>>> or vector<vector<String>> is_json: bool, expiry_duration: Option<u64> ) acquires MultisigWallet { // Shared logic here }This approach centralizes the proposal creation logic and differentiates the handling of
args_listbased on theis_jsonflag.
Line range hint
12-24: Avoid mutable assignment to an immutable variablelimitIn the function
get_proposals, the variablelimitis an input parameter of typeu8. You attempt to modify it within the function:if (limit > MAX_LIMIT) { limit = MAX_LIMIT };Since function parameters are immutable by default in Move, reassigning
limitwill result in a compilation error.To fix this, declare a new mutable local variable to hold the adjusted limit:
+ let mut adjusted_limit = limit; if (limit > MAX_LIMIT) { - limit = MAX_LIMIT + adjusted_limit = MAX_LIMIT };Then use
adjusted_limitin place oflimitthroughout the function.
Line range hint
362-378: Potential division by zero inyes_vote_scorefunctionIn the
yes_vote_scorefunction, when calculating theyes_vote_score, there's a potential risk if the weight of a member is zero, which could lead to unexpected results or division by zero in further computations.Ensure that all member weights are non-zero to prevent any arithmetic errors. Consider adding a check when assigning weights to tiers to enforce this constraint.
Line range hint
755-770: Possible misuse ofstring::appendwith immutable variablesIn the
get_multisig_addresstest function,seedis defined as an immutable variable, but you attempt to mutate it usingstring::append:let seed = address::to_string(@minitia_std); string::append(&mut seed, string::utf8(b"::multisig_v2::MultisigWallet"));This will cause a compilation error because
seedis not mutable.To fix this, declare
seedas mutable:-let seed = address::to_string(@minitia_std); +let mut seed = address::to_string(@minitia_std); string::append(&mut seed, string::utf8(b"::multisig_v2::MultisigWallet"));
Line range hint
138-149: Variableindexshadows outer scope in closuresIn several functions, such as
create_proposalandcreate_proposal_with_json, you declarelet index = 0;outside a closure but then modify it inside the closure. This pattern can lead to errors due to variable immutability and closure capture rules in Move.Consider using
vector::indexed_maporvector::indexed_for_eachto access the index without needing to manage it manually.Apply this change in the
create_proposalfunction:-let index = 0; -let execute_messages = vector::map<address, ExecuteMessage>( +let execute_messages = vector::indexed_map<address, ExecuteMessage>( module_address_list, - |module_address| { + |index, module_address| { let module_name = *vector::borrow(&module_name_list, index); let function_name = *vector::borrow(&function_name_list, index); let type_args = *vector::borrow(&type_args_list, index); let args = *vector::borrow(&args_list, index); - index = index + 1; ExecuteMessage { module_address, module_name, function_name, type_args, args, json_args: vector[] } } );Make a similar change in
create_proposal_with_json.
Line range hint
647-677: Uninitialized variabletotal_weightinupdate_configfunctionIn the
update_configfunction, when the multisig wallet is not weighted (weightedisfalse), the variabletotal_weightis not initialized before being used to update proposals:proposal.total_weight = total_weight(&multisig_wallet.members);This could lead to unexpected behavior or a compilation error if
total_weightis expected to be defined.Ensure that
total_weightis properly initialized in both branches of the conditional. You can move the calculation oftotal_weightoutside theif...elseblock:// After updating members and threshold + let total_weight = total_weight(&multisig_wallet.members); // remove votes of the removed members from active proposals let iter = ... while (table::prepare<u64, Proposal>(iter)) { ... proposal.threshold = new_threshold; - proposal.total_weight = total_weight(&multisig_wallet.members); + proposal.total_weight = total_weight; ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- precompile/modules/initia_stdlib/sources/coin.move (1 hunks)
- precompile/modules/initia_stdlib/sources/multisig_v2.move (2 hunks)
- precompile/modules/minitia_stdlib/sources/multisig_v2.move (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- precompile/modules/initia_stdlib/sources/coin.move
🔇 Additional comments (1)
precompile/modules/initia_stdlib/sources/multisig_v2.move (1)
Line range hint
235-240: Ensure Status Index is Within BoundsIn the
proposal_to_proposal_responsefunction, thestatus_indexvariable could potentially exceed the bounds of theSTATUSvector if additional statuses are introduced without updating the constant. To prevent out-of-bounds access, consider adding a check to ensurestatus_indexis valid.Run the following script to check for any uses of
status_indexthat might exceed the length ofSTATUS:✅ Verification successful
Status Index Bounds Check Verified
All instances of
status_indexused to access theSTATUSvector are within bounds. No potential out-of-bounds access was detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that status_index is within the bounds of STATUS vector # Expected: All instances of status_index used as an index should be less than the length of STATUS ast-grep --lang move --pattern $'vector::borrow(&STATUS, $_status_index)' \ | grep -E 'status_index\s*>=\s*vector::length\(&STATUS\)'Length of output: 241
Script:
#!/bin/bash # Description: Verify that status_index is within the bounds of STATUS vector using ripgrep # Search for all instances where status_index is used to index STATUS rg --ignore-case 'STATUS\[[^]]*status_index[^]]*\]' . # Search for boundary checks related to status_index rg --ignore-case 'status_index\s*<' . && rg --ignore-case 'status_index\s*<=' .Length of output: 95
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
Bug Fixes