refactor: add a new buffer blocks param to delay deletion of pending ballots #3553
Conversation
kingpinXD
changed the title
|
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 📝 WalkthroughWalkthroughThis pull request introduces a new parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant DOR as DistributeObserverRewards
participant BM as Ballot Management
participant PB as Pending Ballots Checker
participant UP as Undistributed Pool
DOR->>BM: Delete finalized ballots at maturity
BM-->>DOR: Acknowledge deletion
DOR->>BM: Delete remaining ballots using buffer blocks
BM-->>DOR: Confirm ballots cleared
DOR->>UP: Transfer funds to undistributed pool
sequenceDiagram
participant M as Migrator
participant EK as EmissionsKeeper (v5)
M->>EK: Retrieve current parameters
EK-->>M: Return existing parameters (if found)
M->>EK: Set updated parameters with new pending_ballots_buffer_blocks
EK-->>M: Confirm parameter update
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
kingpinXD
changed the title
kingpinXD
changed the title
kingpinXD
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3553 +/- ##
===========================================
+ Coverage 64.53% 64.55% +0.01%
===========================================
Files 469 469
Lines 32694 32723 +29
===========================================
+ Hits 21099 21123 +24
- Misses 10635 10639 +4
- Partials 960 961 +1
|
…etion' into feat-delay-unfinished-ballot-deletion
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
x/observer/types/ballot.go (1)
102-125: Optimize vote lookup to avoid repeated index searches.This loop calls
m.GetVoterIndex(address)for each voter, which can degrade performance ifm.VoterListgrows large. You could derive the index once by iterating simultaneously overm.VoterListandm.Votesor by storing votes in a map keyed by address to achieve O(n) overall.x/emissions/types/keys_test.go (2)
31-34: Improve test case naming and description.The test case for negative values could be more descriptive, and the comment about "not expected" should be moved to a separate documentation.
Apply this diff to improve clarity:
{ - name: "negative value(not expected , but adding a test case for completeness)", + name: "negative value", seconds: -10, expectedBlocks: -1, },Consider adding a comment above the test function documenting that negative values are not expected in production but are tested for completeness.
38-41: Enhance test failure message.The failure message could be more descriptive to help debug test failures.
Apply this diff to improve the error message:
-t.Fatalf("expected %d, got %d", tc.expectedBlocks, blocks) +t.Fatalf("SecondsToBlocks(%d) = %d, want %d", tc.seconds, blocks, tc.expectedBlocks)x/emissions/types/expected_keepers.go (1)
22-23: Consider more concise method names.The current method names are descriptive but quite long. Consider shorter alternatives that maintain clarity.
Consider these alternatives:
-ClearAllMaturedBallotsAndBallotList(ctx sdk.Context, maturityBlocks int64) +ClearAllMaturedBallots(ctx sdk.Context, maturityBlocks int64) -ClearFinalizedMaturedBallots(ctx sdk.Context, maturityBlocks int64) +ClearFinalizedBallots(ctx sdk.Context, maturityBlocks int64)x/emissions/keeper/msg_server_update_params_test.go (2)
43-54: LGTM! Consider enhancing test readability.The test case effectively validates the error condition for invalid validator emission percentage.
Consider using a table-driven test to make the test cases more maintainable:
-t.Run("fail for invalid params ,validatorEmissionPercentage is invalid", func(t *testing.T) { +func TestMsgServer_UpdateParams_InvalidParams(t *testing.T) { + tests := []struct { + name string + params func() types.Params + }{ + { + name: "invalid validator emission percentage", + params: func() types.Params { + p := types.DefaultParams() + p.ValidatorEmissionPercentage = "-1.5" + return p + }, + }, + // Add more test cases here + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { k, ctx, _, _ := keepertest.EmissionsKeeper(t) msgServer := keeper.NewMsgServerImpl(*k) - params := types.DefaultParams() - params.ValidatorEmissionPercentage = "-1.5" _, err := msgServer.UpdateParams(ctx, &types.MsgUpdateParams{ Authority: k.GetAuthority(), - Params: params, + Params: tt.params(), }) require.ErrorIs(t, err, types.ErrUnableToSetParams) }) + } +}
56-67: LGTM! Consider adding boundary value tests.The test case correctly validates negative values for pending buffer blocks.
Consider adding boundary value tests to ensure robust validation:
+t.Run("fail for invalid params, pending buffer blocks at boundary values", func(t *testing.T) { + testCases := []struct { + value int64 + name string + }{ + {-1, "negative"}, + {0, "zero"}, + {math.MaxInt64, "max int64"}, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + k, ctx, _, _ := keepertest.EmissionsKeeper(t) + msgServer := keeper.NewMsgServerImpl(*k) + params := types.DefaultParams() + params.PendingBallotsBufferBlocks = tc.value + _, err := msgServer.UpdateParams(ctx, &types.MsgUpdateParams{ + Authority: k.GetAuthority(), + Params: params, + }) + require.ErrorIs(t, err, types.ErrUnableToSetParams) + }) + } +})x/emissions/migrations/v5/migrate_test.go (1)
47-75: Consider separating test setup and assertions.The test case mixes setup, execution, and assertions. Consider structuring it more clearly.
Suggested structure:
t.Run("successful migrate store", func(t *testing.T) { // Setup k, ctx, _, _ := keepertest.EmissionsKeeper(t) mainnetParams := setupMainnetParams() SetLegacyParams(t, k, ctx, mainnetParams) // Execute err := v5.MigrateStore(ctx, k) // Assert require.NoError(t, err) assertParamsMatch(t, k, ctx, mainnetParams) }) +func setupMainnetParams() LegacyParams { + return LegacyParams{ + ValidatorEmissionPercentage: "0.75", + // ... other fields + } +} +func assertParamsMatch(t *testing.T, k *keeper.Keeper, ctx sdk.Context, expected LegacyParams) { + updatedParams, found := k.GetParams(ctx) + require.True(t, found) + require.Equal(t, expected.ValidatorEmissionPercentage, updatedParams.ValidatorEmissionPercentage) + // ... other assertions +}x/emissions/types/params_test.go (1)
121-125: Consider adding more test cases.While the test for negative values is good, consider adding test cases for:
- Zero value (valid case for immediate deletion)
- Positive value (valid case for delayed deletion)
Add these test cases:
t.Run("should error if pending ballots buffer blocks is negative", func(t *testing.T) { params := NewParams() params.PendingBallotsBufferBlocks = -100 require.Error(t, params.Validate()) }) +t.Run("should accept zero pending ballots buffer blocks", func(t *testing.T) { + params := NewParams() + params.PendingBallotsBufferBlocks = 0 + require.NoError(t, params.Validate()) +}) +t.Run("should accept positive pending ballots buffer blocks", func(t *testing.T) { + params := NewParams() + params.PendingBallotsBufferBlocks = 100 + require.NoError(t, params.Validate()) +})x/emissions/module.go (1)
25-25: Consider documenting the consensus version change.The consensus version update and migration registration are properly implemented. Consider adding a comment explaining the reason for the version bump to help future maintainers.
Add a comment above the constant:
+// ConsensusVersion is the current consensus version, incremented to 5 for +// the addition of PendingBallotsBufferBlocks parameter const ConsensusVersion = 5Also applies to: 144-146, 175-175
x/emissions/abci.go (1)
110-111: Track the TODO in an issue.The TODO comment indicates a need to move params to the observer module. Consider tracking this in a dedicated issue.
Would you like me to create an issue to track moving these parameters to the observer module?
docs/openapi/openapi.swagger.yaml (1)
60473-60473: Naming Consistency Between Schema and Sample Values:
In the schema, the property is defined using snake_case aspending_ballots_buffer_blocks, while in the sample values the property is rendered asPendingBallotsBufferBlocks(CamelCase). Ensure that this transformation is handled consistently by your system or document conversion process. If this difference is not deliberate, consider standardizing the naming convention between the schema definition and its examples.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
typescript/zetachain/zetacore/emissions/params_pb.d.tsis excluded by!**/*_pb.d.tsx/emissions/types/params.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (22)
changelog.md(1 hunks)docs/openapi/openapi.swagger.yaml(2 hunks)proto/zetachain/zetacore/emissions/params.proto(2 hunks)testutil/keeper/mocks/emissions/observer.go(1 hunks)x/emissions/abci.go(2 hunks)x/emissions/abci_test.go(4 hunks)x/emissions/keeper/migrator.go(2 hunks)x/emissions/keeper/msg_server_update_params_test.go(2 hunks)x/emissions/migrations/v3/migrate_test.go(1 hunks)x/emissions/migrations/v5/migrate.go(1 hunks)x/emissions/migrations/v5/migrate_test.go(1 hunks)x/emissions/module.go(3 hunks)x/emissions/types/expected_keepers.go(1 hunks)x/emissions/types/keys.go(1 hunks)x/emissions/types/keys_test.go(1 hunks)x/emissions/types/params.go(3 hunks)x/emissions/types/params_test.go(1 hunks)x/observer/keeper/ballot.go(1 hunks)x/observer/keeper/ballot_test.go(5 hunks)x/observer/module.go(2 hunks)x/observer/types/ballot.go(2 hunks)x/observer/types/ballot_test.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/emissions/migrations/v3/migrate_test.gox/emissions/keeper/migrator.gox/observer/types/ballot.gox/emissions/types/keys.gox/emissions/types/params_test.gox/emissions/module.gotestutil/keeper/mocks/emissions/observer.gox/emissions/keeper/msg_server_update_params_test.gox/observer/module.gox/emissions/types/params.gox/emissions/types/keys_test.gox/emissions/migrations/v5/migrate.gox/emissions/types/expected_keepers.gox/emissions/abci_test.gox/observer/types/ballot_test.gox/emissions/abci.gox/observer/keeper/ballot_test.gox/observer/keeper/ballot.gox/emissions/migrations/v5/migrate_test.go
`**/*.proto`: Review the Protobuf definitions, point out iss...
**/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
proto/zetachain/zetacore/emissions/params.proto
🪛 GitHub Check: codecov/patch
x/observer/types/ballot.go
[warning] 82-83: x/observer/types/ballot.go#L82-L83
Added lines #L82 - L83 were not covered by tests
x/emissions/types/params.go
[warning] 135-136: x/emissions/types/params.go#L135-L136
Added lines #L135 - L136 were not covered by tests
x/observer/keeper/ballot.go
[warning] 99-99: x/observer/keeper/ballot.go#L99
Added line #L99 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: start-e2e-test / e2e
🔇 Additional comments (21)
x/observer/types/ballot.go (2)
82-84: Consider adding unit tests forIsFinalized.While this helper function reads cleanly, lines 82-83 are reported as uncovered in the code coverage analysis. Adding a small test would help ensure ongoing reliability and clarify whether any states beyond
BallotStatus_BallotInProgressshould be considered non-finalized in the future.Would you like me to propose a quick unit test snippet?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 82-83: x/observer/types/ballot.go#L82-L83
Added lines #L82 - L83 were not covered by tests
97-100: Acknowledge in-progress ballots yield zero rewards.Returning zero for in-progress ballots aligns with the requirement of distributing rewards only for finalized ballots. Be sure to confirm that any edge cases (e.g., a ballot stuck in progress) are handled elsewhere to avoid indefinite reward omissions.
x/observer/keeper/ballot.go (2)
85-115: Verify test coverage of the not-found ballot scenario.This new method properly checks for a finalized ballot before deleting. However, line 99 shows a coverage gap for ballots that are in the list but not found in the store. Consider adding a test case for that condition to ensure correctness and avoid regressions.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 99-99: x/observer/keeper/ballot.go#L99
Added line #L99 was not covered by tests
117-139: Matured ballot cleanup logic looks solid.The function reliably deletes matured ballots and their list. The implementation is consistent with the approach in other parts of the codebase. There are no significant correctness or maintainability concerns.
x/observer/keeper/ballot_test.go (2)
236-355: All sub-tests forClearAllMaturedBallotsAndBallotListlook correct.These tests cover various scenarios, including missing ballots, partially found ballots, and no ballots. Each scenario ensures clear expectations, and the code properly asserts final states. Overall, this set of sub-tests is thorough and well-written.
359-466: Adequate coverage ofClearFinalizedMaturedBallots.The test function comprehensively checks finalized vs. in-progress ballots, ensuring only finalized ones are removed. This approach validates correctness and helps prevent regressions. Nicely done.
x/emissions/keeper/migrator.go (1)
36-39: LGTM!The implementation follows the established pattern and correctly integrates the v5 migration.
x/emissions/migrations/v3/migrate_test.go (1)
64-66: LGTM! Clear documentation of migration behavior.The explicit setting of
PendingBallotsBufferBlocksto 0 with a clear comment helps understand the v3 migration behavior.x/emissions/types/params.go (2)
19-19: LGTM!The new parameter
PendingBallotsBufferBlocksis properly initialized in theNewParamsfunction.
45-47: LGTM!The validation for
PendingBallotsBufferBlocksis properly integrated into theValidatemethod.testutil/keeper/mocks/emissions/observer.go (1)
19-27: LGTM!The mock implementation correctly reflects the interface changes with:
- Renamed method
ClearAllMaturedBallotsAndBallotList- New method
ClearFinalizedMaturedBallotsx/observer/module.go (1)
24-24: LGTM! Good practice using a constant for version management.The change improves maintainability by centralizing the consensus version in a constant.
Also applies to: 184-184
x/emissions/abci.go (2)
114-118: LGTM! Clear documentation of parameter purposes.The comments effectively explain the purpose of
maturityBlocksandpendingBallotsBufferBlocks, improving code maintainability.
152-159: LGTM! Improved ballot lifecycle management.The separation of ballot deletion into two steps is a good improvement:
- Delete finalized ballots at maturity height
- Delete remaining ballots at buffered maturity height
This change provides a grace period for pending ballots to be finalized.
x/observer/types/ballot_test.go (2)
469-491: LGTM! Good test coverage for pending ballots.The new test case effectively verifies that no rewards are distributed for non-finalized ballots, regardless of the votes cast.
493-539: LGTM! Comprehensive test coverage for rewards distribution.The test cases thoroughly verify the rewards distribution logic with different ballot statuses and initial conditions.
x/emissions/abci_test.go (1)
625-629: LGTM! Good test coverage for ballot deletion.The test verifies that ballots and ballot lists are only deleted when the ballot status is not in progress, aligning with the new buffer blocks functionality.
docs/openapi/openapi.swagger.yaml (1)
60461-60463: New Property Definition forpending_ballots_buffer_blocks:
The new property is added with a type ofstringand a format ofint64. This matches the pattern already used for other large numeric values (e.g.,block_reward_amount). Please verify that encoding this numeric value as a string is an intentional decision for handling large integers and potential precision issues.proto/zetachain/zetacore/emissions/params.proto (2)
16-16: Documentation Sample Update for PendingBallotsBufferBlocks
The added comment line clearly illustrates a sample value for the new parameter. This enhances developer clarity and documentation consistency with other sample values.
31-31: Field Addition: pending_ballots_buffer_blocks
The new field has been correctly introduced as an int64 with field number 12 within the Params message. It follows the existing naming conventions and aligns with the documented sample value. Ensure that any corresponding validation and default initialization in the module’s Go code are updated accordingly.changelog.md (1)
24-24: Changelog Entry for New Buffer Blocks Parameter
The changelog entry clearly captures the addition of the new buffer blocks parameter to delay deletion of pending ballots. It is concise and aligns with the PR objectives.
|
This should close #3511? |
Yeah it should fix it , |
gartnera
added
the
CONSENSUS_BREAKING_ACK
…ballots (#3553) * add buffer blocks for pending ballots * add comments and refactor BuildRewardsDistribution function. * add changelog * update comments * update comments * update comments * explicitly set updated params * add additional checks for migration * refactor to single ClearFinalizedMaturedBallots with forceDeleteAllBallots flag * remove SecondsToBlock function * add pendingBufferBlocks to proposal * format files
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Description
Closes : #3555
Params after update on localnet
How Has This Been Tested?
Summary by CodeRabbit
New Features
Refactor
Tests
Chores