refactor(cardano): move reward calc into its own module - #746
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds public modules Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Node runtime
participant Sweep as sweep::BoundaryVisitor
participant Macros as math_macros (ratio!/pallas_ratio!)
participant Rewards as crate::rewards
User->>Sweep: visit_pool(...)
Sweep->>Macros: pallas_ratio!(a0), pallas_ratio!(d), pallas_ratio!(pool.margin_cost)
Sweep->>Rewards: pool_rewards(epoch_rewards, circulating_supply, _total_stake, active_stake, pool_stake, declared_pledge, live_pledge, k, a0, d, pool_blocks, epoch_blocks)
Rewards->>Rewards: optimal_pool_rewards(...), pool_apparent_performance(...)
Rewards-->>Sweep: TotalPoolReward (u64)
Sweep->>Rewards: pool_operator_share(pool_rewards, fixed_cost, margin_cost, pool_stake, live_pledge, circulating_supply)
Rewards-->>Sweep: OperatorShare (u64)
Sweep->>Rewards: delegator_reward(available_rewards, total_delegated, delegator_stake)
Rewards-->>Sweep: DelegatorReward (u64)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
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 |
![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
🧹 Nitpick comments (2)
crates/cardano/src/rewards.rs (2)
209-237: Address the ignored test or remove it.This test is marked
#[ignore], so it won't run in CI. Either:
- Fix the test to assert expected values deterministically, or
- Remove it if it was used only for development/discovery
The current design (iterating until a match is found, then panicking otherwise) is unusual for a unit test.
189-238: Add test coverage for the public API.The public functions
delegator_reward(line 67) andpool_rewards(line 104) lack tests. Given that these are financial calculations, comprehensive test coverage is recommended to catch edge cases and regressions.Consider adding tests for:
delegator_reward: zero stake, zero total, normal casespool_rewards: underpledged pools (returns 0), normal cases, edge cases with k, d thresholdspool_operator_share: current ignored test plus additional edge cases
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/cardano/src/lib.rs(2 hunks)crates/cardano/src/math_macros.rs(1 hunks)crates/cardano/src/pots.rs(4 hunks)crates/cardano/src/rewards.rs(1 hunks)crates/cardano/src/sweep/rewards.rs(4 hunks)src/bin/dolos/data/dump_state.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
crates/cardano/src/rewards.rs (1)
crates/cardano/src/model.rs (3)
d(840-842)k(824-826)a0(828-830)
src/bin/dolos/data/dump_state.rs (1)
crates/cardano/src/model.rs (1)
live_stake(275-281)
crates/cardano/src/lib.rs (1)
crates/minibf/src/routes/accounts.rs (1)
rewards(550-573)
crates/cardano/src/pots.rs (1)
crates/cardano/src/model.rs (3)
d(840-842)rho(836-838)tau(832-834)
crates/cardano/src/sweep/rewards.rs (2)
crates/cardano/src/rewards.rs (3)
delegator_reward(67-70)pool_rewards(104-134)pool_operator_share(150-187)crates/cardano/src/model.rs (3)
d(840-842)k(824-826)a0(828-830)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check Build
🔇 Additional comments (2)
crates/cardano/src/rewards.rs (2)
7-61: LGTM: Optimal pool rewards calculation is well-implemented.The function correctly implements the Cardano optimal pool reward formula using exact rational arithmetic. The use of
min()to cap relative stakes prevents oversaturation, andfloor_int!ensures proper integer conversion.
72-101: LGTM: Pool apparent performance calculation is correct and defensive.The function properly guards against division by zero (lines 80, 86, 91) and correctly implements the Cardano apparent performance formula with decentralization threshold logic.
| pub fn delegator_reward(available_rewards: u64, total_delegated: u64, delegator_stake: u64) -> u64 { | ||
| let share = (delegator_stake as f64 / total_delegated as f64) * available_rewards as f64; | ||
| share.round() as u64 | ||
| } |
There was a problem hiding this comment.
Critical: Replace floating-point with exact rational arithmetic.
Floating-point arithmetic introduces rounding errors and non-determinism in financial calculations. This is inconsistent with the rest of the module, which uses BigRational for exact arithmetic.
Apply this diff to use exact rational math:
pub fn delegator_reward(available_rewards: u64, total_delegated: u64, delegator_stake: u64) -> u64 {
- let share = (delegator_stake as f64 / total_delegated as f64) * available_rewards as f64;
- share.round() as u64
+ if total_delegated == 0 {
+ return 0;
+ }
+ let share = ratio!(delegator_stake, total_delegated) * ratio!(available_rewards);
+ floor_int!(share, u64)
}📝 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.
| pub fn delegator_reward(available_rewards: u64, total_delegated: u64, delegator_stake: u64) -> u64 { | |
| let share = (delegator_stake as f64 / total_delegated as f64) * available_rewards as f64; | |
| share.round() as u64 | |
| } | |
| pub fn delegator_reward(available_rewards: u64, total_delegated: u64, delegator_stake: u64) -> u64 { | |
| if total_delegated == 0 { | |
| return 0; | |
| } | |
| let share = ratio!(delegator_stake, total_delegated) * ratio!(available_rewards); | |
| floor_int!(share, u64) | |
| } |
🤖 Prompt for AI Agents
In crates/cardano/src/rewards.rs around lines 67-70, replace the f64-based
computation with exact rational arithmetic: import num_bigint::BigInt (or
BigUint) and num_rational::BigRational, convert available_rewards,
total_delegated and delegator_stake into BigInt, compute share_r =
BigRational::new(BigInt::from(available_rewards) *
BigInt::from(delegator_stake), BigInt::from(total_delegated)) (handle
total_delegated == 0), perform correct rounding to the nearest integer by adding
BigRational::from_integer(BigInt::from(1)) /
BigRational::from_integer(BigInt::from(2)) then taking to_integer(), and finally
convert that BigInt to u64 (using try_from/to_u64 and handle overflow or
conversion failure); remove any f64 usage and ensure required crates are added
to Cargo.toml.
| if live_pledge < declared_pledge { | ||
| return 0; | ||
| } | ||
|
|
||
| assert!(k > 0, "k must be > 0"); |
There was a problem hiding this comment.
Major: Replace panic-inducing assert! with graceful error handling.
The assert! on line 121 will panic in production if k is 0. For a public API, this is a runtime hazard. Either validate k at the calling boundary or return 0/early-exit.
Apply this diff to handle k=0 gracefully:
- assert!(k > 0, "k must be > 0");
+ if k == 0 {
+ return 0;
+ }Alternatively, document the precondition clearly in the function's doc comment if panicking is intentional.
📝 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.
| if live_pledge < declared_pledge { | |
| return 0; | |
| } | |
| assert!(k > 0, "k must be > 0"); | |
| if live_pledge < declared_pledge { | |
| return 0; | |
| } | |
| if k == 0 { | |
| return 0; | |
| } |
🤖 Prompt for AI Agents
In crates/cardano/src/rewards.rs around lines 117 to 121, the current assert!(k
> 0, "k must be > 0") can panic in production; replace it with graceful handling
by checking if k == 0 and returning 0 (or an appropriate error result) instead
of panicking — validate the input at the function boundary and return early when
k is zero, or alternatively document the precondition in the function doc
comment if you purposely want callers to ensure k>0.
| pub fn pool_operator_share( | ||
| pool_rewards: u64, | ||
| fixed_cost: u64, | ||
| margin_cost: Ratio, | ||
| pool_stake: u64, | ||
| live_pledge: u64, | ||
| circulating_supply: u64, | ||
| ) -> u64 { | ||
| let c = fixed_cost; | ||
|
|
||
| if pool_rewards <= c { | ||
| return pool_rewards; // operator takes it all if rewards ≤ fixed cost | ||
| } | ||
|
|
||
| let after_cost = pool_rewards - c; | ||
|
|
||
| let s = ratio!(live_pledge, circulating_supply); | ||
| let σ = ratio!(pool_stake, circulating_supply); | ||
|
|
||
| // s/σ — ratio of owner's pledge to pool stake (denominator cancels, so we can | ||
| // use amounts) | ||
| let s_over_sigma = s / σ; | ||
|
|
||
| let one = ratio!(1); | ||
|
|
||
| let m = margin_cost; | ||
|
|
||
| // c + (f̂ − c) · ( m + (1 − m) · s/σ ) | ||
| let term = &m + (one - &m) * s_over_sigma; | ||
|
|
||
| let after_cost = ratio!(after_cost); | ||
|
|
||
| let variable = after_cost * term; | ||
|
|
||
| let variable = floor_int!(variable, u64); | ||
|
|
||
| c + variable | ||
| } |
There was a problem hiding this comment.
Major: Guard against division by zero in ratio construction.
Lines 166-167 construct ratios with circulating_supply as denominator without checking for zero. Line 171 divides by σ, which is zero if pool_stake is zero. Both scenarios would cause division by zero.
Apply this diff to add defensive guards:
pub fn pool_operator_share(
pool_rewards: u64,
fixed_cost: u64,
margin_cost: Ratio,
pool_stake: u64,
live_pledge: u64,
circulating_supply: u64,
) -> u64 {
+ if circulating_supply == 0 || pool_stake == 0 {
+ return pool_rewards;
+ }
+
let c = fixed_cost;📝 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.
| pub fn pool_operator_share( | |
| pool_rewards: u64, | |
| fixed_cost: u64, | |
| margin_cost: Ratio, | |
| pool_stake: u64, | |
| live_pledge: u64, | |
| circulating_supply: u64, | |
| ) -> u64 { | |
| let c = fixed_cost; | |
| if pool_rewards <= c { | |
| return pool_rewards; // operator takes it all if rewards ≤ fixed cost | |
| } | |
| let after_cost = pool_rewards - c; | |
| let s = ratio!(live_pledge, circulating_supply); | |
| let σ = ratio!(pool_stake, circulating_supply); | |
| // s/σ — ratio of owner's pledge to pool stake (denominator cancels, so we can | |
| // use amounts) | |
| let s_over_sigma = s / σ; | |
| let one = ratio!(1); | |
| let m = margin_cost; | |
| // c + (f̂ − c) · ( m + (1 − m) · s/σ ) | |
| let term = &m + (one - &m) * s_over_sigma; | |
| let after_cost = ratio!(after_cost); | |
| let variable = after_cost * term; | |
| let variable = floor_int!(variable, u64); | |
| c + variable | |
| } | |
| pub fn pool_operator_share( | |
| pool_rewards: u64, | |
| fixed_cost: u64, | |
| margin_cost: Ratio, | |
| pool_stake: u64, | |
| live_pledge: u64, | |
| circulating_supply: u64, | |
| ) -> u64 { | |
| // Prevent division-by-zero in ratio! and subsequent division | |
| if circulating_supply == 0 || pool_stake == 0 { | |
| return pool_rewards; | |
| } | |
| let c = fixed_cost; | |
| if pool_rewards <= c { | |
| return pool_rewards; // operator takes it all if rewards ≤ fixed cost | |
| } | |
| let after_cost = pool_rewards - c; | |
| let s = ratio!(live_pledge, circulating_supply); | |
| let σ = ratio!(pool_stake, circulating_supply); | |
| // s/σ — ratio of owner's pledge to pool stake (denominator cancels, so we can | |
| // use amounts) | |
| let s_over_sigma = s / σ; | |
| let one = ratio!(1); | |
| let m = margin_cost; | |
| // c + (f̂ − c) · ( m + (1 − m) · s/σ ) | |
| let term = &m + (one - &m) * s_over_sigma; | |
| let after_cost = ratio!(after_cost); | |
| let variable = after_cost * term; | |
| let variable = floor_int!(variable, u64); | |
| c + variable | |
| } |
🤖 Prompt for AI Agents
In crates/cardano/src/rewards.rs around lines 150–187, guard the ratio
construction and division to avoid divide-by-zero: if circulating_supply == 0
return pool_rewards immediately (can't compute ratios); otherwise build s and σ
using circulating_supply as before but if σ is zero (pool_stake == 0) set
s_over_sigma to zero (or Ratio::zero()) instead of performing s / σ; keep the
remaining math unchanged and ensure any conversions from integers to Ratio
happen after these checks.
| @@ -454,6 +290,7 @@ impl super::BoundaryVisitor for BoundaryVisitor { | |||
| %epoch_rewards, | |||
| %total_pool_reward, | |||
| %operator_share, | |||
| %live_pledge, | |||
| "computed pool rewards" | |||
| ); | |||
|
|
|||
There was a problem hiding this comment.
Use total stake snapshot instead of circulating supply
crate::rewards::pool_rewards (see crates/cardano/src/rewards.rs, lines 103‑133) expects its second argument to be the E‑2 total registered stake so it can derive σ and s with the correct denominator. Feeding circulating_supply shrinks both ratios dramatically, which causes optimal_pool_rewards to underpay every pool. Please pass the actual snapshot total you already track in ctx.active_snapshot (the same number that backs pool_stake), rather than the network-wide supply figure.
🤖 Prompt for AI Agents
In crates/cardano/src/sweep/rewards.rs around lines 260–281, the call sites pass
the network-wide `circulating_supply` into `crate::rewards::pool_rewards`
(second parameter) and `crate::rewards::pool_operator_share` (last parameter),
but those functions expect the E‑2 total registered stake snapshot; replace
`circulating_supply` with the snapshot total you already track in
`ctx.active_snapshot` (the same total that backs `pool_stake`) at both call
sites so σ and s are computed with the correct denominator.
1 participant
Summary by CodeRabbit
New Features
Refactor
User Interface