feat(gov-precompile): Added getDeposit, getDeposits and getTallyResults queries, unit and integration tests. #2941
Conversation
WalkthroughThe pull request enhances the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
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: 7
🧹 Outside diff range and nitpick comments (6)
precompiles/gov/gov.go (1)
114-119: LGTM! Consider grouping related methods for improved readability.The new methods (
GetDepositMethod,GetDepositsMethod, andGetTallyResultMethod) have been correctly implemented and align with the PR objectives. The implementation is consistent with the existing code structure and doesn't introduce any breaking changes.For improved readability, consider grouping related methods together. For example:
case GetVoteMethod: bz, err = p.GetVote(ctx, method, contract, args) case GetVotesMethod: bz, err = p.GetVotes(ctx, method, contract, args) case GetDepositMethod: bz, err = p.GetDeposit(ctx, method, contract, args) case GetDepositsMethod: bz, err = p.GetDeposits(ctx, method, contract, args) case GetTallyResultMethod: bz, err = p.GetTallyResult(ctx, method, contract, args)This grouping makes it easier to see all the "Get" methods together.
precompiles/gov/abi.json (2)
121-217: LGTM:getDepositsfunction implementation with a minor suggestionThe
getDepositsfunction is well-implemented with proper pagination support. It correctly uses theviewmodifier and returns both the deposits data and pagination information.Consider adding a comment or documentation to clarify the purpose of the
PageRequeststruct, especially for developers who might not be familiar with this pagination pattern. This could include a brief explanation of how to use the pagination parameters effectively.
218-258: LGTM:getTallyResultfunction implementation with optimization suggestionThe
getTallyResultfunction is well-defined and correctly uses theviewmodifier. It provides a clear way to retrieve tally results for a specific proposal.Consider using a more appropriate data type for vote counts in the
TallyResultDatastruct. Currently, the votes (yes, abstain, no, noWithVeto) are stored as strings. For numerical values, usinguint256or a similar integer type would be more efficient and easier to work with in calculations. This change would require updating the struct definition and any related functions. For example:struct TallyResultData { uint256 yes; uint256 abstain; uint256 no; uint256 noWithVeto; }This change would improve performance and reduce gas costs when working with vote counts.
precompiles/gov/integration_test.go (3)
366-435: Consider removing commented-out code and tracking TODOs separatelyThe large block of commented-out code for deposit queries (lines 366-435) clutters the file and may become outdated. Consider the following actions:
- Remove the commented-out code to improve readability.
- Create a separate GitHub issue to track the pending implementation of deposit queries.
- Add a short comment with the issue number for reference, e.g., "// TODO(#issueNumber): Implement deposit queries once precompile transaction is available"
This approach will keep the test file clean while ensuring the pending work is properly tracked.
Would you like me to help create a GitHub issue for tracking the deposit queries implementation?
🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 367-367:
commentFormatting: put a space between//and comment text (gocritic)
437-477: LGTM! Consider adding a comment for clarityThe new tally result query test is well-structured and comprehensive. It correctly sets up the preconditions by submitting a vote before querying the tally result, and the assertions check for the expected values.
To improve clarity, consider adding a brief comment explaining why the expected values are what they are. For example, before line 472:
// Expected values based on the single vote cast in BeforeEach Expect(out.TallyResult.Yes).To(Equal("3000000000000000000"))This will help future readers understand the test's expectations more easily.
367-367: Minor: Add space after comment slashesTo improve consistency and adhere to Go style conventions, add a space between the comment slashes and the comment text:
- // TODO: Can't add deposit manually without the precompile transaction available ? + // TODO: Can't add deposit manually without the precompile transaction available?Also, consider removing the space before the question mark at the end of the comment.
🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 367-367:
commentFormatting: put a space between//and comment text (gocritic)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- precompiles/gov/IGov.sol (2 hunks)
- precompiles/gov/abi.json (1 hunks)
- precompiles/gov/errors.go (1 hunks)
- precompiles/gov/gov.go (1 hunks)
- precompiles/gov/integration_test.go (1 hunks)
- precompiles/gov/query.go (2 hunks)
- precompiles/gov/query_test.go (2 hunks)
- precompiles/gov/setup_test.go (3 hunks)
- precompiles/gov/types.go (2 hunks)
🧰 Additional context used
🪛 GitHub Check: Run golangci-lint
precompiles/gov/integration_test.go
[failure] 367-367:
commentFormatting: put a space between//and comment text (gocritic)precompiles/gov/query_test.go
[failure] 236-236:
composites: github.com/evmos/evmos/v20/precompiles/common.Coin struct literal uses unkeyed fields (govet)precompiles/gov/setup_test.go
[failure] 47-47:
redefines-builtin-id: redefinition of the built-in type any (revive)precompiles/gov/types.go
[failure] 70-70:
var-naming: struct field ProposalId should be ProposalID (revive)
[failure] 81-81:
var-naming: struct field ProposalId should be ProposalID (revive)
[failure] 98-98:
var-naming: struct field ProposalId should be ProposalID (revive)
🔇 Additional comments (17)
precompiles/gov/errors.go (1)
26-27: LGTM: New error constant added correctly.The new
ErrInvalidDepositorconstant is well-implemented and consistent with the existing error message pattern in the file. It aligns with the PR objectives of enhancing governance-related functionality.precompiles/gov/setup_test.go (2)
7-9: LGTM: New imports are appropriate for the changes.The added imports for
math,types, andsdkfrom the Cosmos SDK are necessary for the modifications in theSetupTestmethod. They provide essential functionalities for handling mathematical operations, type management, and SDK features.
63-63: LGTM: Improved minimum deposit setup.The modification to set the minimum deposit using
sdk.NewCoins(sdk.NewCoin("aevmos", math.NewInt(100)))is a good improvement. This change:
- Utilizes the
mathpackage to create the deposit amount, ensuring precise handling of large integer values.- Provides more explicit and type-safe creation of the coin value.
- Aligns with best practices for handling currency amounts in the Cosmos SDK.
This update enhances the accuracy and reliability of the minimum deposit setup in the test environment.
precompiles/gov/gov.go (1)
Line range hint
1-153: Consider updating IsTransaction method and addressing integration test TODOThe implementation of the new methods looks great. However, there are two points to consider:
If any of the new methods (
GetDepositMethod,GetDepositsMethod,GetTallyResultMethod) are transactions rather than queries, update theIsTransactionmethod accordingly.Remember to address the TODO mentioned in the PR objectives regarding the integration tests, specifically the difficulty in implementing the
deposittransaction as a precompile transaction.To check if there are any TODOs related to integration tests, run:
Would you like assistance in addressing the integration test TODO or updating the
IsTransactionmethod if necessary?precompiles/gov/abi.json (2)
68-120: LGTM:getDepositfunction implementationThe
getDepositfunction is well-defined with appropriate input parameters and return type. It correctly uses theviewmodifier, indicating it doesn't modify the contract state. TheDepositDatastruct provides comprehensive information about the deposit, including the proposal ID, depositor address, and the amount as an array ofCoinstructs.
Line range hint
1-257: Overall assessment: Solid enhancements to the IGov contractThe additions to the
IGovcontract ABI significantly enhance its functionality by introducing methods to retrieve deposit information and tally results. These new functions (getDeposit,getDeposits, andgetTallyResult) are well-implemented and properly use theviewmodifier for read-only operations.The changes align well with the PR objectives, addressing the need for high-priority queries that were previously missing from the precompiles list. The new functions provide a comprehensive way to interact with governance-related data, which should improve the overall usability of the contract.
To ensure these changes are properly integrated, please run the following verification script:
This script will help ensure that the new functions are properly integrated into the existing codebase and that there are no conflicts or structural issues with the ABI.
✅ Verification successful
Verification Successful: No conflicts found in IGov ABI changes
All new functions (
getDeposit,getDeposits,getTallyResult) are properly integrated into theIGovcontract. No duplicate function signatures were found, and the ABI JSON structure is valid. The additions enhance the contract's functionality without introducing any conflicts or issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new functions are properly integrated and there are no conflicts with existing code. # Test: Check if the new functions are referenced in other parts of the codebase echo "Checking references to new functions:" rg --type solidity -e 'getDeposit' -e 'getDeposits' -e 'getTallyResult' # Test: Verify that there are no duplicate function signatures echo "Checking for duplicate function signatures:" jq '.abi | map(.name) | group_by(.) | map(select(length > 1))' precompiles/gov/abi.json # Test: Ensure that the ABI is valid JSON echo "Validating ABI JSON structure:" jq '.' precompiles/gov/abi.json > /dev/null && echo "ABI is valid JSON" || echo "ABI is not valid JSON"Length of output: 523
Script:
#!/bin/bash # Description: Verify that the new functions are properly integrated and there are no conflicts with existing code. # Test: Check if the new functions are referenced in other parts of the codebase echo "Checking references to new functions in .sol files:" rg --glob '*.sol' -e 'getDeposit' -e 'getDeposits' -e 'getTallyResult' # Test: Verify that there are no duplicate function signatures echo "Checking for duplicate function signatures:" jq '.abi | map(.name) | group_by(.) | map(select(length > 1))' precompiles/gov/abi.json # Test: Ensure that the ABI is valid JSON echo "Validating ABI JSON structure:" jq '.' precompiles/gov/abi.json > /dev/null && echo "ABI is valid JSON" || echo "ABI is not valid JSON"Length of output: 976
precompiles/gov/query.go (1)
18-23: Constants are named consistently with existing codeThe newly added constants
GetDepositMethod,GetDepositsMethod, andGetTallyResultMethodare consistent with the existing naming convention used for method names in this package, ensuring clarity and coherence.precompiles/gov/IGov.sol (5)
41-46: TheDepositDatastruct is appropriately defined.The struct accurately captures the necessary information about a deposit on a proposal, including the proposal ID, depositor's address, and the amount.
126-134: ThegetDepositfunction is correctly defined.The function provides a way to retrieve the deposit information for a specific depositor on a given proposal, which is essential for tracking individual contributions.
135-147: ThegetDepositsfunction is well-defined with pagination support.Incorporating pagination allows efficient retrieval of deposits when dealing with a large number of entries, enhancing scalability and performance.
148-153: ThegetTallyResultfunction is properly implemented.This function enables users to obtain the tally results of a proposal, which is critical for transparency and accountability in governance processes.
49-54:⚠️ Potential issueVerify the use of
stringdata types for vote counts inTallyResultData.The fields
yes,abstain,no, andnoWithVetoare defined asstring. If these fields represent numeric vote counts, consider using numeric types likeuint256oruint64for mathematical operations and to ensure type safety.Run the following script to check how vote counts are handled elsewhere in the codebase:
✅ Verification successful
Vote counts are appropriately using
stringdata types. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find how vote counts are defined and used in the codebase. # Search for definitions of TallyResultData and examine the data types. rg --type solidity 'struct TallyResultData' -A 5 # Search for other instances where tally results are defined or used. rg --type solidity 'tallyResult' -A 5Length of output: 154
Script:
#!/bin/bash # Description: Find how vote counts are defined and used in the codebase. # Search for definitions of TallyResultData and examine the data types. rg --glob '*.sol' 'struct TallyResultData' -A 5 # Search for other instances where tally results are defined or used. rg --glob '*.sol' 'tallyResult' -A 5Length of output: 646
Script:
#!/bin/bash # Description: Find usages of TallyResultData fields to determine if they are used in mathematical operations. # Search for usages of tallyResult.yes rg --glob '*.sol' 'tallyResult\.yes' # Search for usages of tallyResult.abstain rg --glob '*.sol' 'tallyResult\.abstain' # Search for usages of tallyResult.no rg --glob '*.sol' 'tallyResult\.no' # Search for usages of tallyResult.noWithVeto rg --glob '*.sol' 'tallyResult\.noWithVeto'Length of output: 165
precompiles/gov/types.go (2)
82-82: Use correct capitalization for 'ProposalID' field nameAs previously noted, the field
ProposalIdshould be renamed toProposalIDto follow Go naming conventions.Apply this diff to fix the naming:
type DepositsInput struct { - ProposalId uint64 + ProposalID uint64 Pagination query.PageRequest }
98-98: Use correct capitalization for 'ProposalID' field nameAs previously noted, the field
ProposalIdshould be renamed toProposalIDto follow Go naming conventions.Apply this diff to fix the naming:
type DepositData struct { - ProposalId uint64 `abi:"proposalId"` + ProposalID uint64 `abi:"proposalId"` Depositor common.Address `abi:"depositor"` Amount []cmn.Coin `abi:"amount"` }🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 98-98:
var-naming: struct field ProposalId should be ProposalID (revive)precompiles/gov/query_test.go (3)
179-243:TestGetDepositfunction is well-implementedThe
TestGetDepositfunction appropriately tests both valid and invalid scenarios for retrieving a deposit. The test cases are well-structured, and the use ofmalleatefunctions ensures clear setup for each case.🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 236-236:
composites: github.com/evmos/evmos/v20/precompiles/common.Coin struct literal uses unkeyed fields (govet)
245-302: Comprehensive testing inTestGetDepositsThe
TestGetDepositsfunction provides thorough coverage for fetching multiple deposits, including edge cases with invalid proposal IDs. The clarity in test case definitions enhances maintainability.
304-369: Effective validation inTestGetTallyResultThe
TestGetTallyResultfunction accurately tests the retrieval of tally results for proposals, covering both successful retrievals and error handling for non-existent proposals.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
precompiles/gov/setup_test.go (1)
62-66: LGTM with a minor suggestion for clarity.The changes are appropriate:
- Using
anyMessagein theMessagesfield is consistent with the earlier type-safe approach.- Updating the minimum deposit parameter with
math.NewInt(100)provides more precision.For improved readability, consider extracting the minimum deposit amount to a named constant:
const minDepositAmount = 100 govGen.Params.MinDeposit = sdk.NewCoins(sdk.NewCoin("aevmos", math.NewInt(minDepositAmount)))This change would make the code more self-documenting and easier to modify in the future.
precompiles/gov/integration_test.go (1)
456-477: LGTM: Tally result query test implementationThe test for querying tally results is well-implemented:
- It correctly calls the contract and checks the logs.
- The assertions verify the tally results for all vote options (Yes, Abstain, No, NoWithVeto).
- The expected "Yes" count (3000000000000000000) matches the voting power of the single vote cast in the setup.
Consider adding a comment explaining the expected "Yes" count value (3000000000000000000) to improve readability. For example:
// Expected "Yes" count is 3 EVMOS (3e18 in base units), which is the default voting power Expect(out.TallyResult.Yes).To(Equal("3000000000000000000"))precompiles/gov/types.go (3)
325-337: Enhance error messages for clarity in 'ParseTallyResultArgs'.The error messages in
ParseTallyResultArgscould be more informative. Providing detailed context in error messages helps with debugging.
283-304: Simplify argument parsing in 'ParseDepositArgs'.Consider using a structured input or consolidating type assertions to make the code more concise and maintainable.
307-321: Ensure consistent error handling in 'ParseDepositsArgs'.The function
ParseDepositsArgscould benefit from consistent error messages and handling similar to other parsing functions. This improves code readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- precompiles/gov/integration_test.go (1 hunks)
- precompiles/gov/query_test.go (2 hunks)
- precompiles/gov/setup_test.go (3 hunks)
- precompiles/gov/types.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (13)
precompiles/gov/setup_test.go (2)
7-10: LGTM: New imports are appropriate for the changes.The added imports for
math,types, andsdkfrom Cosmos SDK are necessary and align with the modifications in theSetupTestmethod.
45-50: LGTM: Improved type safety and addressed past comment.The changes enhance type safety by wrapping the test proposal in an
Anytype. The error handling is appropriate, and the variable nameanyMessageeffectively addresses the past comment about shadowing theanybuilt-in type.precompiles/gov/query_test.go (6)
4-35: LGTM: New imports and variables look goodThe new imports, variables, and the
getTestProposal()function are well-structured and necessary for the added test cases. They provide the required setup for testing governance-related queries.
180-243: LGTM: TestGetDeposit function is well-structuredThe TestGetDeposit function provides good coverage for both successful and error scenarios. It properly uses the PrecompileTestSuite for setup and assertions, and the malleate function effectively sets up the test state.
245-302: LGTM: TestGetDeposits function is well-implementedThe TestGetDeposits function provides comprehensive coverage for both successful and error scenarios. It effectively uses the PrecompileTestSuite for setup and assertions, and the malleate function properly sets up the test state. The assertions correctly check for the expected deposits and page response.
304-369: LGTM: TestGetTallyResult function is well-designedThe TestGetTallyResult function provides thorough coverage for both successful and error scenarios. It effectively uses the PrecompileTestSuite for setup and assertions. The malleate function comprehensively sets up the test state by submitting a proposal, adding a deposit, and casting a vote. The assertions correctly verify the expected tally result.
Line range hint
1-369: Overall, excellent additions to the governance query testsThe new test functions (TestGetDeposit, TestGetDeposits, and TestGetTallyResult) are well-structured and provide comprehensive coverage for the new governance query methods. They effectively use the PrecompileTestSuite for setup and assertions, and the test cases cover both successful and error scenarios. The only minor issue found was the use of unkeyed fields in a struct literal, which has been addressed in a separate comment.
These additions significantly enhance the test coverage for the governance precompile and will help ensure the reliability of the new query methods.
236-236:⚠️ Potential issueUse keyed fields in struct literal for
cmn.CoinAt line 236, the
cmn.Coinstruct is initialized using unkeyed fields. This can lead to maintenance issues if the struct's definition changes in the future. It's recommended to use field names when initializing structs for better readability and safety.Apply this diff to fix the issue:
-s.Require().Equal([]cmn.Coin{{Denom: "aevmos", Amount: big.NewInt(100)}}, out.Deposit.Amount) +s.Require().Equal([]cmn.Coin{{Denom: "aevmos", Amount: big.NewInt(100)}}, out.Deposit.Amount)precompiles/gov/integration_test.go (2)
437-454: LGTM: Tally result query test setupThe setup for the tally result query test is well-structured:
- It correctly submits a vote before querying the tally results.
- The use of
Expectstatements ensures that the vote is submitted successfully and a new block is created.This setup provides a good foundation for testing the tally result query functionality.
Line range hint
1-477: Summary of changes and suggestions
- The new tests for tally result queries are well-implemented and provide good coverage for this functionality.
- The deposit functionality is not yet implemented, as indicated by the commented-out tests and TODO comment.
Main points:
- Address the TODO for deposit functionality implementation.
- Once deposit functionality is implemented, uncomment and finalize the deposit tests.
- Consider adding a clarifying comment for the expected "Yes" count in the tally result test.
Overall, the changes improve the test coverage for the governance precompile, focusing on tally result queries. The next step would be to implement the deposit functionality to complete the feature set.
precompiles/gov/types.go (3)
70-70: Use consistent capitalization for 'ProposalID'.According to Go naming conventions, acronyms should be capitalized consistently. The field
ProposalIdshould be renamed toProposalIDin bothDepositInputandDepositDatastructs.Also applies to: 98-98
340-343: Handle errors properly in 'FromResponse' methods.In
DepositOutput.FromResponseandDepositsOutput.FromResponse, if address conversion fails, the methods returnnilwithout any error information. This can make debugging difficult. Consider modifying the function signatures to return an error and handle it appropriately.Also applies to: 362-365
351-355: Refactor duplicate code in 'FromResponse' methods to eliminate redundancy.The
FromResponsemethods forDepositOutputandDepositsOutputshare similar code for converting deposits. Refactor this logic into a shared helper function to improve maintainability and reduce code duplication.Also applies to: 373-377
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
precompiles/gov/query_test.go (3)
180-238: Enhance test cases for TestGetDepositWhile the structure of the test function is good, there are a few areas that could be improved:
- The
malleatefunction is empty for both test cases. Consider adding logic to create a deposit for the success case.- The success case doesn't actually verify that a deposit exists before querying it. This might lead to false positives.
- Add more test cases to cover different scenarios, such as:
- A proposal with multiple deposits
- A proposal with no deposits
- Edge cases for proposal IDs
Enhancing these areas will provide more robust testing for the GetDeposit functionality.
240-293: LGTM: TestGetDeposits looks good with minor suggestionsThe TestGetDeposits function is well-structured and covers important scenarios. The use of malleate to create deposits and the testing of pagination are particularly good. To further improve the test coverage, consider:
- Adding a test case for a proposal with multiple deposits to ensure proper handling of multiple results.
- Testing edge cases for pagination, such as requesting a page beyond available results.
- Verifying the contents of the returned deposits more thoroughly, not just the length.
These additions would make the tests even more robust.
295-360: LGTM: TestGetTallyResult is comprehensive with room for minor enhancementsThe TestGetTallyResult function is well-implemented and covers crucial scenarios. The malleate function's comprehensive setup, including proposal creation, deposit, and voting, is particularly commendable. To further strengthen the tests:
- Consider adding a test case for a proposal in different stages (e.g., deposit period, voting period, passed, rejected) to ensure correct tally results in various states.
- Add a test case with multiple votes from different addresses to verify correct tallying of multiple votes.
- Verify that the tally results match the expected values based on the voting power of the accounts used in the test.
These additions would provide even more thorough coverage of the GetTallyResult functionality.
precompiles/gov/integration_test.go (1)
Line range hint
1-478: Overall improvements for governance precompile integration testsThe additions to the integration tests for the governance precompile are valuable and well-structured. However, there are several areas where the tests can be improved:
- Address the TODO comment about adding deposits manually (line 367).
- Enhance the deposit and deposits query tests:
- Ensure proper setup of deposits before querying.
- Test multiple deposits and pagination scenarios.
- Improve the tally result query test:
- Cover multiple voting scenarios (Yes, No, Abstain, NoWithVeto).
- Make expected results dynamic based on votes cast.
- Consider making hardcoded values (like deposit amounts and vote counts) more flexible and easily configurable for different test scenarios.
These improvements will make the tests more robust, comprehensive, and easier to maintain. They will also provide better coverage of edge cases and real-world scenarios for the governance precompile functionality.
As you continue to develop these tests, consider creating helper functions for common setup tasks (like creating deposits or casting votes) to reduce code duplication and improve readability. This will make it easier to add new test scenarios in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- precompiles/gov/integration_test.go (2 hunks)
- precompiles/gov/query_test.go (2 hunks)
- precompiles/gov/setup_test.go (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- precompiles/gov/setup_test.go
🧰 Additional context used
🔇 Additional comments (2)
precompiles/gov/query_test.go (2)
4-35: LGTM: New imports and variables look goodThe new imports, variables, and the
getTestProposal()function are well-structured and necessary for the added test cases. They provide the required setup for testing governance-related queries.
Line range hint
1-360: Overall assessment: Good additions with room for enhancementThe new test functions (TestGetDeposit, TestGetDeposits, and TestGetTallyResult) significantly improve the test coverage for governance-related queries. They are well-structured and cover essential scenarios. However, there's room for enhancement:
- Increase the variety of test cases to cover more edge cases and scenarios.
- Ensure that setup functions (malleate) create necessary data for all test cases.
- Add more thorough verification of returned data in success cases.
These improvements will further strengthen the robustness of the test suite. Great job on expanding the test coverage!
|
https://github.com/Mergifyio backport rama/release-v20 |
✅ Backports have been created
|
Signed-off-by: Tom <[email protected]>
… into Vvaradinov/gov-queries
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (12)
precompiles/gov/query_test.go (1)
176-234: LGTM with suggestion: TestGetDeposit function is well-structured.The TestGetDeposit function follows the established pattern and includes appropriate test cases. However, consider enhancing the
malleatefunction in the "valid query" test case to set up the necessary state (e.g., creating a proposal and adding a deposit) to make the test more robust.Consider updating the "valid query" test case's
malleatefunction:malleate: func() { proposal, err := s.network.App.GovKeeper.SubmitProposal(s.network.GetContext(), TestProposalMsgs, "", "Test Proposal", "description", depositor, false) s.Require().NoError(err) _, err = s.network.App.GovKeeper.AddDeposit(s.network.GetContext(), proposal.Id, depositor, sdk.NewCoins(sdk.NewCoin(s.network.GetDenom(), math.NewInt(100)))) s.Require().NoError(err) },precompiles/gov/abi.json (2)
121-217: LGTM: getDeposits function implementation with a suggestionThe
getDepositsfunction is well-implemented and aligns with the PR objectives. The use of pagination is a good practice for handling potentially large datasets. ThePageRequestandPageResponsestructs provide necessary information for paginated results.However, I have a minor suggestion:
Consider adding a comment or documentation string to explain the purpose and usage of the
PageRequestandPageResponsestructs. This would improve the clarity and maintainability of the code, especially for developers who might not be familiar with these structures.
218-258: LGTM: getTallyResult function implementation with a suggestion for improvementThe
getTallyResultfunction is well-structured and aligns with the PR objectives. TheTallyResultDatastruct provides a comprehensive representation of the tally results, including yes, abstain, no, and noWithVeto votes.However, I have a suggestion for improvement:
Consider using a more appropriate data type for vote counts in the
TallyResultDatastruct. Currently, the vote counts are represented as strings, which might be problematic for large numbers or decimal values. Using a fixed-point decimal type or a custom struct that can handle large integers would be more suitable for vote counts. This change would improve the accuracy and efficiency of vote tallying and comparison operations.CHANGELOG.md (9)
83-83: Consider breaking long line for better readabilityThe line describing the inflation reduction is quite long (135 characters). Consider breaking it into multiple lines for better readability.
You could format it like this:
- (inflation) [#2137](https://github.com/evmos/evmos/pull/2137) Reduce inflation by 2/3. This change significantly alters the token emission rate.🧰 Tools
🪛 Markdownlint
83-83: Expected: 120; Actual: 135
Line length(MD013, line-length)
85-86: Clarify the impact of hardcoding WEVMOS addressThe changelog entry for the Stride and Osmosis outposts refactor is a bit vague. It would be helpful to explain the reasoning behind this change and its potential impact on users or developers.
Consider expanding the entry to provide more context:
- (outposts) [#2185](https://github.com/evmos/evmos/pull/2185) Refactor `Stride` and `Osmosis` outposts to hardcode WEVMOS address. This change improves consistency and reduces potential errors in address handling for these outposts.
Line range hint
87-88: Provide more details on the Burner role additionThe entry about adding the
feecollectorBurner role in the upgrade handler could benefit from more explanation about its purpose and impact.Consider expanding this entry to clarify its significance:
- (upgrade) [#2186](https://github.com/evmos/evmos/pull/2186) Add `feecollector` Burner role in upgrade handler. This change enables automatic burning of collected fees, contributing to the deflationary mechanism of the token economy.🧰 Tools
🪛 Markdownlint
80-80: Expected: 120; Actual: 160
Line length(MD013, line-length)
82-82: Expected: 120; Actual: 135
Line length(MD013, line-length)
83-83: Expected: 120; Actual: 135
Line length(MD013, line-length)
Line range hint
89-89: Highlight the significance of burning incentives pool balanceThe entry about burning the usage incentives pool balance during the v16 upgrade is important and could be emphasized more.
Consider rephrasing this entry to highlight its importance:
- (incentives) [#2221](https://github.com/evmos/evmos/pull/2221) Implement burning of the usage incentives pool balance during v16 upgrade. This is a significant change that affects the token supply and should be noted by validators and token holders.🧰 Tools
🪛 Markdownlint
80-80: Expected: 120; Actual: 160
Line length(MD013, line-length)
82-82: Expected: 120; Actual: 135
Line length(MD013, line-length)
83-83: Expected: 120; Actual: 135
Line length(MD013, line-length)
Line range hint
93-94: Clarify the impact of renaming the inflation moduleThe API Breaking change regarding the renaming of the
inflationmodule could use more context about its impact on developers and users.Consider expanding this entry:
- (inflation) [#2015](https://github.com/evmos/evmos/pull/2015) Rename `inflation` module to `inflation/v1`. This change may require updates to queries and transactions that interact with the inflation module. Developers should update their code accordingly.🧰 Tools
🪛 Markdownlint
80-80: Expected: 120; Actual: 160
Line length(MD013, line-length)
82-82: Expected: 120; Actual: 135
Line length(MD013, line-length)
83-83: Expected: 120; Actual: 135
Line length(MD013, line-length)
Line range hint
95-95: Explain the implications of deprecating legacy EIP-712 ante handlerThe entry about deprecating the legacy EIP-712 ante handler could benefit from more explanation about its impact and any migration steps required.
Consider expanding this entry:
- (ante) [#2078](https://github.com/evmos/evmos/pull/2078) Deprecate legacy EIP-712 ante handler. Users and developers should migrate to the new EIP-712 implementation. This change may affect existing transactions that rely on the legacy handler.🧰 Tools
🪛 Markdownlint
80-80: Expected: 120; Actual: 160
Line length(MD013, line-length)
82-82: Expected: 120; Actual: 135
Line length(MD013, line-length)
83-83: Expected: 120; Actual: 135
Line length(MD013, line-length)
Line range hint
99-100: Clarify the impact of linting testsThe entry about linting tests could benefit from explaining the benefits of this change.
Consider expanding this entry:
- (tests) [#1194](https://github.com/evmos/evmos/pull/1194) Lint tests to ensure consistency with non-test code. This improvement enhances code quality and maintainability across the codebase.🧰 Tools
🪛 Markdownlint
80-80: Expected: 120; Actual: 160
Line length(MD013, line-length)
82-82: Expected: 120; Actual: 135
Line length(MD013, line-length)
83-83: Expected: 120; Actual: 135
Line length(MD013, line-length)
Line range hint
101-101: Highlight the importance of the vulnerability checkerThe addition of a Golang dependency vulnerability checker is significant for security and could be emphasized more.
Consider rephrasing this entry:
- (ci) [#1138](https://github.com/evmos/evmos/pull/1138) Implement Golang dependency vulnerability checker in CI pipeline. This enhancement improves the overall security of the project by identifying potential vulnerabilities early in the development process.🧰 Tools
🪛 Markdownlint
80-80: Expected: 120; Actual: 160
Line length(MD013, line-length)
82-82: Expected: 120; Actual: 135
Line length(MD013, line-length)
83-83: Expected: 120; Actual: 135
Line length(MD013, line-length)
Line range hint
102-103: Explain the benefits of adding the File store listenerThe entry about adding the default File store listener could use more context about its purpose and benefits.
Consider expanding this entry:
- (app) [#1114](https://github.com/evmos/evmos/pull/1114) Add default File store listener for application, implementing [ADR38](https://docs.cosmos.network/v0.47/architecture/adr-038-state-listening). This improvement enables better state tracking and can be used for various purposes such as indexing or real-time data processing.🧰 Tools
🪛 Markdownlint
80-80: Expected: 120; Actual: 160
Line length(MD013, line-length)
82-82: Expected: 120; Actual: 135
Line length(MD013, line-length)
83-83: Expected: 120; Actual: 135
Line length(MD013, line-length)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- CHANGELOG.md (1 hunks)
- precompiles/gov/IGov.sol (2 hunks)
- precompiles/gov/abi.json (2 hunks)
- precompiles/gov/integration_test.go (2 hunks)
- precompiles/gov/query_test.go (2 hunks)
- precompiles/gov/setup_test.go (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- precompiles/gov/IGov.sol
🧰 Additional context used
🪛 Markdownlint
CHANGELOG.md
83-83: Expected: 120; Actual: 135
Line length(MD013, line-length)
🔇 Additional comments (13)
precompiles/gov/setup_test.go (5)
7-13: LGTM: New imports are appropriate for the changes.The added imports from the Cosmos SDK are necessary for the new functionality in the test setup. They enhance the capabilities of the test suite, allowing for more comprehensive testing of the governance precompile.
49-54: LGTM: Improved type safety and error handling.The changes enhance type safety by wrapping the test proposal message in an
Anytype. The error handling forNewAnyWithValueis properly implemented. It's good to see that the previous issue of shadowing the built-inanytype has been addressed by using the nameanyMessage.
66-66: LGTM: Consistent use of type-safe message handling.The
Messagesfield of the proposal now correctly uses theanyMessagevariable, which is consistent with the earlier type-safe approach for handling proposal messages.
69-73: LGTM: Appropriate bank genesis state configuration.The new bank genesis state configuration correctly sets up an initial balance for the governance module's address. The use of
math.NewInt(100)ensures precise handling of the coin amount. This setup is suitable for testing governance-related functionality.
75-80: LGTM: Governance genesis state properly configured with deposit record.The governance genesis state is well-configured with a new deposit entry and the
MinDepositparameter. These changes align with the bank balance setup and provide a good foundation for testing governance queries. It's great to see that the suggestion from the previous review about adding a deposit record has been implemented.precompiles/gov/query_test.go (4)
4-15: LGTM: New imports are appropriate for the added functionality.The new imports, including
math/big,cmn, and various Cosmos SDK types, are necessary for the new test functions and appear to be correctly utilized throughout the file.
23-31: LGTM: New variables are well-defined for testing purposes.The
govAcctvariable correctly represents the governance module address, andTestProposalMsgsprovides a suitable test message for proposal-related tests. These additions enhance the test suite's capabilities.
236-356: LGTM: TestGetDeposits and TestGetTallyResult functions are well-implemented.Both functions follow the established pattern and include comprehensive test cases. The malleate functions properly set up the test state, which is an improvement over the TestGetDeposit function. These additions enhance the test coverage for the governance precompile.
Line range hint
1-356: LGTM: Comprehensive test coverage added for governance queries.The changes in this file significantly enhance the test suite for the governance precompile. The new test functions (TestGetDeposit, TestGetDeposits, and TestGetTallyResult) are well-structured and cover important scenarios. The additions are consistent with the existing code style and testing patterns.
Minor suggestion:
- Consider enhancing the
malleatefunction in the TestGetDeposit "valid query" test case, as mentioned in a previous comment.Overall, these changes improve the robustness of the governance precompile tests.
precompiles/gov/abi.json (2)
68-120: LGTM: getDeposit function implementationThe
getDepositfunction is well-structured and aligns with the PR objectives. It correctly usesuint64for theproposalId, which is consistent with other functions in the ABI. TheDepositDatastruct provides a comprehensive representation of a deposit, including the proposal ID, depositor address, and an array ofCoinstructs for the deposit amount.
Line range hint
1-258: Overall assessment: ABI enhancements are well-implementedThe additions to the
IGovcontract ABI are well-structured and align with the PR objectives. The new functions (getDeposit,getDeposits, andgetTallyResult) enhance the governance capabilities as intended. The use of appropriate data structures and pagination demonstrates good design practices.A few minor suggestions have been made to further improve the implementation:
- Consider adding documentation for the
PageRequestandPageResponsestructs in thegetDepositsfunction.- Evaluate the use of a more suitable data type for vote counts in the
TallyResultDatastruct.These changes will contribute to better clarity, maintainability, and accuracy of the governance precompile.
precompiles/gov/integration_test.go (1)
367-477: Overall assessment of new integration testsThe addition of integration tests for deposit query, deposits query, and tally result query is a positive improvement to the codebase. These tests help ensure the correct functionality of the governance precompile. However, there are some common themes for improvement across all three new contexts:
- Increase test coverage by including more diverse scenarios (e.g., multiple deposits, various voting options).
- Enhance setup procedures to ensure consistent and reliable test environments.
- Make expected values more dynamic and less hardcoded to improve test flexibility and maintainability.
- Utilize pagination testing where applicable to ensure proper handling of large data sets.
Implementing these suggestions will significantly improve the robustness and reliability of the integration tests, providing better coverage of the governance precompile functionality.
CHANGELOG.md (1)
Line range hint
1-265: Overall, the changelog is informative but could benefit from some enhancementsThe CHANGELOG.md file for v16.0.0 provides a comprehensive list of changes, categorized into State Machine Breaking, API Breaking, Improvements, and Bug Fixes. This structure is helpful for users and developers to understand the impact of the update.
However, some entries could benefit from additional context or explanation, particularly regarding their impact on users, developers, or the overall system. Adding more details to key changes would make the changelog even more valuable.
Consider reviewing the suggested improvements in the previous comments to enhance the clarity and usefulness of the changelog. This will help users and developers better understand the significance of each change and any actions they may need to take when upgrading to v16.0.0.
🧰 Tools
🪛 Markdownlint
80-80: Expected: 120; Actual: 160
Line length(MD013, line-length)
82-82: Expected: 120; Actual: 135
Line length(MD013, line-length)
83-83: Expected: 120; Actual: 135
Line length(MD013, line-length)
…ts queries, unit and integration tests. (#2941) * feat: add getDeposit, getDeposits and getTallyResults * feat: add the queries, types, unit and integration tests * run make format * format and lint * tests: address comments from code review * fix: json linter * Update precompiles/gov/integration_test.go Signed-off-by: Tom <[email protected]> * use const for denom in test * chore: add changelog entry * Update precompiles/gov/IGov.sol Co-authored-by: stepit <[email protected]> Signed-off-by: Guillermo Paoletti <[email protected]> * refactor prop msgs --------- Signed-off-by: Tom <[email protected]> Signed-off-by: Guillermo Paoletti <[email protected]> Co-authored-by: Vvaradinov <[email protected]> Co-authored-by: hanchon <[email protected]> Co-authored-by: Tom <[email protected]> Co-authored-by: tom <[email protected]> Co-authored-by: stepit <[email protected]> (cherry picked from commit cb53cf0) # Conflicts: # .golangci.yml # CHANGELOG.md
…ts queries, unit and integration tests. (backport #2941) (#2947) * feat(gov-precompile): Added getDeposit, getDeposits and getTallyResults queries, unit and integration tests. (#2941) * feat: add getDeposit, getDeposits and getTallyResults * feat: add the queries, types, unit and integration tests * run make format * format and lint * tests: address comments from code review * fix: json linter * Update precompiles/gov/integration_test.go Signed-off-by: Tom <[email protected]> * use const for denom in test * chore: add changelog entry * Update precompiles/gov/IGov.sol Co-authored-by: stepit <[email protected]> Signed-off-by: Guillermo Paoletti <[email protected]> * refactor prop msgs --------- Signed-off-by: Tom <[email protected]> Signed-off-by: Guillermo Paoletti <[email protected]> Co-authored-by: Vvaradinov <[email protected]> Co-authored-by: hanchon <[email protected]> Co-authored-by: Tom <[email protected]> Co-authored-by: tom <[email protected]> Co-authored-by: stepit <[email protected]> (cherry picked from commit cb53cf0) # Conflicts: # .golangci.yml # CHANGELOG.md * fix conflicts * fix import --------- Co-authored-by: Vladislav Varadinov <[email protected]> Co-authored-by: tom <[email protected]>
Description
This PR adds the 3 high priority queries from the list of missing precompiles. There is one TODO left that I couldn't get to work in the integration tests without implementing the
deposittransaction as a precompile transaction. Maybe there is a workaround ?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
Bug Fixes
Tests
Documentation