feat: initial rewards calculation formulas #1260
Conversation
📝 WalkthroughWalkthroughThis PR adds end-to-end reward calculation features: new core types (AdaPots, RewardParameters, RewardSnapshot, PoolRewards, RewardCalculationResult, PoolRetirementInfo), complete reward calculation and helper functions (CalculateAdaPots, CalculateRewards, eta/pool performance/share/saturation helpers, distribution logic), a RewardService that orchestrates epoch reward calculation and pot updates, a RewardState interface embedded into LedgerState, updates to the mock ledger state to satisfy the new interface, and extensive tests validating formulas, reference scenarios, edge cases, and an integration-style test using the mock ledger. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
wolf31o2
force-pushed
the
feat/initial-rewards
branch
2 times, most recently
from
3816143 to
f884773
Compare
![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 (12)
ledger/common/state.go (1)
41-58: RewardState shape looks coherent; consider future extensibility around parametersThe RewardState interface and its embedding into LedgerState align cleanly with the new RewardService and mocks:
CalculateRewards,GetAdaPots,UpdateAdaPots, andGetRewardSnapshotare exactly what the service needs. The only thing to keep in mind is thatCalculateEpochRewardscurrently hardcodesRewardParametersinside the service; if you later move protocol parameters into ledger state, you may want either an additional interface (e.g.,ProtocolState) or a method here to fetch them rather than constructing defaults in the service.Also applies to: 66-66
internal/test/ledger/ledger.go (1)
29-31: MockLedgerState correctly implements RewardState; clarify UpdateAdaPots semanticsThe added fields and methods give
MockLedgerStatefullRewardStatecoverage and delegate reward calculation to the productionCalculateRewards, which is useful for integration-style tests. Note thatUpdateAdaPotsis intentionally a no-op; if any tests start relying on pots mutation through this mock (similar to the localmockLedgerStateinledger/common/rewards_test.go), they will need a stateful implementation here as well or explicit comments indicating that only read‑only behavior is expected from this mock.Also applies to: 95-116
ledger/common/rewards_test.go (2)
28-71: Reward pot and distribution tests are well‑structured; potential future enhancementsThe
TestCalculateAdaPotsandTestCalculateRewardsscenarios line up with the formulas inCalculateAdaPotsandCalculateRewardsand assert the key invariants (pot movements, full pot depletion, and distribution sum). Once the core formulas settle, you might consider tightening these tests by also asserting specific per‑pool values (or at least relative proportions) to catch regressions in share calculations, not just conservation of the total.Also applies to: 73-147
212-220: Handle possible JSON unmarshal failure in createGenesisRat
createGenesisRatignores the error fromUnmarshalJSON. If the JSON shape ever changes, tests may silently use a zero value instead of failing loudly, making debugging harder. Since this is test‑only code, panicking on error is acceptable and keeps failures obvious.func createGenesisRat(num, denom int64) GenesisRat { var r GenesisRat jsonData := []byte( fmt.Sprintf(`{"numerator": %d, "denominator": %d}`, num, denom), ) - r.UnmarshalJSON(jsonData) + if err := r.UnmarshalJSON(jsonData); err != nil { + panic(fmt.Sprintf("failed to unmarshal GenesisRat: %v", err)) + } return r }ledger/common/rewards_validation_test.go (2)
25-90: Spec helper formulas are encoded correctly; consider using them to assert on production results
TestShelleyRewardFormulasplus thecalculateLeaderReward/calculateMemberRewardhelpers implement the Shelley/Amaru reward formulas in a clear, numeric way and validate them against hand‑computed expectations. Right now these helpers are only used to test themselves; as a follow‑up, it may be worth using them to derive expected leader/member results from a smallRewardSnapshotand compare those against whatCalculateRewards/distributePoolRewardsproduce for a single pool, to directly pin production behavior to the spec.Also applies to: 339-371
222-337: Validation and compatibility tests provide strong coverage; some duplication is acceptableThe edge‑case tests (
TestRewardEdgeCases) and the Java/Amaru compatibility suites do a good job of pinning behavior around high margins, zero‑reward pools, owner‑only delegation, eta calculation, pool performance, and monetary expansion. There is some intentional duplication between the “MonetaryExpansion_With_Eta” and “MonetaryExpansion_With_Eta_Amaru” subtests, but they serve as regression tests from different reference perspectives; if this starts to feel noisy later, you can always factor a shared helper for the common numeric scenario without changing coverage.Also applies to: 373-681
ledger/common/rewards_service.go (2)
22-83: Epoch reward workflow is coherent; protocol parameters are currently hardcoded
RewardService.CalculateEpochRewardswires togetherGetAdaPots,GetRewardSnapshot,CalculateRewards, andUpdateAdaPotsin a straightforward way, with good error wrapping. The inlinedRewardParameters(rho, tau, a₀) are clearly marked as temporary defaults; when you later fetch real protocol parameters from the ledger, this method should adapt cleanly because all dependencies are already abstracted behindLedgerState.
85-112: ValidateRewardDistribution only checks total sum; document or extend as needed
ValidateRewardDistributioncurrently verifies that the sum of all actual rewards matches the sum of per‑poolTotalRewards, which is a useful coarse check but doesn’t validate per‑address correctness. That’s fine as a first step, especially given the comment about MIR certificates, but it might be worth either documenting this explicitly or extending it later to aggregate expected rewards byAddrKeyHashand compare address‑by‑address once the distribution mechanism is finalized.ledger/common/rewards.go (4)
347-383: Reuse saturation helper and guard against pathological parameter values incalculatePoolShareThe core logic is fine, but a couple of robustness points:
- Saturation is hard-coded as
stakeRatio/0.05(5%) while you also exposeCalculatePoolSaturationwith a configurablesaturationPoint. Consider reusing that helper or a shared constant so you don’t end up with conflicting saturation definitions.a0Float, _ := a0.Float64()andmarginFloat(margin)assume reasonable parameter ranges. Extremely large/negativea0ormargin > 1could makedenominator <= 0ornumerator < 0, which then produce negative or infinite shares. Even if inputs are supposed to be constrained, a defensive clamp helps keep the math sane.Example defensive tweaks:
- saturation := math.Min(stakeRatio/0.05, 1.0) // Assuming 5% saturation point + saturation := math.Min(stakeRatio/0.05, 1.0) // TODO: consider wiring a param or helper @@ - a0Float, _ := a0.Float64() - denominator := 1.0 + a0Float*saturation + a0Float, _ := a0.Float64() + if a0Float < 0 { + a0Float = 0 + } + denominator := 1.0 + a0Float*saturation + if denominator <= 0 { + return 0 + }
385-455: Clarify leftover handling and deregistration semantics indistributePoolRewardsThe high-level operator/delegator split looks good, but a few behavioral points are worth tightening:
- Rounding: Delegator rewards are computed via float math and truncated to
uint64, sosum(delegatorRewards)may be less thanstakeholderRewardsTotal. The leftovers are silently dropped butTotalRewardsis still reported astotalPoolRewards. If you care about conservation, you might want to track the actually assigned sum or redirect the remainder (e.g., to treasury or operator).- Deregistrations: Only
StakeRegistrationsis consulted. The snapshot also carriesEarlyDeregistrationsandLateDeregistrations, but they aren’t used here. If protocol rules require different handling for those sets (e.g., redirecting their rewards to reserves/treasury), that logic is currently missing from the distribution layer.If you want to preserve all rewards per pool, one simple option is to assign any rounding remainder to the operator:
- if totalPoolStake > 0 && stakeholderRewardsTotal > 0 { + assigned := uint64(0) + if totalPoolStake > 0 && stakeholderRewardsTotal > 0 { for stakeKey, stake := range delegatorStake { @@ ) - delegatorRewards[stakeKey] = reward + delegatorRewards[stakeKey] = reward + assigned += reward } } + + if stakeholderRewardsTotal > assigned { + operatorRewards += stakeholderRewardsTotal - assigned + }
475-487:CalculateOptimalPoolCountstub is fine for now but should be clearly marked as simplifiedThe implementation currently ignores
totalActiveStake(using_is good) and just returnsk, which matches the comment about simplification. If this is intentional as a placeholder, you might want to add a TODO so callers don’t over-interpret the result as mathematically “optimal”.Example:
-// totalActiveStake is unused as current implementation is simplified and only uses k parameter +// totalActiveStake is unused as current implementation is simplified and only uses k parameter. +// TODO: implement real optimal pool count formula when requirements are finalized.
512-529:CalculatePoolSaturationbehavior for small stakes and zero saturation pointThe implementation is straightforward and numerically safe. A couple of behavioral notes:
- When
saturationPointStake == 0(e.g., tinytotalActiveStakeor misconfiguredsaturationPoint), you return1.0(“fully saturated”), which is a reasonable fail-safe but might be surprising—documenting this edge case would help.- You’re using a percent-style
saturationPointhere, butcalculatePoolSharehard-codes 5% via0.05. Over time, it’d be good to align both on the same configuration source to avoid divergent definitions.Otherwise, this helper looks fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/test/ledger/ledger.go(2 hunks)ledger/common/rewards.go(1 hunks)ledger/common/rewards_service.go(1 hunks)ledger/common/rewards_test.go(1 hunks)ledger/common/rewards_validation_test.go(1 hunks)ledger/common/state.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
ledger/common/state.go (1)
ledger/common/rewards.go (5)
CalculateRewards(240-345)AdaPots(26-30)RewardSnapshot(64-99)RewardParameters(33-61)RewardCalculationResult(108-117)
internal/test/ledger/ledger.go (1)
ledger/common/rewards.go (5)
AdaPots(26-30)RewardSnapshot(64-99)CalculateRewards(240-345)RewardParameters(33-61)RewardCalculationResult(108-117)
ledger/common/rewards_service.go (2)
ledger/common/state.go (1)
LedgerState(61-68)ledger/common/rewards.go (4)
RewardCalculationResult(108-117)RewardParameters(33-61)CalculateRewards(240-345)PoolRewards(120-129)
ledger/common/rewards_test.go (5)
ledger/common/rewards.go (9)
AdaPots(26-30)RewardParameters(33-61)CalculateAdaPots(132-181)RewardSnapshot(64-99)CalculateRewards(240-345)PoolRewards(120-129)ValidatePoolPledge(491-510)CalculatePoolSaturation(513-529)RewardCalculationResult(108-117)ledger/common/certs.go (1)
PoolKeyHash(331-331)cbor/tags.go (1)
Rat(94-96)ledger/common/common.go (1)
GenesisRat(523-523)ledger/common/rewards_service.go (1)
NewRewardService(28-32)
ledger/common/rewards_validation_test.go (2)
ledger/common/rewards.go (7)
AdaPots(26-30)RewardSnapshot(64-99)RewardParameters(33-61)CalculateRewards(240-345)PoolRewards(120-129)CalculateAdaPots(132-181)PoolRetirementInfo(102-105)ledger/common/certs.go (1)
PoolKeyHash(331-331)
ledger/common/rewards.go (4)
ledger/common/pparams.go (1)
ProtocolParametersProtocolVersion(29-33)cbor/tags.go (1)
Rat(94-96)ledger/common/certs.go (1)
PoolKeyHash(331-331)ledger/common/common.go (1)
GenesisRat(523-523)
🔇 Additional comments (5)
ledger/common/rewards_test.go (2)
149-210: Margin, pledge, and saturation tests give good coverageThe margin tests (
TestMarginFloat) and the pledge/saturation checks (TestValidatePoolPledge,TestCalculatePoolSaturation) closely mirror the intended semantics of the underlying helpers (zero/no‑rat margin, owner stake vs pledge, saturation capped at 1). These should be useful guardrails against subtle changes in those utilities.Also applies to: 163-183
222-304: RewardServiceIntegration + local mockLedgerState are consistent with RewardService designThe
TestRewardServiceIntegrationflow, together with the localmockLedgerState, exercises the fullRewardService.CalculateEpochRewardspath: reading pots and snapshot, delegating toCalculateRewards, updating pots, and validating total distribution. The mock’s mutableadaPotsandGetAdaPots/UpdateAdaPotsimplementations accurately simulate the state update the service expects, and the test assertions (reward count, total rewards, updated pots.Rewards == 0) match the production behavior.Also applies to: 306-363
ledger/common/rewards.go (3)
25-129: Type definitions look coherent and match the intended domainThe
AdaPots,RewardParameters,RewardSnapshot,PoolRetirementInfo,RewardCalculationResult, andPoolRewardsstructs are well-organized and give you a clear, extendable shape for reward calculation. Field types and comments line up with the domain, and maps are keyed consistently onPoolKeyHash/AddrKeyHash.No issues here from a correctness or Go-idiom perspective.
458-473:marginFloatis robust but can be simplified slightlyThe zero-margin guard and nil checks make this safe even with the
cbor.Ratwrapper, and usingbig.Ratas an intermediate is fine. GivenGenesisRatis an alias, you could optionally simplify the zero check to something closer to the underlying representation if desired, but that’s stylistic.Logic-wise this looks correct.
489-510:ValidatePoolPledgeimplementation looks correct and idiomaticThis function cleanly:
- Handles nil
poolParamsby returningfalse.- Sums owner stake from the provided
ownerStakemap (safe even if the map is nil).- Compares against
poolParams.Pledgeas expected.No changes needed.
| // CalculateAdaPots calculates the ADA pots for the next epoch | ||
| func CalculateAdaPots( | ||
| currentPots AdaPots, | ||
| params RewardParameters, | ||
| totalActiveStake uint64, | ||
| epochFees uint64, | ||
| totalBlocksInEpoch uint32, | ||
| ) AdaPots { | ||
| // Calculate eta (pool performance factor) | ||
| eta := calculateEta(totalBlocksInEpoch, params) | ||
|
|
||
| // Calculate monetary expansion (rho * eta * reserves) | ||
| // rho is in millionths (3000 = 0.3%) | ||
| monetaryExpansion := new(big.Int).SetUint64(params.MonetaryExpansion) | ||
| reserves := new(big.Int).SetUint64(currentPots.Reserves) | ||
| expansion := new(big.Int).Mul(monetaryExpansion, reserves) | ||
| expansion.Div( | ||
| expansion, | ||
| new(big.Int).SetUint64(1000000), | ||
| ) // Convert from millionths | ||
|
|
||
| // Apply eta factor | ||
| etaBig := eta | ||
| expansion = new(big.Int).Mul(expansion, etaBig.Num()) | ||
| expansion.Div(expansion, etaBig.Denom()) | ||
|
|
||
| // Add epoch fees to reward pot | ||
| rewardPot := new(big.Int).SetUint64(epochFees) | ||
| rewardPot.Add(rewardPot, expansion) | ||
|
|
||
| // Calculate treasury contribution (tau * (expansion + fees)) | ||
| // tau is in ten-thousandths (2000 = 20%) | ||
| treasuryGrowth := new(big.Int).SetUint64(params.TreasuryGrowth) | ||
| treasuryContribution := new(big.Int).Mul(treasuryGrowth, rewardPot) | ||
| treasuryContribution.Div( | ||
| treasuryContribution, | ||
| new(big.Int).SetUint64(10000), | ||
| ) // Convert from ten-thousandths | ||
|
|
||
| // Subtract treasury contribution from reward pot | ||
| rewardPot.Sub(rewardPot, treasuryContribution) | ||
|
|
||
| // Update pots | ||
| newPots := AdaPots{ | ||
| Reserves: currentPots.Reserves - expansion.Uint64(), | ||
| Treasury: currentPots.Treasury + treasuryContribution.Uint64(), | ||
| Rewards: rewardPot.Uint64(), | ||
| } | ||
|
|
||
| return newPots | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Fix unused parameter and guard against reserve underflow in CalculateAdaPots
totalActiveStakeis not used anywhere in the function body. In Go this is a compile error, so either use it or change the parameter name to_until it’s needed.Reserves: currentPots.Reserves - expansion.Uint64()can underflow if parameters are misconfigured such thatexpansion > currentPots.Reserves. That will wrap theuint64and silently produce a huge reserves value.
Consider:
-func CalculateAdaPots(
- currentPots AdaPots,
- params RewardParameters,
- totalActiveStake uint64,
- epochFees uint64,
- totalBlocksInEpoch uint32,
-) AdaPots {
+func CalculateAdaPots(
+ currentPots AdaPots,
+ params RewardParameters,
+ _ uint64, // totalActiveStake (currently unused)
+ epochFees uint64,
+ totalBlocksInEpoch uint32,
+) AdaPots {
@@
- newPots := AdaPots{
- Reserves: currentPots.Reserves - expansion.Uint64(),
- Treasury: currentPots.Treasury + treasuryContribution.Uint64(),
- Rewards: rewardPot.Uint64(),
- }
+ expansionU64 := expansion.Uint64()
+ reserves := currentPots.Reserves
+ if expansionU64 > reserves {
+ expansionU64 = reserves
+ }
+ newPots := AdaPots{
+ Reserves: reserves - expansionU64,
+ Treasury: currentPots.Treasury + treasuryContribution.Uint64(),
+ Rewards: rewardPot.Uint64(),
+ }I'll verify the review comment by examining the actual codebase. Let me search for the relevant code and confirm the issues.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine the CalculateAdaPots function in rewards.go
find . -name "rewards.go" -path "*/common/*" -type f 2>/dev/null | head -5
Length of output: 26
🏁 Script executed:
#!/bin/bash
# Search for the CalculateAdaPots function to verify parameter usage
rg -n "func CalculateAdaPots" --type go -A 50Length of output: 3033
🏁 Script executed:
#!/bin/bash
# Check if totalActiveStake is referenced anywhere in the function
rg -n "totalActiveStake" --type go -B 5 -A 5Length of output: 3583
🏁 Script executed:
#!/bin/bash
# Look for AdaPots struct definition to understand the type
rg -n "type AdaPots" --type go -A 10Length of output: 740
🏁 Script executed:
#!/bin/bash
# Check for any guards or validation logic around reserve calculations
rg -n "Reserves.*-" --type go -B 3 -A 3 ledger/common/Length of output: 861
Fix unused parameter and guard against reserve underflow in CalculateAdaPots
The totalActiveStake parameter at line 135 is unused and will cause a Go compilation error. Additionally, line 175 lacks underflow protection: if expansion.Uint64() exceeds currentPots.Reserves, the uint64 subtraction silently wraps around, corrupting the reserves value.
-func CalculateAdaPots(
- currentPots AdaPots,
- params RewardParameters,
- totalActiveStake uint64,
- epochFees uint64,
- totalBlocksInEpoch uint32,
-) AdaPots {
+func CalculateAdaPots(
+ currentPots AdaPots,
+ params RewardParameters,
+ _ uint64, // totalActiveStake (currently unused)
+ epochFees uint64,
+ totalBlocksInEpoch uint32,
+) AdaPots {
@@
- newPots := AdaPots{
- Reserves: currentPots.Reserves - expansion.Uint64(),
- Treasury: currentPots.Treasury + treasuryContribution.Uint64(),
- Rewards: rewardPot.Uint64(),
- }
+ expansionU64 := expansion.Uint64()
+ reserves := currentPots.Reserves
+ if expansionU64 > reserves {
+ expansionU64 = reserves
+ }
+ newPots := AdaPots{
+ Reserves: reserves - expansionU64,
+ Treasury: currentPots.Treasury + treasuryContribution.Uint64(),
+ Rewards: rewardPot.Uint64(),
+ }📝 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.
| // CalculateAdaPots calculates the ADA pots for the next epoch | |
| func CalculateAdaPots( | |
| currentPots AdaPots, | |
| params RewardParameters, | |
| totalActiveStake uint64, | |
| epochFees uint64, | |
| totalBlocksInEpoch uint32, | |
| ) AdaPots { | |
| // Calculate eta (pool performance factor) | |
| eta := calculateEta(totalBlocksInEpoch, params) | |
| // Calculate monetary expansion (rho * eta * reserves) | |
| // rho is in millionths (3000 = 0.3%) | |
| monetaryExpansion := new(big.Int).SetUint64(params.MonetaryExpansion) | |
| reserves := new(big.Int).SetUint64(currentPots.Reserves) | |
| expansion := new(big.Int).Mul(monetaryExpansion, reserves) | |
| expansion.Div( | |
| expansion, | |
| new(big.Int).SetUint64(1000000), | |
| ) // Convert from millionths | |
| // Apply eta factor | |
| etaBig := eta | |
| expansion = new(big.Int).Mul(expansion, etaBig.Num()) | |
| expansion.Div(expansion, etaBig.Denom()) | |
| // Add epoch fees to reward pot | |
| rewardPot := new(big.Int).SetUint64(epochFees) | |
| rewardPot.Add(rewardPot, expansion) | |
| // Calculate treasury contribution (tau * (expansion + fees)) | |
| // tau is in ten-thousandths (2000 = 20%) | |
| treasuryGrowth := new(big.Int).SetUint64(params.TreasuryGrowth) | |
| treasuryContribution := new(big.Int).Mul(treasuryGrowth, rewardPot) | |
| treasuryContribution.Div( | |
| treasuryContribution, | |
| new(big.Int).SetUint64(10000), | |
| ) // Convert from ten-thousandths | |
| // Subtract treasury contribution from reward pot | |
| rewardPot.Sub(rewardPot, treasuryContribution) | |
| // Update pots | |
| newPots := AdaPots{ | |
| Reserves: currentPots.Reserves - expansion.Uint64(), | |
| Treasury: currentPots.Treasury + treasuryContribution.Uint64(), | |
| Rewards: rewardPot.Uint64(), | |
| } | |
| return newPots | |
| } | |
| // CalculateAdaPots calculates the ADA pots for the next epoch | |
| func CalculateAdaPots( | |
| currentPots AdaPots, | |
| params RewardParameters, | |
| _ uint64, // totalActiveStake (currently unused) | |
| epochFees uint64, | |
| totalBlocksInEpoch uint32, | |
| ) AdaPots { | |
| // Calculate eta (pool performance factor) | |
| eta := calculateEta(totalBlocksInEpoch, params) | |
| // Calculate monetary expansion (rho * eta * reserves) | |
| // rho is in millionths (3000 = 0.3%) | |
| monetaryExpansion := new(big.Int).SetUint64(params.MonetaryExpansion) | |
| reserves := new(big.Int).SetUint64(currentPots.Reserves) | |
| expansion := new(big.Int).Mul(monetaryExpansion, reserves) | |
| expansion.Div( | |
| expansion, | |
| new(big.Int).SetUint64(1000000), | |
| ) // Convert from millionths | |
| // Apply eta factor | |
| etaBig := eta | |
| expansion = new(big.Int).Mul(expansion, etaBig.Num()) | |
| expansion.Div(expansion, etaBig.Denom()) | |
| // Add epoch fees to reward pot | |
| rewardPot := new(big.Int).SetUint64(epochFees) | |
| rewardPot.Add(rewardPot, expansion) | |
| // Calculate treasury contribution (tau * (expansion + fees)) | |
| // tau is in ten-thousandths (2000 = 20%) | |
| treasuryGrowth := new(big.Int).SetUint64(params.TreasuryGrowth) | |
| treasuryContribution := new(big.Int).Mul(treasuryGrowth, rewardPot) | |
| treasuryContribution.Div( | |
| treasuryContribution, | |
| new(big.Int).SetUint64(10000), | |
| ) // Convert from ten-thousandths | |
| // Subtract treasury contribution from reward pot | |
| rewardPot.Sub(rewardPot, treasuryContribution) | |
| // Update pots | |
| expansionU64 := expansion.Uint64() | |
| reserves := currentPots.Reserves | |
| if expansionU64 > reserves { | |
| expansionU64 = reserves | |
| } | |
| newPots := AdaPots{ | |
| Reserves: reserves - expansionU64, | |
| Treasury: currentPots.Treasury + treasuryContribution.Uint64(), | |
| Rewards: rewardPot.Uint64(), | |
| } | |
| return newPots | |
| } |
🤖 Prompt for AI Agents
In ledger/common/rewards.go around lines 131–181: the parameter totalActiveStake
is unused (causes a compile error) and reserves subtraction can underflow if
expansion > currentPots.Reserves. Remove totalActiveStake from the function
signature (and update all call sites) or else rename it to _ if you
intentionally want to keep it, and replace the uint64 subtraction with a safe
big.Int-based subtraction: convert currentPots.Reserves to a big.Int, compute
reservesMinusExpansion = reservesBig.Sub(reservesBig, expansion) and if
reservesMinusExpansion is negative set new reserves to 0, otherwise set
newPots.Reserves to reservesMinusExpansion.Uint64(); keep other fields assigned
from the computed big.Int results.
| // calculateEta calculates the pool performance factor (η) | ||
| // Following Amaru's approach: eta = min(1, blocks_produced / (epoch_length * active_slot_coeff)) | ||
| func calculateEta(totalBlocksInEpoch uint32, params RewardParameters) *big.Rat { | ||
| // Calculate expected blocks: epoch_length * active_slot_coeff | ||
| expectedBlocks := new(big.Rat).Mul( | ||
| new(big.Rat).SetUint64(uint64(params.ExpectedSlotsPerEpoch)), | ||
| params.ActiveSlotsCoeff, | ||
| ) | ||
|
|
||
| // eta = min(1, totalBlocksInEpoch / expectedBlocks) | ||
| actualBlocks := new(big.Rat).SetUint64(uint64(totalBlocksInEpoch)) | ||
| eta := new(big.Rat).Quo(actualBlocks, expectedBlocks) | ||
|
|
||
| // Cap at 1.0 | ||
| oneRat := new(big.Rat).SetInt64(1) | ||
| if eta.Cmp(oneRat) > 0 { | ||
| eta = oneRat | ||
| } | ||
|
|
||
| return eta | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Handle zero/invalid ExpectedSlotsPerEpoch or ActiveSlotsCoeff in calculateEta
calculateEta assumes:
params.ExpectedSlotsPerEpoch > 0params.ActiveSlotsCoeff != niland not zero
If either is zero/nil, expectedBlocks becomes zero or the Mul uses a nil *big.Rat, and eta := actualBlocks / expectedBlocks risks invalid math (division by zero) or a panic depending on big.Rat behavior.
I’d defensively guard:
func calculateEta(totalBlocksInEpoch uint32, params RewardParameters) *big.Rat {
- // Calculate expected blocks: epoch_length * active_slot_coeff
- expectedBlocks := new(big.Rat).Mul(
- new(big.Rat).SetUint64(uint64(params.ExpectedSlotsPerEpoch)),
- params.ActiveSlotsCoeff,
- )
+ if params.ActiveSlotsCoeff == nil || params.ExpectedSlotsPerEpoch == 0 {
+ // Fall back to eta = 1.0 if config is missing/invalid
+ return new(big.Rat).SetInt64(1)
+ }
+ // Calculate expected blocks: epoch_length * active_slot_coeff
+ expectedBlocks := new(big.Rat).Mul(
+ new(big.Rat).SetUint64(uint64(params.ExpectedSlotsPerEpoch)),
+ params.ActiveSlotsCoeff,
+ )
@@
- eta := new(big.Rat).Quo(actualBlocks, expectedBlocks)
+ if expectedBlocks.Sign() == 0 {
+ return new(big.Rat).SetInt64(1)
+ }
+ eta := new(big.Rat).Quo(actualBlocks, expectedBlocks)I'll verify the concerns raised in this review comment by examining the codebase structure and understanding the actual constraints on the parameters.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find RewardParameters struct definition
rg -nP 'type\s+RewardParameters\s+(struct|interface)' --type=go -A 10
Length of output: 731
🏁 Script executed:
#!/bin/bash
# Find where ExpectedSlotsPerEpoch is set/initialized
rg -nP 'ExpectedSlotsPerEpoch\s*[:=]' --type=go -B 2 -A 2Length of output: 1822
🏁 Script executed:
#!/bin/bash
# Find where ActiveSlotsCoeff is set/initialized
rg -nP 'ActiveSlotsCoeff\s*[:=]' --type=go -B 2 -A 2Length of output: 2744
🏁 Script executed:
#!/bin/bash
# Find all calls to calculateEta to understand usage context
rg -nP 'calculateEta\s*\(' --type=go -B 3 -A 3Length of output: 3862
🏁 Script executed:
#!/bin/bash
# Search for validation logic in the rewards package
rg -nP 'func.*[Vv]alidate.*Param' --type=go -A 10Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the complete RewardParameters struct definition
rg -nP 'type\s+RewardParameters\s+struct' --type=go -A 20
Length of output: 1197
🏁 Script executed:
#!/bin/bash
# Find where CalculateAdaPots is called - this is the production caller
rg -nP 'CalculateAdaPots\s*\(' --type=go -B 5 -A 5Length of output: 3675
🏁 Script executed:
#!/bin/bash
# Look for constructor functions or factory patterns for RewardParameters
rg -nP 'func.*RewardParameters|NewRewardParameters' --type=go -A 8Length of output: 659
🏁 Script executed:
#!/bin/bash
# Search for any initialization or setup of RewardParameters outside tests
rg -nP 'RewardParameters\s*{' --type=go | grep -v _test.goLength of output: 64
🏁 Script executed:
#!/bin/bash
# Check if there are any tests that specifically test zero or nil cases
rg -nP 'ExpectedSlotsPerEpoch.*0|ActiveSlotsCoeff.*nil' --type=goLength of output: 418
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full RewardParameters struct - need to see ActiveSlotsCoeff and ExpectedSlotsPerEpoch fields
rg -nP 'type\s+RewardParameters\s+struct' --type=go -A 40
Length of output: 2259
🏁 Script executed:
#!/bin/bash
# Examine the production initialization in rewards_service.go
cat ledger/common/rewards_service.go | sed -n '45,75p'Length of output: 713
🏁 Script executed:
#!/bin/bash
# Check if ActiveSlotsCoeff and ExpectedSlotsPerEpoch are explicitly defined in struct
rg -nP '(ActiveSlotsCoeff|ExpectedSlotsPerEpoch)\s*\*?big\.(Rat|Int)' --type=go -B 1 -A 1Length of output: 157
🏁 Script executed:
#!/bin/bash
# Look for any validation of these parameters
rg -nP 'ActiveSlotsCoeff|ExpectedSlotsPerEpoch' ledger/common/rewards.go | head -20Length of output: 161
🏁 Script executed:
#!/bin/bash
# Check the full context of rewards_service.go to understand if it's incomplete
cat ledger/common/rewards_service.goLength of output: 2959
🏁 Script executed:
#!/bin/bash
# Search for where CalculateRewards is defined or if RewardParameters is properly initialized elsewhere
rg -nP 'CalculateRewards|func.*RewardParameters' --type=go -B 2 -A 8Length of output: 6793
🏁 Script executed:
#!/bin/bash
# Search for any code that sets these fields after initialization
rg -nP 'params\.(ActiveSlotsCoeff|ExpectedSlotsPerEpoch)\s*=' --type=goLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look for default values or initialization patterns
rg -nP 'ActiveSlotsCoeff\s*=' ledger/common/rewards.go -B 5 -A 5Length of output: 0
🌐 Web query:
Go big.Rat Mul behavior with nil pointer
💡 Result:
Short answer: don’t pass nil pointers. (*big.Rat).Mul expects pointer operands and a non-nil receiver; if z, x or y is a nil *big.Rat you will get a runtime nil-pointer dereference (panic). The docs show Mul has signature func (z *Rat) Mul(x, y *Rat) *Rat and the package treats the zero value of a Rat (not a nil *Rat) as 0 — you must allocate a Rat (e.g. new(big.Rat) or big.NewRat) before calling Mul. [1][2]. For the general rule on nil pointer deref panics see the Go nil-pointer behavior. [3]
Sources:
- math/big package docs (Mul and zero-value note). [1]
- math/big docs (Denom/zero-value vs uninitialized note). [2]
- Go nil-pointer dereference explanation. [3]
---
🏁 Script executed:
```shell
#!/bin/bash
# Verify CalculateRewards calls CalculateAdaPots which calls calculateEta
rg -nP 'CalculateAdaPots\s*\(' ledger/common/rewards.go -B 3 -A 3
Length of output: 190
🏁 Script executed:
#!/bin/bash
# Check if calculateEta is called from CalculateAdaPots
sed -n '239,280p' ledger/common/rewards.goLength of output: 1063
Add defensive guards in calculateEta to handle nil/zero ActiveSlotsCoeff and ExpectedSlotsPerEpoch
Production code in rewards_service.go initializes RewardParameters without setting ActiveSlotsCoeff and ExpectedSlotsPerEpoch, leaving them at their zero values (nil and 0 respectively). The calculateEta function at lines 187–189 calls new(big.Rat).Mul() directly on params.ActiveSlotsCoeff without a nil check. big.Rat.Mul with a nil operand causes a runtime nil-pointer dereference panic. Additionally, if ExpectedSlotsPerEpoch is 0, the resulting expectedBlocks will be zero, leading to division by zero in the Quo call at line 196.
Apply the suggested defensive checks:
func calculateEta(totalBlocksInEpoch uint32, params RewardParameters) *big.Rat {
+ if params.ActiveSlotsCoeff == nil || params.ExpectedSlotsPerEpoch == 0 {
+ // Fall back to eta = 1.0 if config is missing/invalid
+ return new(big.Rat).SetInt64(1)
+ }
// Calculate expected blocks: epoch_length * active_slot_coeff
expectedBlocks := new(big.Rat).Mul(
new(big.Rat).SetUint64(uint64(params.ExpectedSlotsPerEpoch)),
params.ActiveSlotsCoeff,
)
// eta = min(1, totalBlocksInEpoch / expectedBlocks)
actualBlocks := new(big.Rat).SetUint64(uint64(totalBlocksInEpoch))
+ if expectedBlocks.Sign() == 0 {
+ return new(big.Rat).SetInt64(1)
+ }
eta := new(big.Rat).Quo(actualBlocks, expectedBlocks)🤖 Prompt for AI Agents
In ledger/common/rewards.go around lines 183 to 203, add defensive guards before
using params.ActiveSlotsCoeff and params.ExpectedSlotsPerEpoch: if
params.ActiveSlotsCoeff is nil treat it as 0 (use a zero *big.Rat) and if
params.ExpectedSlotsPerEpoch == 0 then short-circuit and return a zero *big.Rat
to avoid division by zero and nil-pointer dereference; otherwise compute
expectedBlocks = ExpectedSlotsPerEpoch * ActiveSlotsCoeff, compute eta =
actualBlocks / expectedBlocks, cap eta at 1 as before, and return the result.
| // calculatePoolPerformance calculates the apparent performance of a pool | ||
| // Following Amaru's approach: performance = (pool_blocks / total_blocks) * (total_stake / pool_stake) | ||
| // params is unused as performance calculation only requires block production data from snapshot | ||
| func calculatePoolPerformance( | ||
| poolID PoolKeyHash, | ||
| snapshot RewardSnapshot, | ||
| _ RewardParameters, | ||
| ) float64 { | ||
| // Get blocks produced by this pool | ||
| poolBlocks := snapshot.PoolBlocks[poolID] | ||
|
|
||
| // If no total blocks, assume optimal performance | ||
| if snapshot.TotalBlocksInEpoch == 0 { | ||
| return 1.0 | ||
| } | ||
|
|
||
| // If no blocks produced, performance = 0 | ||
| if poolBlocks == 0 { | ||
| return 0.0 | ||
| } | ||
|
|
||
| // Calculate blocks ratio: pool_blocks / total_blocks | ||
| blocksRatio := float64(poolBlocks) / float64(snapshot.TotalBlocksInEpoch) | ||
|
|
||
| // Calculate stake ratio: total_stake / pool_stake | ||
| poolStake := snapshot.PoolStake[poolID] | ||
| stakeRatio := float64(snapshot.TotalActiveStake) / float64(poolStake) | ||
|
|
||
| // Performance = blocks_ratio * stake_ratio | ||
| performance := blocksRatio * stakeRatio | ||
|
|
||
| return performance | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid division by zero for zero-stake pools in calculatePoolPerformance
Here:
poolStake := snapshot.PoolStake[poolID]
stakeRatio := float64(snapshot.TotalActiveStake) / float64(poolStake)If poolStake == 0 (e.g., a pool entry created but with zero active stake), this becomes TotalActiveStake / 0, yielding +Inf in Go. That propagates into performance and later into reward shares, leading to nonsensical pool weights.
Suggest:
- poolStake := snapshot.PoolStake[poolID]
- stakeRatio := float64(snapshot.TotalActiveStake) / float64(poolStake)
+ poolStake := snapshot.PoolStake[poolID]
+ if poolStake == 0 {
+ // No stake in the pool → no meaningful performance
+ return 0.0
+ }
+ stakeRatio := float64(snapshot.TotalActiveStake) / float64(poolStake)I'll verify this review comment by examining the codebase to confirm the division-by-zero concern and validate whether the suggested fix is necessary.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the rewards.go file in the ledger common directory
fd -t f rewards.go ledger/common/
Length of output: 24
🏁 Script executed:
#!/bin/bash
# Show the exact code at lines 205-237
sed -n '205,237p' ledger/common/rewards.goLength of output: 1059
🏁 Script executed:
#!/bin/bash
# Search for calls to calculatePoolPerformance
rg -n 'calculatePoolPerformance' --type goLength of output: 641
🏁 Script executed:
#!/bin/bash
# Search for how PoolStake map is populated and whether zero entries are possible
rg -n 'PoolStake\[' --type go -C 3Length of output: 488
🏁 Script executed:
#!/bin/bash
# Check if there's any existing nil or zero-check pattern for pool stakes
rg -n 'poolStake.*== 0' --type goLength of output: 0
I need to dive deeper into the caller context and how the snapshot is populated to determine if zero-stake entries are possible.
---
🏁 Script executed:
```shell
#!/bin/bash
# Show context around line 356 where calculatePoolPerformance is called
sed -n '340,370p' ledger/common/rewards.go
Length of output: 935
🏁 Script executed:
#!/bin/bash
# Search for PoolStake map initialization and population
rg -n 'PoolStake.*=' --type go -B 2 -A 2 | head -50Length of output: 2826
🏁 Script executed:
#!/bin/bash
# Look for RewardSnapshot struct definition to understand PoolStake type
rg -n 'type RewardSnapshot' --type go -A 20Length of output: 1285
🏁 Script executed:
#!/bin/bash
# Check test cases to see if they test zero-stake scenarios
sed -n '410,440p' ledger/common/rewards_validation_test.goLength of output: 789
🏁 Script executed:
#!/bin/bash
# Check for any pool filtering or guards before calling calculatePoolPerformance
rg -n 'for.*poolID\|for.*pool' ledger/common/rewards.go -A 5 | head -60Length of output: 0
I need to check where the RewardSnapshot is constructed and whether zero-stake pools can legitimately exist.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for where RewardSnapshot is created/populated
rg -n 'RewardSnapshot\{' --type go -B 5 -A 10 | head -80
Length of output: 5693
🏁 Script executed:
#!/bin/bash
# Look for NewRewardSnapshot or similar constructor functions
rg -n 'func.*Snapshot|snapshot :=' ledger/common/rewards.go -B 2 -A 5 | head -100Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's filtering of pools or validation before reward calculation
rg -n 'PoolBlocks\[' --type go -B 3 -A 3Length of output: 1521
🏁 Script executed:
#!/bin/bash
# Search the entire context around calculatePoolShare to see if poolStake is pre-validated
sed -n '345,380p' ledger/common/rewards.goLength of output: 1191
I need to check where calculatePoolShare is called to understand if there are guards filtering out zero-stake pools upstream.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find all calls to calculatePoolShare
rg -n 'calculatePoolShare' --type go -B 3 -A 3
Length of output: 763
🏁 Script executed:
#!/bin/bash
# Look for the main rewards calculation loop that calls calculatePoolShare
rg -n 'for.*range.*PoolStake\|for.*range.*PoolParams' --type go -B 5 -A 10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Show broader context of where rewards are actually calculated
sed -n '320,360p' ledger/common/rewards.goLength of output: 970
I need to see the complete loop structure to understand the iteration and guard conditions.
---
🏁 Script executed:
```shell
#!/bin/bash
# Show the full loop around line 274 where calculatePoolShare is called
sed -n '250,290p' ledger/common/rewards.go
Length of output: 959
🏁 Script executed:
#!/bin/bash
# Specifically look for the for loop that iterates over pools
sed -n '240,310p' ledger/common/rewards.goLength of output: 1980
Fix division-by-zero vulnerability for zero-stake pools in calculatePoolPerformance
The vulnerability is confirmed. The loop at line 268 iterates over snapshot.PoolStake, which can contain zero-valued entries (pools with no active stake). The only guard is a check for PoolParams existence, leaving no protection against poolStake == 0.
When calculatePoolPerformance is called at line 231, it divides by this potentially-zero stake value:
stakeRatio := float64(snapshot.TotalActiveStake) / float64(poolStake)In Go, division by zero for floats silently returns +Inf, which propagates into the performance metric and distorts reward calculations.
The suggested fix is correct:
poolStake := snapshot.PoolStake[poolID]
if poolStake == 0 {
// No stake in the pool → no meaningful performance
return 0.0
}
stakeRatio := float64(snapshot.TotalActiveStake) / float64(poolStake)This aligns with the existing check at line 224 for zero blocks and correctly treats zero-stake pools as having zero performance.
🤖 Prompt for AI Agents
In ledger/common/rewards.go around lines 205 to 237, calculatePoolPerformance
divides by snapshot.PoolStake[poolID] which can be zero; add a guard after
reading poolStake: if poolStake == 0 { return 0.0 } so zero-stake pools are
treated as zero performance and avoid producing +Inf values; keep the existing
checks for TotalBlocksInEpoch and poolBlocks and then compute stakeRatio and
performance only when poolStake > 0.
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
wolf31o2
force-pushed
the
feat/initial-rewards
branch
from
f884773 to
b3b245f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ledger/common/rewards_service.go (1)
34-83: Good orchestration, but consider documenting hardcoded parameters.The method correctly orchestrates the reward calculation flow with proper error handling. However, the hardcoded
RewardParametersat lines 53-57 should be highlighted with a TODO comment for visibility:// TODO: Get protocol parameters from ledger state instead of using hardcoded defaults params := RewardParameters{ MonetaryExpansion: 3000, // 0.3% TreasuryGrowth: 2000, // 20% PoolInfluence: big.NewRat(1, 1), // a0 = 1 }This makes it clearer that parameter sourcing needs to be implemented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/test/ledger/ledger.go(2 hunks)ledger/common/rewards.go(1 hunks)ledger/common/rewards_service.go(1 hunks)ledger/common/rewards_test.go(1 hunks)ledger/common/rewards_validation_test.go(1 hunks)ledger/common/state.go(1 hunks)
🔇 Additional comments (30)
ledger/common/state.go (1)
41-66: LGTM! Clean interface design.The
RewardStateinterface is well-structured with clear separation of concerns. The method signatures are intuitive, and embedding it intoLedgerStatefollows idiomatic Go patterns for interface composition.internal/test/ledger/ledger.go (2)
29-31: LGTM! Mock fields follow conventions.The new
MockAdaPotsandMockRewardSnapshotfields are consistent with the existing mock pattern and provide the necessary test data storage.
95-116: LGTM! Mock implementation is appropriate.The mock methods correctly delegate to production logic where needed (
CalculateRewards) and provide sensible test behavior for state queries. The no-opUpdateAdaPotswith its explanatory comment is acceptable for a mock.ledger/common/rewards_test.go (8)
28-69: LGTM! Thorough monetary expansion test.The test validates the complete ADA pots calculation flow with realistic parameters. The inline comments explaining expected calculations (lines 54-61) make the test easy to verify and maintain.
71-145: LGTM! Comprehensive reward calculation validation.The test exercises the full reward distribution pipeline with realistic multi-pool scenarios. The assertion at line 141 ensures conservation of rewards, and line 144 validates pot depletion—both critical invariants.
147-159: LGTM! Appropriate float conversion tests.The test covers key margin values with reasonable float comparison tolerance (
InDeltawith 0.001).
161-181: LGTM! Good pledge validation coverage.The test validates both passing and failing pledge scenarios with multi-owner pools, covering the essential validation logic.
183-208: LGTM! Saturation calculation tests look correct.The test validates both fully-saturated (1.0) and under-saturated (0.2) scenarios with clear expected values.
210-220: LGTM! Useful test helper.The
createGenesisRathelper simplifies test data construction for margin and ratio values, improving test readability.
222-304: LGTM! Solid integration test.The integration test validates the complete RewardService workflow, including pot updates and reward conservation (lines 292-303). This provides valuable end-to-end coverage.
306-363: LGTM! Complete mock implementation.The local
mockLedgerStateprovides all necessary interface implementations for integration testing, with appropriate delegation to productionCalculateRewardsat line 349.ledger/common/rewards_validation_test.go (7)
25-90: LGTM! Excellent Shelley formula validation.The test validates reward formulas against the Shelley specification with detailed inline calculations (lines 44-59). This provides a clear reference for the expected behavior and formula correctness.
92-219: LGTM! Thorough reference implementation validation.The test exercises a realistic 3-pool scenario and validates key invariants: positive rewards, reward pot depletion (line 178), and conservation of total rewards (lines 213-218). The previous assertion issue was addressed in commit b3b245f.
221-336: LGTM! Comprehensive edge case coverage.The test suite covers critical edge cases: zero distributable rewards (when reward equals cost), high margin scenarios, and single-owner pools. These validate that the formulas handle boundary conditions correctly.
338-370: LGTM! Clean reference formula implementations.The helper functions provide clear reference implementations of Shelley reward formulas, with proper handling of the edge case when
poolReward <= poolCost.
372-507: LGTM! Thorough Java reference validation.The test suite validates key scenarios from the Java reference implementation (cf-java-rewards-calculation), including eta calculation with various block counts, Amaru-style pool performance, and monetary expansion with eta. This ensures compatibility with established reference implementations.
509-641: LGTM! Excellent Amaru compatibility validation.The test suite explicitly validates compatibility with Amaru's implementation, documenting key differences (e.g., eta calculation without decentralization dependency, performance not capped at 1.0). The inline formula documentation (e.g., lines 513-514, 546-547) provides valuable context.
643-678: LGTM! Complete monetary expansion validation.The test validates Amaru's monetary expansion approach end-to-end, ensuring correct calculation of incentives, treasury contributions, and final pot values (lines 674-676).
ledger/common/rewards_service.go (2)
22-32: LGTM! Clean service construction.The
RewardServicestruct and constructor follow standard Go service patterns with clear dependency injection viaLedgerState.
85-112: LGTM! Basic validation logic is appropriate.The method provides a simple but effective total rewards validation. The comment at lines 90-91 correctly notes that more detailed validation would depend on the specific distribution mechanism (MIR certificates, etc.).
ledger/common/rewards.go (10)
26-62: LGTM! Well-structured core types.The
AdaPotsandRewardParameterstypes are clearly documented with appropriate field types. The unit documentation for rates (lines 36-37) is particularly helpful for understanding the fixed-point representations.
64-106: LGTM! Comprehensive reward snapshot design.The
RewardSnapshottype captures all necessary state for reward calculation. The detailed comments on deregistration timing (lines 85-92) are particularly valuable for understanding the complex Cardano reward rules.
108-130: LGTM! Clean result type hierarchy.The result types provide a clear hierarchical structure: overall calculation result → per-pool rewards → individual rewards. This makes it easy to navigate and validate reward distributions.
132-190: LGTM! Monetary expansion calculation with proper underflow protection.The function correctly implements the Cardano monetary expansion formula. The underflow protection at lines 173-181 (added in commit b3b245f) properly handles edge cases where expansion might exceed reserves.
192-221: LGTM! Eta calculation with defensive guards.The function properly implements Amaru's efficiency-based eta calculation. The defensive guards at lines 195-202 (added in commit b3b245f) protect against nil pointers and division by zero, ensuring robustness.
223-258: LGTM! Pool performance calculation with complete edge case handling.The function correctly implements Amaru's pool performance formula with proper guards for edge cases: no total blocks (line 235), no pool blocks (line 240), and zero pool stake (lines 249-251, added in commit b3b245f).
260-380: LGTM! Comprehensive reward calculation with robust guards.The main reward calculation function properly handles all edge cases identified in previous reviews. The guards for empty pools (lines 303-305) and zero total share (lines 307-315) prevent division by zero and ensure fair fallback behavior. The rounding adjustment (lines 332-342) ensures exact distribution.
382-424: LGTM! Pool share calculation with defensive guards.The function implements the leader stake influence formula correctly with appropriate defensive guards for nil parameters (lines 401-404) and potential division issues (lines 413-419).
426-504: LGTM! Correct Shelley reward distribution.The function properly implements the Shelley formula for distributing pool rewards between operators and delegators. The check for stake registration (line 478) ensures only eligible participants receive rewards, and the rounding remainder is correctly assigned to the operator (lines 495-497).
506-580: LGTM! Solid utility function implementations.The utility functions provide correct implementations with appropriate edge case handling:
marginFloat: proper nil/zero checks (lines 509-516)CalculateOptimalPoolCount: simplified implementation with clear TODO (line 525)ValidatePoolPledge: correct pledge validation logicCalculatePoolSaturation: appropriate handling of zero denominators (lines 569-576)
✅ Actions performedReview triggered.
|
Summary by CodeRabbit
New Features
Tests