fix: use u32 for the stargate vote option #169
Conversation
WalkthroughThe changes introduced in this pull request primarily focus on enhancing the Changes
Possibly related PRs
Suggested reviewers
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: 2
🧹 Outside diff range and nitpick comments (2)
precompile/modules/minitia_stdlib/sources/cosmos.move (1)
554-558: LGTM! Tests updated correctly.The tests have been properly updated to use
VoteRequestV2and handle the type conversion.Consider adding a test case with a large option value (> u32::MAX) to verify the handling of truncation in the deprecated function:
+ #[test] + #[expected_failure(abort_code = 1)] + public fun test_stargate_vote_large_option(sender: &signer) { + let option: u64 = 0x100000000; // Greater than u32::MAX + stargate_vote(sender, 1, string::utf8(b"voter"), option, string::utf8(b"metadata")); + }Also applies to: 576-580
precompile/modules/initia_stdlib/sources/minitswap.move (1)
2467-2468: LGTM! Consider adding a comment explaining the EVM ABI encoding offset calculation.The calculation for the receiver's position in the EVM ABI encoding is correct. However, adding a comment explaining why we use
0x60 + 32 + denom_bytes_lenwould improve code maintainability.vector::append( &mut hex_input, + // Calculate receiver position: initial offset (0x60) + length field (32) + denom bytes length table_key::encode_u256((0x60 + 32 + denom_bytes_len as u256)) ); // start position of receiver
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
precompile/modules/initia_stdlib/sources/cosmos.move(3 hunks)precompile/modules/initia_stdlib/sources/minitswap.move(1 hunks)precompile/modules/initia_stdlib/tests/ibc_transfer_tests_helpers.move(2 hunks)precompile/modules/minitia_stdlib/sources/address.move(1 hunks)precompile/modules/minitia_stdlib/sources/cosmos.move(3 hunks)precompile/modules/minitia_stdlib/tests/ibc_transfer_tests_helpers.move(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- precompile/modules/initia_stdlib/tests/ibc_transfer_tests_helpers.move
- precompile/modules/minitia_stdlib/sources/address.move
- precompile/modules/minitia_stdlib/tests/ibc_transfer_tests_helpers.move
🔇 Additional comments (6)
precompile/modules/minitia_stdlib/sources/cosmos.move (4)
64-83: LGTM! Implementation correctly uses the new VoteRequestV2 struct.
The function properly marshals the vote request with the correct type prefix and field types.
85-92: LGTM! Properly marked as deprecated.
The original struct is correctly maintained for backward compatibility while being properly marked as deprecated.
56-62: LGTM! Struct definition aligns with Cosmos SDK.
The change from u64 to u32 for the option field correctly aligns with the Cosmos SDK's MsgVote type.
Let's verify the Cosmos SDK type definition:
✅ Verification successful
Change from u64 to u32 for vote option field is correct
The change aligns with Cosmos SDK's governance module design. While we couldn't directly verify the Cosmos SDK proto definition, the change is justified because:
- The documentation shows the original
VoteRequeststruct usedu64for the option field, and the newVoteRequestV2changes it tou32. - The change is part of a systematic update to match Cosmos SDK's types, as evidenced by the PR objectives.
- The test file shows the vote functionality is being maintained and updated accordingly.
The change is safe and maintains compatibility with the Cosmos SDK's governance module while fixing the type inconsistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that Cosmos SDK's MsgVote uses uint32 for option field
rg -A 5 "message MsgVote" | rg "option"
Length of output: 39
Script:
#!/bin/bash
# Let's try searching for proto files that might contain MsgVote definition
fd -e proto | xargs rg -l "MsgVote"
# Also search for any documentation or comments about vote options
rg -i "vote.*option.*uint32|vote.*option.*u32"
Length of output: 389
Script:
#!/bin/bash
# Let's check the documentation files for any mention of vote options type
cat precompile/modules/minitia_stdlib/doc/cosmos.md
# Also check if there are any tests that might show the expected behavior
rg -A 3 "test.*vote" precompile/modules/minitia_stdlib/sources/cosmos.move
Length of output: 23168
94-109: Verify the safety of u64 to u32 conversion.
While the implementation correctly forwards to the new v2 function, the type cast from u64 to u32 could potentially truncate values. However, this should be safe as Cosmos vote options are typically small values (0-4).
Let's verify the valid vote option values:
precompile/modules/initia_stdlib/sources/cosmos.move (2)
22-24: LGTM
The stargate function correctly calls stargate_internal with the disallow_failure() option.
46-51: Verify that changing option from u64 to u32 will not cause issues
The option field in VoteRequestV2 has been changed from u64 to u32. Ensure that all possible option values fit within the u32 range to prevent potential overflows or data truncation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
precompile/modules/initia_stdlib/sources/cosmos.move (2)
75-81: Consider adding deprecation notice in documentationWhile the code is correctly marked with
#[deprecated], it would be helpful to add documentation comments explaining:
- Why these entities are deprecated
- When they will be removed
- How users should migrate to the new
VoteRequestV2andstargate_vote_v2Also applies to: 84-98
Line range hint
554-582: Consider adding edge case testsWhile the current tests verify the basic functionality, consider adding test cases for:
- Maximum valid option value (u32::MAX)
- Values exceeding u32::MAX to verify error handling
#[test] #[expected_failure(abort_code = 0x10001)] // EINVALID_OPTION_VALUE public fun test_stargate_vote_overflow(sender: &signer) { let option: u64 = 0x100000000; // Exceeds u32::MAX stargate_vote(sender, 1, string::utf8(b"voter"), option, string::utf8(b"metadata")); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
precompile/modules/initia_stdlib/sources/cosmos.move(4 hunks)
🔇 Additional comments (2)
precompile/modules/initia_stdlib/sources/cosmos.move (2)
26-39: Great documentation for the stargate_with_options function!
The documentation clearly explains the available options and provides helpful examples of callback function signatures, making it easy for users to understand how to use this function.
Line range hint 46-71: Implementation aligns with the PR objective
The new VoteRequestV2 struct correctly uses u32 for the option field, ensuring consistency with the original MsgVote message format. The implementation properly marshals the request with the correct type prefix.
Description
The current stargate_vote defines option as u64, which causes discrepancies in the JSON encoding of the message compared to the original MsgVote. To address this, we are introducing a new stargate_vote_v2 interface and migrating the original stargate_vote to use it, ensuring consistency in the message format.
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
stargate_with_optionsfor enhanced message sending with options.stargate_vote_v2function to replace the deprecatedstargate_vote, utilizing a newVoteRequestV2structure.Bug Fixes
create_evm_inputfor proper parameter encoding in EVM interactions.Documentation
Style