feat(gov): Add VoteWeighted transaction, unit and integration tests. #2922
Conversation
WalkthroughThe changes in this pull request involve significant updates to the governance module of the Evmos blockchain, particularly introducing a weighted voting mechanism. Key modifications include the addition of new methods, events, and error handling related to weighted votes. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (12)
precompiles/gov/errors.go (2)
24-25: LGTM with a suggestion: New error constant for invalid weighted vote option weight.The constant
ErrInvalidWeightedVoteOptionWeightis well-named and consistent with the other new constants. It provides a specific error message for cases where the weight of a weighted vote option is invalid.However, I have a small suggestion:
Consider using
%vinstead of%sin the error message format. Weights are typically numeric, and%vwould allow for more flexible formatting of the invalid weight value, whether it's a string, number, or another type.- ErrInvalidWeightedVoteOptionWeight = "invalid weighted vote option weight %s " + ErrInvalidWeightedVoteOptionWeight = "invalid weighted vote option weight %v "
18-25: Overall assessment: Comprehensive error handling for weighted voting.The addition of these four new error constants significantly enhances the error handling capabilities for weighted voting in the governance module. They provide specific and detailed error messages for various aspects of weighted voting, including invalid options, option types, and weights.
These additions will greatly improve the ability to debug issues related to weighted voting and provide more informative feedback to users. The constants are well-named, consistent with the existing code style, and cover a good range of potential error scenarios.
To further improve the robustness of the governance module, consider the following:
- Ensure that these new error constants are used consistently throughout the codebase where weighted voting is implemented.
- Update any relevant documentation or user guides to include information about these new error types.
- Consider adding unit tests that specifically check for these error conditions to ensure they are triggered appropriately.
precompiles/gov/events.go (1)
72-72: Remove unused linter directive.The
//nolint:gosec // G115directive on this line appears to be unused for the "gosec" linter. Consider removing it if it's no longer necessary.You can remove the directive like this:
- packed, err := arguments.Pack(proposalID, options) //nolint:gosec // G115 + packed, err := arguments.Pack(proposalID, options)🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 72-72:
directive//nolint:gosec // G115is unused for linter "gosec" (nolintlint)precompiles/gov/gov.go (1)
107-108: LGTM! Consider adding a comment for the new method.The addition of the
VoteWeightedMethodcase in theRunmethod is consistent with the existing structure and aligns with the PR objective.Consider adding a brief comment explaining the purpose of the
VoteWeightedmethod, similar to the existing comment for theVotemethod:case VoteMethod: bz, err = p.Vote(ctx, evm.Origin, contract, stateDB, method, args) case VoteWeightedMethod: + // Handle weighted voting bz, err = p.VoteWeighted(ctx, evm.Origin, contract, stateDB, method, args)precompiles/gov/events_test.go (2)
95-138: Consider adding more test cases for comprehensive coverage.The current test case effectively checks for a successful event emission with multiple voting options. To enhance test coverage, consider adding the following test cases:
- A case with a single voting option (100% weight).
- A case with the maximum allowed number of voting options.
- An error case where the sum of weights exceeds 1.0.
- An error case with an invalid option number.
These additional cases will help ensure the robustness of the
VoteWeightedfunctionality across various scenarios.🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 120-120:
G115: integer overflow conversion int64 -> uint64 (gosec)
88-167: Overall, good implementation with room for improvement.The
TestVoteWeightedEventmethod is well-structured and consistent with existing test patterns. It effectively tests the basic functionality of weighted voting. However, consider the following improvements:
- Expand test coverage with additional test cases as suggested earlier.
- Address the potential integer overflow issue in the log block number comparison.
- Consider adding more detailed assertions in the
postCheckfunction, such as verifying the metadata field.These enhancements will further improve the robustness and reliability of the test suite for the weighted voting functionality.
Would you like assistance in implementing these improvements?
🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 120-120:
G115: integer overflow conversion int64 -> uint64 (gosec)precompiles/gov/abi.json (1)
31-67: LGTM! Consider using a numeric type for weight.The new
VoteWeightedevent structure aligns well with the PR objectives of introducing weighted voting. The use of a tuple array for options allows for multiple weighted votes per proposal, which is a flexible approach.Consider using a numeric type (e.g.,
uint256) for theweightinstead ofstring. This could improve gas efficiency and make it easier to perform calculations with the weights on-chain if needed in the future.CHANGELOG.md (3)
Line range hint
60-64: Multiple module removals and refactorsThe removal of the
x/recovery,x/incentives, andx/claimsmodules, along with burning of incentives pool balance, represents a significant change to the system's functionality. This could impact users who were relying on these features.Please address the following concerns:
- Ensure there's a clear migration path for users affected by the removed modules.
- Verify that burning the incentives pool balance doesn't negatively impact any ongoing incentive programs.
- Update all documentation to reflect these removals and changes.
🧰 Tools
🪛 Markdownlint
58-58: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
67-67: Significant change: Inflation reductionReducing inflation by 2/3 is a major economic change that will affect token holders and the overall tokenomics of the system.
Please ensure the following:
- This change has been thoroughly analyzed for its long-term economic impact.
- Stakeholders have been informed and consensus has been reached.
- Update all relevant documentation and economic models to reflect this change.
🧰 Tools
🪛 Markdownlint
58-58: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
72-72: Burning of usage incentives pool balanceThis change, along with the removal of the incentives module, represents a significant shift in the platform's incentive structure.
Please address the following:
- Ensure all affected users are notified of this change.
- Verify that no ongoing incentive programs will be disrupted.
- Update all relevant documentation to reflect the removal of incentives.
🧰 Tools
🪛 Markdownlint
58-58: Expected: 120; Actual: 136
Line length(MD013, line-length)
precompiles/gov/IGov.sol (2)
51-55: Improve documentation for theVoteWeightedeventThe documentation comments for the
VoteWeightedevent can be made clearer and more grammatically correct:
- Line 51: Clarify the event description.
- Line 53: Correct the phrase "the proposal of id" to "the ID of the proposal."
- Line 54: Enhance the description of the
optionsparameter.Suggested changes:
51- /// @dev VoteWeighted defines an Event emitted when a proposal voted. 51+ /// @dev VoteWeighted event is emitted when a weighted vote is cast on a proposal. 53- /// @param proposalId the proposal of id 53+ /// @param proposalId the ID of the proposal 54- /// @param options the options for voter 54+ /// @param options the weighted vote options provided by the voter
72-83: Enhance documentation for thevoteWeightedfunctionTo improve clarity and correctness in the documentation comments for the
voteWeightedfunction:
- Line 72: Specify that the method adds a weighted vote.
- Line 75: Provide a more descriptive explanation for the
optionsparameter.- Line 76: Correct the grammar in the
metadataparameter description.Suggested changes:
72- /// @dev voteWeighted defines a method to add a vote on a specific proposal. 72+ /// @dev voteWeighted defines a method to add a weighted vote on a specific proposal. 75- /// @param options The options for voter 75+ /// @param options The weighted vote options selected by the voter 76- /// @param metadata The metadata for voter send 76+ /// @param metadata The metadata provided by the voter
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- CHANGELOG.md (1 hunks)
- precompiles/gov/IGov.sol (2 hunks)
- precompiles/gov/abi.json (3 hunks)
- precompiles/gov/errors.go (1 hunks)
- precompiles/gov/events.go (2 hunks)
- precompiles/gov/events_test.go (1 hunks)
- precompiles/gov/gov.go (2 hunks)
- precompiles/gov/integration_test.go (2 hunks)
- precompiles/gov/tx.go (2 hunks)
- precompiles/gov/tx_test.go (1 hunks)
- precompiles/gov/types.go (3 hunks)
🧰 Additional context used
🪛 GitHub Check: Run golangci-lint
precompiles/gov/events.go
[failure] 72-72:
directive//nolint:gosec // G115is unused for linter "gosec" (nolintlint)precompiles/gov/events_test.go
[failure] 120-120:
G115: integer overflow conversion int64 -> uint64 (gosec)
🔇 Additional comments (25)
precompiles/gov/errors.go (3)
18-19: LGTM: New error constant for invalid weighted vote options.The new constant
ErrInvalidWeightedVoteOptionsis well-named and consistent with the existing error message style. It provides a clear error message for invalid weighted vote options, which will be helpful for debugging and user feedback.
20-21: LGTM: New error constant for an invalid weighted vote option.The constant
ErrInvalidWeightedVoteOptionis appropriately named and consistent with the existing error message style. It provides a specific error message for a single invalid weighted vote option, which will be useful for pinpointing issues with individual options.
22-23: LGTM: New error constant for invalid weighted vote option type.The constant
ErrInvalidWeightedVoteOptionTypeis well-defined and follows the established naming convention. It provides a specific error message for cases where the type of a weighted vote option is invalid, which will help in identifying and debugging type-related issues in weighted voting.precompiles/gov/events.go (3)
19-20: LGTM: New constant for weighted vote event type.The addition of
EventTypeVoteWeightedconstant is consistent with the existing code structure and naming conventions. It clearly defines the event type for the new weighted voting functionality.
55-85: LGTM: New method for emitting weighted vote events.The
EmitVoteWeightedEventmethod is well-implemented and consistent with the existingEmitVoteEventmethod. It correctly uses the newEventTypeVoteWeightedconstant, handles errors appropriately, and properly packs and logs the event data.🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 72-72:
directive//nolint:gosec // G115is unused for linter "gosec" (nolintlint)
Line range hint
1-85: Overall changes enhance governance module functionality.The additions to this file (new constant
EventTypeVoteWeightedand methodEmitVoteWeightedEvent) successfully extend the governance module to support weighted voting events. These changes are well-integrated, maintain consistency with existing code, and enhance the module's capabilities without introducing apparent issues.🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 72-72:
directive//nolint:gosec // G115is unused for linter "gosec" (nolintlint)precompiles/gov/gov.go (2)
139-139: LGTM! Changes are consistent and well-documented.The updates to the
IsTransactionmethod are appropriate:
- The comment has been updated to include
VoteWeightedas an available transaction.VoteWeightedMethodhas been added to the switch case, ensuring it's correctly identified as a transaction.These changes align well with the introduction of the new
VoteWeightedfunctionality.Also applies to: 142-142
Line range hint
1-153: Verify completeness of VoteWeighted implementationThe changes in this file look good and are consistent with the PR objectives. However, to ensure a complete implementation of the
VoteWeightedfunctionality, we should verify if there are any other files that might need updates.Let's run a script to check for any potential missing implementations or references:
✅ Verification successful
Let's refine our verification by accurately checking the
abi.jsonstructure for thevoteWeightedmethod.
VoteWeighted implementation is complete
The
VoteWeightedfunctionality is fully implemented across the codebase, including all necessary references and ABI definitions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential missing implementations or references to VoteWeighted # Test 1: Search for VoteWeighted references echo "Searching for VoteWeighted references:" rg --type go "VoteWeighted" # Test 2: Check if VoteWeighted is implemented in the ABI echo "Checking ABI for VoteWeighted:" cat precompiles/gov/abi.json | jq '.[] | select(.name == "voteWeighted")' # Test 3: Look for potential test files echo "Searching for potential test files:" fd -e go testLength of output: 12961
Script:
#!/bin/bash # Description: Verify the presence of VoteWeighted in the ABI with correct jq query echo "Checking ABI structure for VoteWeighted:" jq -r 'paths(scalars) as $p | select(getpath($p) == "voteWeighted") | $p' precompiles/gov/abi.jsonLength of output: 218
precompiles/gov/events_test.go (1)
88-93: LGTM: Method signature and variable declarations are correct.The method signature and variable declarations are consistent with the existing
TestVoteEventmethod. The use ofVoteWeightedMethodis appropriate for testing the new weighted voting functionality.precompiles/gov/abi.json (2)
Line range hint
68-260: LGTM! Consistent updates to support weighted voting.The updates to the
getVoteandgetVotesfunctions are well-implemented and necessary to support the new weighted voting feature. The changes maintain backwards compatibility by keeping the existing function names and input parameters while updating the output structures to include the newWeightedVoteOption[]andWeightedVote[]types.
261-306: LGTM! Consider adding input validation.The new
voteWeightedfunction is well-structured and aligns perfectly with the PR objectives and the newVoteWeightedevent. The use ofWeightedVoteOption[]allows for multiple weighted votes in a single transaction, providing flexibility for complex voting scenarios.Consider adding input validation for the
optionsparameter. For example, you could check that the sum of all weights equals 100% (or 1.0, depending on your weight representation) to ensure the vote is valid.To ensure consistent usage across the codebase, let's verify the implementation:
✅ Verification successful
LGTM! Consider adding input validation.
The
voteWeightedfunction is properly implemented inprecompiles/gov/IGov.soland align 10000 s with the PR objectives and the newVoteWeightedevent. The use ofWeightedVoteOption[]allows for multiple weighted votes in a single transaction, providing flexibility for complex voting scenarios.
- Implementation confirmed in
precompiles/gov/IGov.sol.Suggestion:
- Add input validation for the
optionsparameter. For example, ensure that the sum of all weights equals 100% (or 1.0, depending on your weight representation) to guarantee the vote's validity.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent implementation of voteWeighted function # Expected: Find implementations in Solidity and Go files echo "Searching for voteWeighted implementations:" rg --type go --type solidity 'func.*voteWeighted|function.*voteWeighted' echo "\nChecking for potential callers of voteWeighted:" rg --type go --type solidity 'voteWeighted\('Length of output: 394
Script:
#!/bin/bash # Description: Check for consistent implementation of voteWeighted function # Expected: Find implementations in Solidity (.sol) and Go (.go) files echo "Searching for voteWeighted implementations:" rg --glob '*.go' --glob '*.sol' 'func\s+voteWeighted|function\s+voteWeighted' echo "\nChecking for potential callers of voteWeighted:" rg --glob '*.go' --glob '*.sol' 'voteWeighted\('Length of output: 440
CHANGELOG.md (7)
56-57: Significant addition: VoteWeighted transaction to gov precompileThis change adds a new transaction type to the governance precompile, which could impact how voting is conducted on the platform. It's important to ensure this new feature is thoroughly tested and documented.
58-58: Performance improvement for eth_headerByHash RPC callThis optimization could lead to faster response times for clients querying block headers. It's a positive change that should improve overall system performance.
🧰 Tools
🪛 Markdownlint
58-58: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
65-66: EVM channel selector additionAdding a channel selector based on chain ID could improve routing and interoperability. This is a positive change that should enhance the system's flexibility.
🧰 Tools
🪛 Markdownlint
58-58: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
56-72: Summary of v16.0.0 changesThis version introduces substantial changes to the Evmos platform, including:
- Addition of new governance features (VoteWeighted transaction)
- Performance improvements
- Removal of several modules (recovery, incentives, claims)
- Significant economic changes (inflation reduction, burning of incentive pool)
- Refactoring of outposts and EVM channel selection
These changes represent a major evolution of the platform and will require careful testing, clear communication to users, and updates to documentation. Particular attention should be paid to the economic impacts and ensuring smooth transitions for users affected by removed modules.
Please ensure comprehensive testing of all changes, particularly:
- The new governance features
- The refactored outposts
- The new inflation rate
- The removal of incentives and other modules
Additionally, verify that all documentation has been updated to reflect these changes.
🧰 Tools
🪛 Markdownlint
58-58: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
68-69: Refactoring of outpostsThe refactoring of Stride and Osmosis outposts to hardcode the WEVMOS address could improve security and reduce potential points of failure. However, it also reduces flexibility.
Please confirm that:
- The hardcoded WEVMOS address is correct and consistent across all environments.
- There's a process in place to update this address if needed in the future.
#!/bin/bash # Check for hardcoded WEVMOS addresses grep -R "WEVMOS" ./x/erc20🧰 Tools
🪛 Markdownlint
58-58: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
70-71: Addition of feecollector Burner roleAdding a Burner role for the fee collector in the upgrade handler is a significant change to the fee management system.
Please ensure that:
- The Burner role has appropriate access controls.
- The burning mechanism is properly implemented and doesn't introduce any vulnerabilities.
#!/bin/bash # Check for Burner role implementation grep -R "Burner" ./x/auth🧰 Tools
🪛 Markdownlint
58-58: Expected: 120; Actual: 136
Line length(MD013, line-length)
59-59: Update to Interpreter interface for custom Opcodes supportThis change allows for more flexibility in the EVM by supporting custom opcodes. It's a powerful feature but should be implemented with caution to maintain compatibility and security.
Please ensure that:
- The custom opcodes don't introduce any security vulnerabilities.
- Backward compatibility is maintained for existing contracts.
- The gas costs for these custom opcodes are properly calibrated.
precompiles/gov/tx_test.go (3)
218-233: Ensure error message for invalid total weight is accurateIn the test case
"fail - invalid weight sum", the expected error message is"total weight overflow 1.00". Verify that this matches the actual error message returned by the system to ensure accurate test validation.
203-215: Validate the handling of invalid vote optionsIn the test case
"fail - invalid vote option", an option with value10is used. Confirm that10is indeed outside the valid range of vote options. Ensure that the error message"invalid vote option"is appropriate and aligns with the actual error produced.
235-257: Well-implemented test case for successful weighted votingThe test case
"success - vote weighted proposal"effectively validates weighted voting with correct option weights summing to1.0. The post-check assertions accurately verify the tally results against expected values.precompiles/gov/types.go (4)
27-32: Definition ofEventVoteWeightedstruct follows existing patternsThe
EventVoteWeightedstruct is appropriately defined to handle the event data for theVoteWeightedtransaction, maintaining consistency with existing event structures in the codebase.
65-67: Proper introduction ofWeightedVoteOptionstypeThe
WeightedVoteOptionstype is correctly introduced as a slice ofWeightedVoteOption, enhancing code readability and type safety.
135-139: Consistent handling ofmetadataargument validationThe validation for the
metadataargument is correctly implemented. Ensure that any constraints on themetadatafield (such as length or content restrictions) are enforced if applicable.
140-147: Construction ofMsgVoteWeightedmessage is correctThe
MsgVoteWeightedmessage is properly constructed with all necessary fields. The conversion of the voter's address and inclusion of weighted options align with the expected message format.
Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com>
Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
precompiles/gov/events_test.go (1)
95-167: Consider adding more test cases for comprehensive coverage.The current implementation only includes one test case for the
VoteWeightedevent. To ensure robust testing, consider adding more test cases that cover different scenarios, such as:
- Multiple voting options with different weights
- Edge cases (e.g., very small or large weights)
- Invalid inputs (e.g., weights that don't sum to 1)
- Error cases (e.g., incorrect proposal ID)
Here's an example of an additional test case you could add:
{ "error - invalid weights", func(voter common.Address, proposalId uint64, options gov.WeightedVoteOptions) []interface{} { return []interface{}{ voter, proposalId, gov.WeightedVoteOptions{ {Option: 1, Weight: "0.70"}, {Option: 2, Weight: "0.40"}, // Sum of weights > 1 }, "", } }, func() { // No post-check needed for error case }, 20000, true, "sum of weights must be 1.00", },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- precompiles/gov/events.go (2 hunks)
- precompiles/gov/events_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- precompiles/gov/events.go
🧰 Additional context used
🔇 Additional comments (2)
precompiles/gov/events_test.go (2)
88-167: LGTM! Well-structured test function for VoteWeighted event.The overall structure and logic of the
TestVoteWeightedEventfunction are well-implemented. It follows good testing practices such as:
- Using a table-driven test approach for easy addition of more test cases.
- Properly setting up the test environment and contract.
- Thorough checking of emitted event details, including address, signature, and unpacked data.
- Clear and descriptive variable names.
Good job on maintaining consistency with the existing
TestVoteEventfunction while adapting it for the newVoteWeightedfunctionality.
120-120:⚠️ Potential issueAddress potential integer overflow in log block number comparison.
The potential integer overflow issue flagged in a previous review is still present in this new function. Consider implementing the suggested fix to ensure safe conversion from
int64touint64.Apply the following fix:
blockHeight := ctx.BlockHeight() if blockHeight < 0 { s.T().Fatalf("Unexpected negative block height: %d", blockHeight) } s.Require().Equal(log.BlockNumber, uint64(blockHeight))Alternatively, you can use the suggestion from the previous review:
s.Require().Equal(log.BlockNumber, uint64(ctx.BlockHeight())) //nolint:gosec // G115
|
https://github.com/Mergifyio backport rama/release-v20 |
✅ Backports have been created
|
…(backport #2922) (#2940) * feat(gov): Add VoteWeighted transaction, unit and integration tests. (#2922) (cherry picked from commit ab3747f) * update chlog * make proto-all * make contracts-all --------- Co-authored-by: Vladislav Varadinov <vladislav.varadinov@gmail.com> Co-authored-by: tom <tomasguerraalda@hotmail.com>
Description
This adds the
VoteWeightedtransaction from the governance module to the governance precompile. Unit and integration tests are included.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...
Reviewers 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...
Unreleasedsection inCHANGELOG.mdSummary by CodeRabbit
New Features
VoteWeightedand methodvoteWeightedfor enhanced governance voting capabilities.Improvements
Bug Fixes
Tests