feat: add tally result querier #266
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughWalkthroughThe changes involve modifications to the governance module, specifically within the Changes
Possibly related PRs
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 (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range and nitpick comments (6)
proto/initia/gov/v1/query.proto (1)
107-120: LGTM: New message types for TallyResult queryThe new message types
QueryTallyResultRequestandQueryTallyResultResponseare well-defined and consistent with the PR objectives. The use ofuint64forproposal_idand the options set on thetally_resultfield are appropriate.Consider adding a comment to explain the purpose of the
amino.dont_omitemptyoption on thetally_resultfield, as it might not be immediately clear to all developers why this is necessary.x/gov/keeper/tally_test.go (2)
178-179: LGTM! Consider adding a comment for clarity.The changes correctly reflect the new structure of
tallyResults, accessing the counts throughV1TallyResult. This is consistent with the restructuring mentioned in the PR summary.Consider adding a brief comment explaining the
V1TallyResultstructure for better clarity:// Accessing counts through V1TallyResult due to restructuring of tally results require.Equal(t, tallyResults.V1TallyResult.YesCount, math.LegacyNewDec(1_500_000+100_000_000).TruncateInt().String()) require.Equal(t, tallyResults.V1TallyResult.NoCount, math.LegacyNewDec(100_000_000).TruncateInt().String())
Remaining References to
tallyResultsFoundThe restructuring of
tallyResultsis not fully applied. The following files still reference the oldtallyResultsstructure:
x/gov/abci.go
- Lines related to
handleTallyResult- Direct assignments to
proposal.FinalTallyResultx/gov/keeper/tally_test.go
- Assertions on
tallyResults.V1TallyResult.YesCountandNoCountPlease update these references to align with the new
tallyResultsstructure to ensure consistency and prevent potential issues.Analysis chain
Line range hint
1-179: Comprehensive test coverage maintained with minimal changes.The changes to
Test_Tallyare minimal and don't affect the overall structure or logic of the tests. The test coverage remains thorough, including various scenarios such as regular votes and vesting votes. The restructuring oftallyResultsis correctly reflected in the final assertions.To ensure that all necessary changes have been made, please run the following script to check for any remaining references to the old
tallyResultsstructure:This will help verify that the restructuring has been consistently applied across the codebase.
Line range hint
1-179: Further verification required fortallyResultsrestructuring.The previous script did not produce any output, which might indicate that there are no remaining references to
YesCountorNoCount. To ensure that the restructuring oftallyResultshas been consistently applied across the codebase, please run the following additional script:This will help identify any lingering references or usages of
tallyResultsthat may need to be addressed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old tallyResults structure # Test: Search for direct access to YesCount or NoCount on tallyResults rg --type go 'tallyResults\.(YesCount|NoCount)' x/gov/Length of output: 56
Script:
#!/bin/bash # Description: Check for any remaining references to the tallyResults structure # Broader search for any usage of tallyResults rg --type go 'tallyResults' x/gov/ --context 2Length of output: 7105
x/gov/abci.go (1)
Line range hint
223-336: Summary: Introduction of custom TallyResult typeThe changes in this file are part of the larger feature addition described in the PR objectives. The introduction of
customtypes.TallyResultaligns with the newTallyResultmessage structure mentioned in the PR summary. These modifications appear to be setting the groundwork for handling the new fields liketally_height,total_staking_power, andtotal_vesting_power.A few points to consider:
- Ensure that the new
customtypes.TallyResultincludes all the fields mentioned in the PR summary (tally_height,total_staking_power,total_vesting_power, andv1_tally_result).- Verify that the quorum calculation logic (summing
total_staking_powerandtotal_vesting_power) is implemented correctly, possibly in theTallyfunction called earlier in this file.- Update any documentation or comments related to the
handleTallyResultfunction to reflect the usage of the new custom type.Consider adding a comment above the
handleTallyResultfunction explaining the structure and purpose of the newcustomtypes.TallyResult, especially highlighting how it differs from the standardv1.TallyResult.x/gov/keeper/custom_grpc_query_test.go (2)
191-191: Usectx.BlockTime()instead oftime.Now().UTC()for consistent test timingUsing
ctx.BlockTime()ensures that the test remains consistent and does not depend on the actual current time. Replacetime.Now().UTC()withctx.BlockTime()to utilize the context's block time.Apply this diff to use the context's block time:
-ctx = ctx.WithBlockTime(time.Now().UTC().Add(time.Minute * 15)) +ctx = ctx.WithBlockTime(ctx.BlockTime().Add(time.Minute * 15))
199-200: Clarify the magic numbers used in tally result comparisonsThe use of magic numbers like
1_500_000and100_000_000may reduce code clarity. Consider defining constants or adding comments to explain the significance of these values in the context of the test.For example:
const ( vestingVotePower = 1_500_000 validatorVotePower = 100_000_000 ) // Then use these constants in your assertions require.Equal(t, tallyResults.V1TallyResult.YesCount, math.LegacyNewDec(vestingVotePower + validatorVotePower).TruncateInt().String()) require.Equal(t, tallyResults.V1TallyResult.NoCount, math.LegacyNewDec(validatorVotePower).TruncateInt().String())
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
client/docs/swagger-ui/swagger.yamlis excluded by!**/*.yamlx/gov/types/gov.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/gov/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/gov/types/query.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.go
Files selected for processing (9)
- proto/initia/gov/v1/gov.proto (7 hunks)
- proto/initia/gov/v1/query.proto (4 hunks)
- x/gov/abci.go (2 hunks)
- x/gov/keeper/custom_grpc_query.go (1 hunks)
- x/gov/keeper/custom_grpc_query_test.go (2 hunks)
- x/gov/keeper/grpc_query.go (1 hunks)
- x/gov/keeper/tally.go (5 hunks)
- x/gov/keeper/tally_test.go (1 hunks)
- x/gov/types/proposal.go (4 hunks)
Additional context used
buf
proto/initia/gov/v1/gov.proto
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
proto/initia/gov/v1/query.proto
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
Additional comments not posted (20)
proto/initia/gov/v1/query.proto (2)
36-39: LGTM: New TallyResult RPC methodThe addition of the
TallyResultRPC method aligns well with the PR objectives. The method signature and HTTP endpoint are appropriately defined for querying the tally of a proposal vote.
Line range hint
1-120: Overall assessment: Well-implemented changes for tally result queryingThe changes to this file successfully implement the new tally result querying functionality as outlined in the PR objectives. The new RPC method and associated message types are well-defined and consistent. Minor suggestions for improvement have been made, but overall, the implementation is solid.
x/gov/abci.go (2)
336-336: LGTM: Simplified assignment ofFinalTallyResultThe direct assignment of
tallyResultstoproposal.FinalTallyResultis a clean simplification. This change suggests that thecustomtypes.TallyResultis now directly compatible with theFinalTallyResultfield of the proposal.
223-223: Verify compatibility ofcustomtypes.TallyResultThe change from
v1.TallyResulttocustomtypes.TallyResultis noted. This shift to a custom type could provide more flexibility or additional fields specific to your project's needs.Please ensure that
customtypes.TallyResultmaintains compatibility with any external systems or modules that may depend on the standardv1.TallyResultstructure. Run the following script to verify the structure of the custom type:Verification successful
Compatibility of
customtypes.TallyResultVerifiedThe structure of
customtypes.TallyResultincludes a reference tov1.TallyResult, ensuring that compatibility with existing systems and modules is maintained. The additional fields provide extended functionality without disrupting the original type's integrity.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the structure of customtypes.TallyResult # Test: Check the definition of customtypes.TallyResult rg --type go -A 10 'type TallyResult struct' x/gov/typesLength of output: 1122
x/gov/types/proposal.go (4)
8-8: Import the necessary package for math operationsThe
cosmossdk.io/mathpackage is appropriately imported to utilizemath.ZeroInt()in theEmptyTallyResult()function.
28-28: InitializeFinalTallyResultusingEmptyTallyResult()The
FinalTallyResultis now initialized using theEmptyTallyResult()function, which provides a structuredTallyResultwith zero values. This promotes code consistency and reuse.
65-65: UpdateFinalTallyResultassignment inToV1()methodIn the
ToV1()method,FinalTallyResultis correctly assigned usingp.FinalTallyResult.V1TallyResultto ensure proper conversion to the v1Proposaltype.
145-155: AddEmptyTallyResult()function for consistent initializationThe
EmptyTallyResult()function provides a standardized way to initializeTallyResultwith zero values, enhancing code reuse and maintainability.x/gov/keeper/custom_grpc_query_test.go (4)
129-129: Verify that passingnilas proposal content is appropriatePassing
nilas the proposal content inSubmitProposalmay lead to unexpected behavior if the content is required. Please ensure thatnilis acceptable in this context, or provide valid proposal content to avoid potential issues.
181-189: EnsurevestingVoteris properly initialized
vestingVoteris set toaddrs[1], but it's not clear whereaddrsis defined or initialized. Verify thatvestingVoteris correctly initialized to prevent potentialnilpointer dereferences or index out-of-range errors.
192-194: Verify the necessity of usingcacheCtxfor the Tally operationThe use of
cacheCtxin theTallyoperation may not be necessary if the state modifications do not need to be discarded. Ensure thatcacheCtxis required for your test scenario; otherwise, consider usingctxdirectly to avoid unnecessary complexity.
202-205: Ensure the tally result from the query matches expected valuesWhile comparing
tallyResultswithres.TallyResult, ensure that all fields (includingTotalStakingPowerandTotalVestingPower) are as expected. This can help catch any discrepancies in the tally calculation.You can enhance the assertion:
require.Equal(t, tallyResults.V1TallyResult, res.TallyResult.V1TallyResult) require.Equal(t, tallyResults.TotalStakingPower, res.TallyResult.TotalStakingPower) require.Equal(t, tallyResults.TotalVestingPower, res.TallyResult.TotalVestingPower)proto/initia/gov/v1/gov.proto (2)
169-172: Update handling offinal_tally_resultinProposalThe
Proposalmessage now uses the newTallyResultmessage forfinal_tally_result. Ensure that all logic related to proposal handling, especially when accessing or modifyingfinal_tally_result, is updated to accommodate the new structure.Run the following script to identify all Go code that accesses
final_tally_resultwithinProposal:#!/bin/bash # Description: Find all usages of `final_tally_result` in Go files. rg --type go 'Proposal.*final_tally_result'
145-154: Ensure compatibility with the newTallyResultmessageThe introduction of the new
TallyResultmessage with additional fields (tally_height,total_staking_power,total_vesting_power, andv1_tally_result) is a significant change. Verify that all parts of the codebase that interact withTallyResultare updated accordingly, and that serialization/deserialization processes handle the new structure properly.Run the following script to find all usages of
cosmos.gov.v1.TallyResultthat may need to be updated:x/gov/keeper/tally.go (6)
22-22: Function signature updated to returncustomtypes.TallyResultThe
Tallyfunction now returnstallyResultsof typecustomtypes.TallyResult, aligning with the updated data structures.
35-36: Proper initialization oftotalStakingPowerandtotalVestingPowerInitializing
totalStakingPowerandtotalVestingPowerto zero ensures accurate accumulation of voting power.
179-185: Constructcustomtypes.TallyResultcorrectlyThe
tallyResultsobject is constructed appropriately with the updated fields:
V1TallyResultTallyHeightTotalStakingPowerTotalVestingPower
187-189: Correctly handle case whentotalPoweris zeroThe check for zero
totalPowerensures that proposals fail when there are no staked or vesting coins, which is logical and prevents division by zero errors later in the code.
194-194: CalculatepercentVotingusingtotalVotingPowerandtotalPowerThe calculation of
percentVotingis updated to use the newtotalPower, correctly reflecting the sum of staking and vesting power, ensuring the quorum is based on the combined total.
111-111: Type mismatch when addingamounttototalVestingPowerEnsure that
amountreturned fromk.vestingKeeper.GetUnclaimedVestedAmountis of typemath.Int, consistent withtotalVestingPower.To verify the return type of
GetUnclaimedVestedAmount, run the following script:
| switch { | ||
| case proposal.Status == v1.StatusDepositPeriod: | ||
| tallyResult = customtypes.EmptyTallyResult() | ||
|
|
||
| case proposal.Status == v1.StatusPassed || proposal.Status == v1.StatusRejected: | ||
| tallyResult = proposal.FinalTallyResult | ||
|
|
||
| default: | ||
| // proposal is in voting period | ||
| params, err := q.Keeper.Params.Get(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| _, _, _, tallyResult, err = q.Keeper.Tally(ctx, params, proposal) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Explicitly handle all proposal statuses to improve code clarity
In the switch statement starting at line 141~, consider explicitly handling all possible proposal statuses. Currently, the code uses a default case to handle proposals in the voting period and any other statuses. Explicitly listing all statuses can enhance readability and prevent unexpected behaviors if new statuses are introduced in the future.
Apply this change to explicitly handle the StatusVotingPeriod and return an error for any unanticipated statuses:
switch {
case proposal.Status == v1.StatusDepositPeriod:
tallyResult = customtypes.EmptyTallyResult()
+case proposal.Status == v1.StatusVotingPeriod:
// proposal is in voting period
params, err := q.Keeper.Params.Get(ctx)
if err != nil {
return nil, err
}
_, _, _, tallyResult, err = q.Keeper.Tally(ctx, params, proposal)
if err != nil {
return nil, err
}
+default:
+ return nil, status.Errorf(codes.InvalidArgument, "invalid proposal status: %s", proposal.Status.String())
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch { | |
| case proposal.Status == v1.StatusDepositPeriod: | |
| tallyResult = customtypes.EmptyTallyResult() | |
| case proposal.Status == v1.StatusPassed || proposal.Status == v1.StatusRejected: | |
| tallyResult = proposal.FinalTallyResult | |
| default: | |
| // proposal is in voting period | |
| params, err := q.Keeper.Params.Get(ctx) | |
| if err != nil { | |
| return nil, err | |
| } | |
| _, _, _, tallyResult, err = q.Keeper.Tally(ctx, params, proposal) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| switch { | |
| case proposal.Status == v1.StatusDepositPeriod: | |
| tallyResult = customtypes.EmptyTallyResult() | |
| case proposal.Status == v1.StatusPassed || proposal.Status == v1.StatusRejected: | |
| tallyResult = proposal.FinalTallyResult | |
| case proposal.Status == v1.StatusVotingPeriod: | |
| // proposal is in voting period | |
| params, err := q.Keeper.Params.Get(ctx) | |
| if err != nil { | |
| return nil, err | |
| } | |
| _, _, _, tallyResult, err = q.Keeper.Tally(ctx, params, proposal) | |
| if err != nil { | |
| return nil, err | |
| } | |
| default: | |
| return nil, status.Errorf(codes.InvalidArgument, "invalid proposal status: %s", proposal.Status.String()) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
client/docs/swagger-ui/swagger.yamlis excluded by!**/*.yamlx/gov/types/gov.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (7)
- proto/ibc/applications/perm/v1/genesis.proto (1 hunks)
- proto/ibc/applications/perm/v1/query.proto (1 hunks)
- proto/ibc/applications/perm/v1/tx.proto (3 hunks)
- proto/ibc/applications/perm/v1/types.proto (1 hunks)
- proto/initia/gov/v1/gov.proto (4 hunks)
- proto/initia/gov/v1/query.proto (3 hunks)
- x/gov/keeper/tally.go (5 hunks)
Files skipped from review due to trivial changes (4)
- proto/ibc/applications/perm/v1/genesis.proto
- proto/ibc/applications/perm/v1/query.proto
- proto/ibc/applications/perm/v1/tx.proto
- proto/ibc/applications/perm/v1/types.proto
Files skipped from review as they are similar to previous changes (1)
- x/gov/keeper/tally.go
Additional context used
buf
proto/initia/gov/v1/gov.proto
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
proto/initia/gov/v1/query.proto
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
Additional comments not posted (6)
proto/initia/gov/v1/query.proto (3)
35-39: Approve addition of TallyResult RPC methodThe new
TallyResultRPC method is a valuable addition to theQueryservice. It provides a way to query the tally of votes for a specific proposal, which is consistent with the governance functionality. The method signature and HTTP endpoint are well-defined and follow the existing patterns in the service.
107-111: Approve QueryTallyResultRequest message definitionThe
QueryTallyResultRequestmessage is well-defined and consistent with other request messages in the file. It contains a singleproposal_idfield of typeuint64, which is appropriate for identifying a specific proposal for tally querying.
113-117: Approve QueryTallyResultResponse message and verify TallyResult typeThe
QueryTallyResultResponsemessage is well-defined and consistent with other response messages in the file. The use ofgogoproto.nullable = falseensures that thetally_resultfield is always present, which is good for API consistency.However, please ensure that the
TallyResulttype is properly defined, likely in theinitia/gov/v1/gov.protofile, as it's not defined in this file.To verify the
TallyResulttype definition, you can run the following script:#!/bin/bash # Search for TallyResult definition in .proto files echo "Searching for TallyResult definition:" rg -t proto 'message TallyResult'proto/initia/gov/v1/gov.proto (3)
4-4: Import "amino/amino.proto" still does not exist.The import statement references a file that does not exist, which will lead to compilation errors.
Tools
buf
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
123-131: Introduction of newTallyResultmessage enhances governance functionality.The addition of the
TallyResultmessage with fieldstally_height,total_staking_power,total_vesting_power, andv1_tally_resultprovides a more detailed representation of tally results, improving clarity and functionality.
147-147: Updatedfinal_tally_resultfield to use newTallyResulttype.Changing the
final_tally_resultfield in theProposalmessage to use the newTallyResultmessage ensures consistency with the updated tally structure.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #266 +/- ##
==========================================
+ Coverage 40.70% 40.72% +0.02%
==========================================
Files 264 264
Lines 25230 25255 +25
==========================================
+ Hits 10269 10286 +17
- Misses 13371 13374 +3
- Partials 1590 1595 +5
|
Description
New Endpoints
initia/gov/v1/tally_resultvote quorum is calculated with
total_staking_power+total_vesting_power.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...