refactor(zetacore): delete ballots after maturity #2863
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduced in this pull request focus on enhancing ballot management within the system, specifically addressing the deletion of matured ballots. This includes the addition of new message types, modifications to existing functions for handling ballots, and the introduction of event emissions upon ballot deletion. The updates aim to streamline the lifecycle management of ballots, ensuring that matured ballots are efficiently removed from the state to optimize resource use. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Keeper
participant EventManager
User->>Keeper: Request to delete matured ballots
Keeper->>Keeper: Identify matured ballots
Keeper->>Keeper: Delete matured ballots
Keeper->>EventManager: Emit EventBallotDeleted
EventManager-->>Keeper: Confirmation of event emission
Assessment against linked issues
Possibly related PRs
Suggested labels
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
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2863 +/- ##
===========================================
+ Coverage 61.71% 61.79% +0.07%
===========================================
Files 440 441 +1
Lines 31141 31235 +94
===========================================
+ Hits 19220 19302 +82
- Misses 11058 11066 +8
- Partials 863 867 +4
|
kingpinXD
changed the title
kingpinXD
requested review from
brewmaster012,
lumtis,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (6)
x/emissions/types/expected_keepers.go (1)
19-19: Consider enhancing the clarity and documentation of theClearMaturedBallotsmethod.The addition of the
ClearMaturedBallotsmethod to theObserverKeeperinterface is a good change as it allows for the clearing of matured ballots. However, consider the following suggestions to improve the clarity and documentation of the method:
Consider renaming the method to something more specific, such as
RemoveMaturedBallotsFromState, to clearly indicate that it's removing the ballots from the state.Consider renaming the
maturityBlocksparameter to a more descriptive name, such asmaturityThreshold, to clarify its purpose.Add a comment above the method to explain its functionality and any important details about its usage. This will help future maintainers understand the purpose and behavior of the method.
x/observer/keeper/events_test.go (1)
15-37: Consider defining the test case struct outside the test function for better readability.The test case struct is defined within the test function, which can make the test function harder to read. Consider moving the struct definition outside the test function.
-func TestEmitEventBallotDeleted(t *testing.T) { - tt := []struct { - name string - ballotIdentifier string - ballotType types.ObservationType - voters []string - voteType []types.VoteType - }{ - { - name: "successfull votes only", - ballotIdentifier: sample.ZetaIndex(t), - ballotType: types.ObservationType_InboundTx, - voters: []string{"voter1", "voter2"}, - voteType: []types.VoteType{types.VoteType_SuccessObservation, types.VoteType_SuccessObservation}, - }, - { - name: "failed votes only", - ballotIdentifier: sample.ZetaIndex(t), - ballotType: types.ObservationType_InboundTx, - voters: []string{"voter1", "voter2"}, - voteType: []types.VoteType{types.VoteType_FailureObservation, types.VoteType_FailureObservation}, - }, - } +type testCase struct { + name string + ballotIdentifier string + ballotType types.ObservationType + voters []string + voteType []types.VoteType +} + +func TestEmitEventBallotDeleted(t *testing.T) { + tt := []testCase{ + { + name: "successfull votes only", + ballotIdentifier: sample.ZetaIndex(t), + ballotType: types.ObservationType_InboundTx, + voters: []string{"voter1", "voter2"}, + voteType: []types.VoteType{types.VoteType_SuccessObservation, types.VoteType_SuccessObservation}, + }, + { + name: "failed votes only", + ballotIdentifier: sample.ZetaIndex(t), + ballotType: types.ObservationType_InboundTx, + voters: []string{"voter1", "voter2"}, + voteType: []types.VoteType{types.VoteType_FailureObservation, types.VoteType_FailureObservation}, + }, + }x/observer/keeper/grpc_query_ballot_test.go (1)
Line range hint
127-132: Approved with SuggestionsThe code changes are minimal and do not introduce any issues. However, consider the following suggestions to improve the code:
Improve Error Handling: In the 3rd sub-test, use
require.ErrorIsto check for a specific error type instead of justrequire.Error. This provides a more precise assertion.Simplify
VotersField: TheVotersfield in the expected response can be simplified by using a slice literal instead of a slice of structs. This improves readability and reduces verbosity.- Voters: []types.VoterList{ - { - VoterAddress: voter, - VoteType: types.VoteType_SuccessObservation, - }, - }, + Voters: []types.VoterList{{voter, types.VoteType_SuccessObservation}},x/observer/keeper/ballot_test.go (1)
211-310: Suggestions for improvement:The
TestKeeper_ClearMaturedBallotsfunction is well-structured and covers various scenarios. However, consider the following suggestions to enhance the test:
- Extract the hardcoded
numberOfBallotsvalue as a constant or use a random number to ensure the tests are more robust.- Extract the event counting logic into a separate helper function to avoid duplication in the first and third sub-tests.
- Use
appendinstead of initializing theballotsslice with a fixed size based onnumberOfBallotsto make the code more flexible.x/emissions/abci_test.go (2)
Line range hint
206-330: Consider refactoring the test for better readability, robustness, and performance.While the test covers the core functionality well, consider the following improvements:
Split the test into smaller, focused sub-tests using
t.Run()for better readability and maintainability. Each sub-test can cover a specific scenario, such as successful distribution, edge cases, and error scenarios.Add more assertions to cover edge cases and error scenarios, such as:
- Testing with an empty observer set.
- Testing with no ballots.
- Testing with insufficient funds in the emission pool.
- Testing with invalid parameters.
Optimize the test by reducing the number of blocks (
numberOfTestBlocks) to the minimum required to cover the scenarios. This will make the test run faster without compromising the coverage.Use more concise assertions by leveraging helper functions or libraries like
testify/require. This will make the test more readable and maintainable.
Line range hint
332-487: Consider enhancing the test for better robustness and maintainability.The test is well-structured and covers the important scenarios. The use of table-driven tests is a good practice. However, consider the following improvements:
Add assertions for error scenarios, such as:
- Testing with an invalid observer set.
- Testing with invalid ballots.
- Testing with insufficient funds in the emission pool.
Extract the common setup code into a helper function to avoid duplication and improve readability. The setup code includes:
- Creating the keeper instances.
- Setting up the observer set and ballots.
- Funding the emission pool.
- Setting the parameters.
This will make the test more maintainable and easier to extend with new scenarios.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (6)
typescript/zetachain/zetacore/observer/ballot_pb.d.tsis excluded by!**/*_pb.d.tstypescript/zetachain/zetacore/observer/events_pb.d.tsis excluded by!**/*_pb.d.tstypescript/zetachain/zetacore/observer/query_pb.d.tsis excluded by!**/*_pb.d.tsx/observer/types/ballot.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/observer/types/events.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/observer/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (16)
- changelog.md (1 hunks)
- proto/zetachain/zetacore/observer/ballot.proto (1 hunks)
- proto/zetachain/zetacore/observer/events.proto (2 hunks)
- proto/zetachain/zetacore/observer/query.proto (1 hunks)
- testutil/keeper/mocks/emissions/observer.go (1 hunks)
- x/emissions/abci.go (3 hunks)
- x/emissions/abci_test.go (4 hunks)
- x/emissions/types/expected_keepers.go (1 hunks)
- x/observer/keeper/ballot.go (3 hunks)
- x/observer/keeper/ballot_test.go (1 hunks)
- x/observer/keeper/events.go (1 hunks)
- x/observer/keeper/events_test.go (1 hunks)
- x/observer/keeper/grpc_query_ballot.go (1 hunks)
- x/observer/keeper/grpc_query_ballot_test.go (1 hunks)
- x/observer/types/ballot.go (1 hunks)
- x/observer/types/ballot_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- changelog.md
Additional context used
Path-based instructions (15)
x/emissions/types/expected_keepers.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.proto/zetachain/zetacore/observer/ballot.proto (1)
Pattern
**/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.x/observer/keeper/grpc_query_ballot.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.proto/zetachain/zetacore/observer/events.proto (1)
Pattern
**/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.x/observer/keeper/events.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/observer/keeper/events_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.testutil/keeper/mocks/emissions/observer.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/observer/keeper/ballot.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/observer/types/ballot.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/observer/keeper/grpc_query_ballot_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/emissions/abci.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/observer/keeper/ballot_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.proto/zetachain/zetacore/observer/query.proto (1)
Pattern
**/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.x/observer/types/ballot_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/emissions/abci_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (19)
proto/zetachain/zetacore/observer/ballot.proto (1)
45-48: LGTM!The
VoterListmessage is well-structured and follows the protocol buffer conventions. The field names are descriptive and the types are appropriate. The use of the existingVoteTypeenum for thevote_typefield ensures type safety.This message definition aligns with the PR objectives of tracking voter information and can be effectively used to represent a voter's address and their corresponding vote type.
x/observer/keeper/grpc_query_ballot.go (1)
46-46: Refactoring Approved: Voter List Generation Delegated to a Separate MethodThe refactoring of the voter list generation logic into the
GenerateVoterListmethod improves code clarity and maintainability. It encapsulates the complexity of constructing the voter list, reducing the potential for errors in theBallotByIdentifierfunction.However, to ensure a comprehensive review, it is recommended to also review the implementation of the
GenerateVoterListmethod to verify its correctness and efficiency.proto/zetachain/zetacore/observer/events.proto (2)
7-7: LGTM!The import statement is necessary for using the
VoterListmessage in theEventBallotDeletedmessage.
19-24: LGTM!The
EventBallotDeletedmessage is well-defined and captures the necessary information for a ballot deletion event. The use ofrepeatedand(gogoproto.nullable) = falsefor thevotersfield is a good practice to ensure data integrity.x/observer/keeper/events_test.go (1)
77-79: LGTM!The
RemoveQuotesfunction is simple and straightforward. The code changes are approved.testutil/keeper/mocks/emissions/observer.go (1)
18-21: LGTM!The new mock function
ClearMaturedBallotsis correctly implemented. It properly registers the invocation with the provided arguments using theCalledmethod from themock.Mocklibrary, which is the expected behavior for a mock function.x/observer/keeper/ballot.go (5)
23-26: LGTM!The
DeleteBallotfunction is implemented correctly and follows the standard pattern for deleting data from the store using a prefix store. The code changes are approved.
28-31: LGTM!The
DeleteBallotListfunction is implemented correctly and follows the standard pattern for deleting data from the store using a prefix store. The code changes are approved.
54-54: LGTM!The changes to the
GetMaturedBallotsfunction improve the clarity and modularity of the code by separating the height calculation logic into a new helper functionGetMaturedBallotHeight. The code changes are approved.
79-88: LGTM!The
ClearMaturedBallotsfunction is well-structured and encapsulates the logic for clearing matured ballots effectively. It iterates over the list of ballots, deletes each one using theDeleteBallotmethod, and emits an event for each deletion. It also deletes the ballot list corresponding to the matured height using theDeleteBallotListmethod. The code changes are approved.
90-92: LGTM!The
GetMaturedBallotHeightfunction is a simple and clear implementation of the height calculation logic. It improves the readability and modularity of the code by separating this logic from theGetMaturedBallotsfunction. The code changes are approved.x/observer/keeper/grpc_query_ballot_test.go (1)
Line range hint
14-80: LGTM!The
TestKeeper_HasVotedfunction is well-structured and covers important scenarios. The code changes are approved.x/emissions/abci.go (3)
13-13: LGTM!The import statement is necessary and correctly added.
106-112: LGTM!The code changes for initializing the
ballotsslice and accumulating the ballots are correctly implemented.
167-169: LGTM!The code changes for clearing the matured ballots are correctly implemented.
x/observer/keeper/ballot_test.go (2)
122-163: LGTM!The
TestKeeper_DeleteBallotfunction is well-structured and covers the necessary scenarios for testing the deletion of ballots. The use of sub-tests enhances readability and maintainability.
165-209: LGTM!The
TestKeeper_DeleteBallotListfunction is well-structured and covers the necessary scenarios for testing the deletion of ballot lists. The use of sub-tests enhances readability and maintainability.proto/zetachain/zetacore/observer/query.proto (1)
252-252: Approve the non-nullable option for thevotersfield.The addition of the
(gogoproto.nullable) = falseoption for thevotersfield enhances data integrity by enforcing that the list cannot contain null values. This change aligns with the goal of improving the robustness of thevotersfield.Verify the removal of the
VoterListmessage.The
VoterListmessage has been removed from the diff. While this change does not directly affect theQueryBallotByIdentifierResponsemessage, it is important to ensure that the removal of theVoterListmessage does not introduce any compatibility issues or unintended consequences in other parts of the codebase.Please confirm that the removal of the
VoterListmessage is intentional and does not impact the functionality or compatibility of the system.x/observer/types/ballot_test.go (1)
482-585: Excellent test coverage for theGenerateVoterListmethod.The
TestBallot_GenerateVoterListfunction is a valuable addition to the test suite. It employs a table-driven approach to test theGenerateVoterListmethod of theBallotstruct, covering scenarios with success observations, failure observations, and mixed observations.The test cases are well-defined, and the expected voter lists are clearly specified. This approach enhances the readability and maintainability of the test code.
Great work on improving the test coverage!
kingpinXD
changed the title
kingpinXD
changed the title
x/observer/keeper/ballot.go
Outdated
|
|
||
| // ClearMaturedBallotsAndBallotList deletes all matured ballots and the list of ballots for a given height. | ||
| // It also emits an event for each ballot deleted. | ||
| func (k Keeper) ClearMaturedBallotsAndBallotList(ctx sdk.Context, ballots []types.Ballot, maturityBlocks int64) { |
There was a problem hiding this comment.
I agree with @swift1337 , we should add more encapsulation, the external module shouldn't need to provide a list of ballot
-> You provide an height
-> You can get all ballots from that height in the function
-> You delete all the ballots
There was a problem hiding this comment.
The primary reason for not doing that is that the ballots are already fetched once, and I would want to avoid reading the same data from the store twice.
There was a problem hiding this comment.
I don't think you need to reread the store for ballots? The ballot list from height give the ID list that can be directly used in the delete function?
This is small optimization in any case I would consider the encapsulation of the code more important
There was a problem hiding this comment.
That is true, but the delete ballot event uses other fields of the ballot.
We could alternatively fire the event from abci.go, but I feel that firing it from the for loop is better.
IMO, the current design looks cleaner.
Fetch ballotslist -> Fetch Ballot -> distribute rewards -> delete them.
The new flow would be
Fetch ballots -> distribute rewards -> Fetch ballotslist ->Fetch Ballots -> delete them.
There was a problem hiding this comment.
The current interface doesn't respect encapsulation principle. We delete ballots from a list and a BallotListForHeight value, while the list of ballots can be derived from this height. Both values should be deleted atomically from the same inputs, the current interface allows an external module to create an inconsistent state for the ballot
| func (k Keeper) ClearMaturedBallotsAndBallotList(ctx sdk.Context, ballots []types.Ballot, maturityBlocks int64) { | ||
| for _, ballot := range ballots { | ||
| k.DeleteBallot(ctx, ballot.BallotIdentifier) | ||
| EmitEventBallotDeleted(ctx, ballot) |
There was a problem hiding this comment.
I don't think we need an event here
There was a problem hiding this comment.
Once indexed, the event can be used to query details about deleted ballots without having to query an archive node with an older height.
#942 (comment)
There was a problem hiding this comment.
I do agree though we should remove the event if its not useful .
There was a problem hiding this comment.
I agree we should remove it. This will just cause further state size increases right?
There was a problem hiding this comment.
Events would go into the blockstore , and I think nodes have the option to not store them through configuration, events usually are not part of the application.db .
But yeah, we should avoid adding anything to the node that we don't need
The primary use case that I can think of , where we use the data from these events is for debugging to see if there is a particular observer missing votes consistently fro more than maturityBlocks
|
Small note: let's keep this one aside for now to not introduce migration scripts as we already heavy operations with gateway and Solana |
|
Hi @lumtis We can consider breaking the pr down, into
|
We actually want to remove all old ballot so we can easily query all active ballots |
| func (k Keeper) ClearMaturedBallotsAndBallotList(ctx sdk.Context, ballots []types.Ballot, maturityBlocks int64) { | ||
| for _, ballot := range ballots { | ||
| k.DeleteBallot(ctx, ballot.BallotIdentifier) | ||
| EmitEventBallotDeleted(ctx, ballot) |
There was a problem hiding this comment.
I agree we should remove it. This will just cause further state size increases right?
| @@ -30,18 +42,21 @@ func (k Keeper) GetBallot(ctx sdk.Context, index string) (val types.Ballot, foun | |||
| return val, true | |||
| } | |||
|
|
|||
| func (k Keeper) GetBallotList(ctx sdk.Context, height int64) (val types.BallotListForHeight, found bool) { | |||
| func (k Keeper) GetBallotListForHeight(ctx sdk.Context, height int64) (val types.BallotListForHeight, found bool) { | |||
There was a problem hiding this comment.
Would just returning a types.BallotListForHeight be enough for the caller?
There was a problem hiding this comment.
I think it should be fine , but this pr just renames the function ,
We can consider changing the signature in a separate PR, as the function is used in a few different places.
| if !found { | ||
| continue | ||
| } | ||
| k.DeleteBallot(ctx, ballotIndex) |
There was a problem hiding this comment.
what happens if the ballot is still pending with minority votes (e.g., 4/9)?
There was a problem hiding this comment.
I think the chances of that are quite unlikely, that is why we have maturity blocks
However, if that does happen, a new ballot would be created when the fifth vote comes in, and the four observers who already voted would need to revote. This can be triggered by adding trackers manually.
Overall though the maturity blocks should be adjusted so that it has enough time for a ballot to finalize as we do not want to keep the ballots in the system indefinitly
Description
Closes #942
The PR does the following
How Has This Been Tested?
Summary by CodeRabbit
New Features
VoterListmessage type to track individual voter details.Bug Fixes
Tests
Documentation