fix: improve observer emissions distribution#3904
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 📝 WalkthroughWalkthroughThis update refines the observer emissions reward distribution mechanism. The reward calculation logic is clarified and made more explicit, with improved separation between reward unit accumulation and final allocation. Related tests are updated to match the new logic, and method signatures are adjusted for clarity and correctness. Extensive comments are added to enhance maintainability. Changes
Sequence Diagram(s)sequenceDiagram
participant EmissionsModule
participant Ballot
participant Observer
EmissionsModule->>Ballot: BuildRewardsDistribution(rewardsMap)
Ballot-->>EmissionsModule: updates rewardsMap (no return value)
EmissionsModule->>EmissionsModule: Calculate total positive reward units
EmissionsModule->>EmissionsModule: Compute reward per unit
loop For each Observer
EmissionsModule->>Observer: Distribute reward or apply slash based on reward units
end
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3904 +/- ##
========================================
Coverage 64.87% 64.88%
========================================
Files 470 470
Lines 34400 34408 +8
========================================
+ Hits 22316 22324 +8
Misses 11051 11051
Partials 1033 1033
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
x/emissions/abci.go (2)
188-201: Move the explanatory block into proper GoDoc or a standalone design noteThe new comment block is very helpful, yet it sits in-line inside the critical loop.
Consider extracting these explanations into:
- A package-level comment in
doc.go, or- A well-named helper that carries its own GoDoc (e.g.
applyBallotRewards).This keeps the core loop compact and prevents future drift between code and commentary.
215-217: Unallocated remainder silently stays in the pool – decide on an explicit policyInteger division (
Quo) guarantees the contract never over-allocates, but it also leaves a residualamount % totalRewardsUnitsundistributed.
Over time, these dust amounts can accumulate to noticeable sums and distort accounting.Options:
- Carry the remainder forward into the next block’s
UndistributedObserverRewardsPool(explicitly log the rollover).- Distribute the remainder to the observers with the highest positive units (deterministically, e.g. first in
sortedKeys).- Burn it if the protocol economics allow.
Whichever approach you choose, codify it so the intention is clear and auditable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
changelog.md(1 hunks)x/emissions/abci.go(2 hunks)x/emissions/abci_test.go(2 hunks)x/observer/types/ballot.go(1 hunks)x/observer/types/ballot_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/types/ballot_test.gox/emissions/abci_test.gox/observer/types/ballot.gox/emissions/abci.go
🧬 Code Graph Analysis (1)
x/observer/types/ballot.go (1)
testutil/sample/observer.go (1)
Ballot(21-34)
🔇 Additional comments (7)
changelog.md (1)
52-52: Well-formatted changelog entry.The changelog entry clearly communicates the purpose of this PR - improving observer emissions distribution to maximize pool utilization.
x/observer/types/ballot_test.go (2)
441-441: Improved test readability.The added blank line enhances code readability by visually separating the test case fields.
555-556: Test correctly adapted to method signature change.The test has been properly updated to reflect the new
BuildRewardsDistributionmethod signature, which no longer returns a total reward count. The test now correctly focuses on verifying the map state changes.x/emissions/abci_test.go (2)
463-463: Clearer test case name.The updated test name more accurately describes the behavior being tested - that withdrawable emissions are added only for correct votes, rather than remaining unchanged when rewards and slashes are equal.
483-485: Updated expected values to match new distribution logic.The test now correctly expects higher reward values (122 instead of 120) for observers with correct votes, reflecting the improved reward distribution mechanism where slashed amounts are redistributed to observers who voted correctly.
x/observer/types/ballot.go (2)
96-100: Improved method signature to focus on side effects.The
BuildRewardsDistributionmethod now appropriately focuses on its primary responsibility - updating the rewards map. Removing the return value clarifies the method's purpose and decouples reward unit accumulation from map updates, aligning with the single responsibility principle.
114-116: Enhanced code clarity with explicit comment.The added comment clarifies that observers who did not vote (
NotVoted) are penalized similarly to those who voted incorrectly. This makes the intention of the code explicit and improves maintainability.
ws4charlie
left a comment
ws4charlie
left a comment
There was a problem hiding this comment.
the new logic looks correct
# Conflicts: # changelog.md
5 participants
Description
Closes : https://github.com/zeta-chain/protocol-private/issues/247
Improve reward pool utilization to distribute all rewards allocated for emissions to the observers .
Older logic
The portion of rewards allocated to the observers who are being slashed was retained by the protocol.
Newer logic
The portion of rewards allocated to the observers who are being slashed is distributed to the other observers therby increasing thier earning slightly
How Has This Been Tested?
Summary by CodeRabbit
Bug Fixes
Documentation
Tests