refactor(cardano): store incentives as an epoch field #803
Conversation
WalkthroughEpoch incentives are integrated into the epoch lifecycle: EpochState gains an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/cardano/src/pots.rs (2)
92-102: Consider removing duplicate functionality.The TODO comment on line 93 is valid.
neutral()andDefault::default()produce identical results. Consider either:
- Removing
neutral()and usingDefault::default()consistently, or- Removing the
Defaultderive and keeping onlyneutral()with explicit namingThe semantic naming of
neutral()is clearer for the domain, but having both creates API surface redundancy.
393-437: Good test coverage for Byron delta path.The test validates:
- Pot consistency before and after delta application
- Correct reserves/utxos calculations for Byron era
- Uses realistic mainnet-like values
Minor: Line 428 has a
dbg!(&pots)that could be removed or converted to an assertion.- dbg!(&pots); - assert!(pots.is_consistent(MAX_SUPPLY));crates/cardano/src/rewards/mocking.rs (1)
201-204: Consider a more descriptive panic message.The
.expect()will panic ifMockContextis created without going throughfrom_json_file(). For test code this is acceptable, but the message could guide users to the solution.fn available_rewards(&self) -> u64 { self.available_rewards - .expect("available rewards not computed") + .expect("available rewards not computed - use MockContext::from_json_file() to initialize") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
crates/cardano/src/estart/reset.rs(6 hunks)crates/cardano/src/ewrap/wrapup.rs(2 hunks)crates/cardano/src/genesis/mod.rs(2 hunks)crates/cardano/src/model.rs(2 hunks)crates/cardano/src/pots.rs(4 hunks)crates/cardano/src/rewards/mocking.rs(3 hunks)crates/cardano/src/rewards/mod.rs(4 hunks)crates/cardano/src/rupd/loading.rs(3 hunks)crates/cardano/src/rupd/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
crates/cardano/src/rewards/mocking.rs (2)
crates/cardano/src/rewards/mod.rs (2)
pots(302-302)available_rewards(301-301)crates/cardano/src/pots.rs (1)
epoch_incentives(261-289)
crates/cardano/src/rewards/mod.rs (2)
crates/cardano/src/rewards/mocking.rs (1)
available_rewards(201-204)crates/cardano/src/rupd/loading.rs (1)
available_rewards(172-174)
crates/cardano/src/rupd/loading.rs (3)
crates/cardano/src/rewards/mocking.rs (2)
pots(206-208)available_rewards(201-204)crates/cardano/src/rewards/mod.rs (2)
pots(302-302)available_rewards(301-301)crates/cardano/src/rupd/mod.rs (1)
RupdWork(173-173)
crates/cardano/src/estart/reset.rs (1)
crates/cardano/src/pots.rs (3)
apply_delta(373-378)calculate_eta(231-259)epoch_incentives(261-289)
🔇 Additional comments (18)
crates/cardano/src/rupd/mod.rs (1)
97-107: LGTM!The simplification from
EpochIncentivesto a plainu64available_rewardsfield is a clean approach that aligns with the broader refactor. The struct now exposes only the data needed downstream, reducing coupling to the internal incentives structure.crates/cardano/src/genesis/mod.rs (1)
67-78: LGTM!Initializing
incentiveswithEpochIncentives::default()at genesis is appropriate since no incentives have been computed for epoch 0.crates/cardano/src/model.rs (2)
1463-1465: Deprecation approach is consistent.The
__epoch_incentivesfield follows the same deprecation pattern as other fields inEndStats(e.g.,__drep_deposits,__drep_refunds). Consider adding a tracking issue for removing these deprecated fields in a future release.
1488-1516: The field indices were pre-designed with intentional gaps for forward compatibility.The indices 4-8 in
EpochStateare reserved slots, not evidence of reindexing. CBOR's explicit field numbering (#[n(...)]) inherently supports forward and backward compatibility—old persisted data with missing fields deserializes correctly, and new data with additional fields can coexist with older readers. Theincentivesfield at#[n(3)]occupies a pre-planned position and does not conflict with existing serialized data.crates/cardano/src/ewrap/wrapup.rs (1)
127-139: LGTM!Setting
__epoch_incentivestoEpochIncentives::default()is correct here since incentives are now tracked inEpochState.incentivesrather than inEndStats. This maintains backward compatibility for the deprecated field while the actual data flows through the new path.crates/cardano/src/estart/reset.rs (4)
174-213: Incentives computation moved to epoch transition.The
define_new_incentivesfunction correctly:
- Returns neutral incentives for Byron era
- Computes incentives using the new pots' reserves (not the old ones)
- Uses
mark()snapshot for fees, which is appropriate for the epoch being transitionedThis is the key refactor: incentives are now computed during epoch transition and stored in
EpochState.incentivesfor use in the next epoch's rewards calculation.
215-272: Pot delta application now uses stored incentives.Line 251 correctly uses
epoch.incentives(the incentives computed during the previous epoch transition) when applying the delta. This ensures consistency between the incentives used for rewards and the pots update.
308-325: Epoch transition now carries incentives for the next epoch.The ordering is correct:
define_new_pots- compute pots using current epoch's incentivesdefine_new_incentives- compute incentives for the next epoch using new potsThis ensures the new epoch starts with pre-computed incentives available for rewards distribution.
142-172: Eta calculation logic is correct.The
define_etafunction properly handles:
- Byron era returning eta = 1 (no performance-based rewards)
- Missing mark data returning eta = 1
- Shelley+ era computing eta from actual vs expected blocks
The use of
is_none_oris consistent with the project's practices—it's already used in multiple locations throughout the codebase (e.g.,src/serve/grpc/watch.rs) and the CI pipeline uses the stable toolchain, which supports this method.crates/cardano/src/pots.rs (2)
291-312: Byron delta application correctly handles era-specific semantics.The implementation properly:
- Zeroes out all Shelley+ concepts (deposits, treasury, fees, rewards, etc.)
- Only tracks utxo changes
- Derives reserves from utxo changes (consumed utxos go back to reserves)
This matches Byron's simpler economic model where there's no staking/delegation.
373-378: Protocol version dispatch is correct.The routing logic correctly maps:
- Protocol 0, 1 → Byron (pre-Shelley)
- Protocol 2+ → Shelley and later eras
This provides clean separation of era-specific pot accounting logic.
crates/cardano/src/rupd/loading.rs (2)
151-159: Clean simplification of RupdWork loading.Loading
available_rewardsdirectly fromepoch.incentives.available_rewardsis cleaner than computing it on-demand. The incentives are now pre-computed during epoch transition, making this a simple data access.
171-178: RewardsContext implementation simplified.The
available_rewards()method now simply returns the pre-loaded value, consistent with the trait's simplified interface that exposes only the numeric rewards value rather than the fullEpochIncentivesstructure.crates/cardano/src/rewards/mocking.rs (2)
137-139: LGTM!The field simplification from
Option<EpochIncentives>toOption<u64>is a clean refactor that stores only the value needed by theRewardsContexttrait.
159-167: LGTM!The incentives computation correctly extracts only the
available_rewardsvalue needed for the simplifiedRewardsContexttrait interface.crates/cardano/src/rewards/mod.rs (3)
200-208: LGTM!The simplified
new()method that takes no arguments is clean. Note that it's now functionally identical to theDefaultimplementation, but keeping both is idiomatic Rust.
300-301: LGTM!The trait method simplification from returning
&EpochIncentivesto returningu64is a clean API improvement that exposes only what callers need. The relevant code snippets confirm consistent implementations in bothmocking.rsandloading.rs.
329-347: LGTM!The call sites are correctly updated to use the simplified API:
RewardMap::<C>::new()matches the new parameterless signaturectx.available_rewards()is more direct than the previousctx.incentives().available_rewards
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/cardano/src/pots.rs (1)
92-102: Remove duplication betweenneutral()andDefault::default().Both
neutral()andDefault::default()(derived on line 77) produce identical results. The TODO comment acknowledges this duplication. Consider removingneutral()and usingDefault::default()consistently, or document if there's a semantic distinction intended.Apply this diff if choosing to remove
neutral():-impl EpochIncentives { - // TODO: this and default are same, commit to one - pub fn neutral() -> Self { - Self { - total: 0, - treasury_tax: 0, - available_rewards: 0, - used_fees: 0, - } - } -}Then update call sites (e.g., line 184 in
crates/cardano/src/estart/reset.rs) to useEpochIncentives::default()instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/cardano/src/estart/reset.rs(6 hunks)crates/cardano/src/ewrap/wrapup.rs(2 hunks)crates/cardano/src/pots.rs(1 hunks)crates/cardano/src/rewards/mod.rs(5 hunks)crates/cardano/src/rupd/mod.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/cardano/src/rupd/mod.rs
- crates/cardano/src/ewrap/wrapup.rs
🧰 Additional context used
🧬 Code graph analysis (2)
crates/cardano/src/estart/reset.rs (1)
crates/cardano/src/pots.rs (3)
apply_delta(403-408)calculate_eta(256-284)epoch_incentives(286-314)
crates/cardano/src/rewards/mod.rs (2)
crates/cardano/src/rupd/loading.rs (1)
available_rewards(172-174)crates/cardano/src/rewards/mocking.rs (1)
available_rewards(201-204)
⏰ 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). (4)
- GitHub Check: Test (windows-latest)
- GitHub Check: Test (macos-13)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Test (macos-14)
🔇 Additional comments (10)
crates/cardano/src/pots.rs (1)
104-104: LGTM!Adding
DefaultforPotDeltais reasonable and aligns with the pattern of providing default initialization. The derived implementation initializesprotocol_versionto 0, which matchesPotDelta::neutral(0).crates/cardano/src/estart/reset.rs (5)
89-89: LGTM!The addition of
new_incentivesfield toEpochTransitionand its application toentity.incentivescorrectly implements the refactor objective of storing incentives as an epoch field.Also applies to: 119-119
174-213: LGTM!The
define_new_incentivesfunction correctly computes epoch incentives based on context and pot state. The Byron special case handling and debug logging are appropriate.
251-251: LGTM!The change to use
&epoch.incentivesinapply_deltacorrectly reflects that incentives are now stored as an epoch field, aligning with the PR objective.
313-319: LGTM!The flush logic correctly computes both
new_potsandnew_incentivesin sequence and passes them to theEpochTransitionconstructor, completing the integration of incentives into the epoch lifecycle.
142-172: LGTM. Theis_none_ormethod is available and actively used throughout the codebase.The
define_etafunction correctly computes eta based on genesis parameters and epoch state. The early returns for Byron and epoch 0 cases are appropriate defaults. Theis_none_ormethod is already used in multiple places in the codebase (e.g.,src/serve/grpc/watch.rs), confirming it's available in the project's Rust version. The TODO on line 150 about verifying epoch 0 behavior against specs is noted for future work.crates/cardano/src/rewards/mod.rs (4)
142-148: LGTM!The removal of the
incentivesfield and addition ofPhantomData<C>correctly simplifiesRewardMapby removing unused data while maintaining the generic parameter.
174-182: LGTM!The
DefaultandCloneimplementations are correctly updated to match the simplified struct definition.Also applies to: 185-193
197-204: LGTM!The simplified
new()constructor correctly removes the unusedincentivesparameter.
292-292: LGTM!The API change from
incentives() -> &EpochIncentivestoavailable_rewards() -> u64is a clean improvement that exposes only the necessary data. The updated usage indefine_rewardsis consistent with this change.Also applies to: 321-321, 338-338
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.