Distribute Pooled Staking rewards once per session and reduce the amount of events#1469
Open
Distribute Pooled Staking rewards once per session and reduce the amount of events#1469
Conversation
📝 WalkthroughWalkthroughAggregate pooled-staking rewards into on‑chain Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Pallet as PooledStaking
participant Accumulator as PendingRewards (Storage)
participant Timer as RewardsDistributionTimer / OnInitialize
participant Currency as Currency
Caller->>Pallet: accumulate_rewards(candidate, amount)
Pallet->>Accumulator: add amount to candidate entry
Pallet-->>Caller: return (no immediate transfer)
Note over Timer,Accumulator: Timer elapses (session/block boundary)
Timer->>Accumulator: read pending rewards
loop per candidate
Timer->>Pallet: distribute_accumulated_rewards(candidate, amount)
Pallet->>Currency: transfer(amount -> recipients)
Currency-->>Pallet: transfer result
Pallet->>Timer: emit RewardsDistributed(candidate, amounts)
end
Timer->>Accumulator: clear distributed entries / update last_distribution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Contributor
WASM runtime size check:Compared to target branchdancebox runtime: 1900 KB (no changes) ✅ flashbox runtime: 1124 KB (no changes) ✅ dancelight runtime: 2672 KB (+16 KB) 🚨 starlight runtime: 2584 KB (no changes) 🚨 container chain template simple runtime: 1516 KB (no changes) ✅ container chain template frontier runtime: 1848 KB (no changes) ✅ |
…ng-aggregate-rewards-distrubution
…ng-aggregate-rewards-distrubution
nanocryk
added
a0-pleasereview
…ng-aggregate-rewards-distrubution
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/suites/dev-tanssi/staking/test_staking_rewards_non_balanced.ts (1)
172-212: Re-fetch events after the session jump forRewardsDistributed.With distribution deferred to session boundaries, filtering the pre-jump
eventscan miss theRewardsDistributedevent and make the assertions stale.🔧 Suggested fix
- const events = await polkadotJs.query.system.events(); - const reward = await fetchRewardAuthorOrchestrator(events); - - await jumpSessions(context, 1); + const events = await polkadotJs.query.system.events(); + const reward = await fetchRewardAuthorOrchestrator(events); + + await jumpSessions(context, 1); + const distributionEvents = await polkadotJs.query.system.events(); @@ - const stakingRewarded = await filterRewardStakingDistributed(events, reward.accountId.toString()); + const stakingRewarded = await filterRewardStakingDistributed( + distributionEvents, + reward.accountId.toString() + );
🤖 Fix all issues with AI agents
In
`@chains/orchestrator-relays/runtime/dancelight/src/tests/inflation_rewards.rs`:
- Around line 19-23: The import traits::Timer from pallet_pooled_staking is
unused and causing CI to fail; remove traits::Timer from the use list in the
test file (the glob that currently reads pallet_pooled_staking::{ traits::Timer,
ActivePoolKind, PendingOperationKey, PendingOperationQuery }) or alternatively
reference Timer where intended—prefer simply deleting traits::Timer to eliminate
the unused import.
In `@pallets/pooled-staking/src/lib.rs`:
- Around line 590-642: The weight increments in on_initialize are being dropped
because Weight::saturating_add returns a new Weight rather than mutating; update
each discarded call to assign back to weight (e.g., replace
weight.saturating_add(T::DbWeight::get().reads(2)) and the other two calls with
weight = weight.saturating_add(...)) so that the reads/writes and per-reward
fn_weight are correctly accumulated; refer to the on_initialize function, the
local variable weight, and the calls to T::DbWeight::get().reads/writes(...) and
fn_weight to locate the three places to fix.
In `@pallets/pooled-staking/src/pools.rs`:
- Around line 838-843: The PendingRewards struct uses #[pallet::unbounded] on
its BTreeMap<Candidate, Balance>, so the missing MaxEncodedLen is intentional;
update the code by adding a concise comment above the PendingRewards definition
(or the associated storage item) documenting why unbounded storage is
acceptable: mention the #[pallet::unbounded] attribute, that rewards are
accumulated in BTreeMap<Candidate, Balance>, that EligibleCandidatesBufferSize
(100) and the distribution logic clear pending rewards each cycle (so growth is
bounded in practice), and note any assumptions for future maintainers; reference
PendingRewards, BTreeMap, #[pallet::unbounded], and EligibleCandidatesBufferSize
in the comment.
In `@test/suites/dev-tanssi-relay/staking/test_staking_rewards_non_balanced.ts`:
- Around line 177-185: The test accesses pendingRewards.toJSON().rewards without
checking the Option; guard the result of
polkadotJs.query.pooledStaking.pendingRewards() with its isSome/isNone before
unwrapping: call pendingRewards.isSome() and only read
pendingRewards.toJSON().rewards (and derive collatorPendingRewards) when it's
Some, otherwise treat collatorPendingRewards as undefined/null or an empty value
and adjust the assertion against reward (or assert pendingRewards.isSome() if
the test requires rewards to exist). Update references in the test to use the
safe-guarded value for collatorPendingRewards and avoid direct unwrapping of
pendingRewards.
🧹 Nitpick comments (8)
test/suites/dev-tanssi-relay/staking/test_staking_join.ts (1)
81-88: Remove commented-out test assertions instead of leaving dead code.If
IncreasedStakeandUpdatedCandidatePositionevents are no longer emitted immediately uponrequestDelegatedue to the deferred distribution model, these assertions should be deleted rather than commented out. Commented-out code reduces test clarity and creates maintenance burden.If there's a specific reason to keep these (e.g., events are still emitted but at a different point), consider adding a test that verifies the new expected behavior during session-based distribution instead.
♻️ Suggested change
await context.createBlock([await tx.signAsync(alice)]); const events = await polkadotJs.query.system.events(); - // const ev1 = events.filter((a) => { - // return a.event.method === "IncreasedStake"; - // }); - // expect(ev1.length).to.be.equal(1); - // const ev2 = events.filter((a) => { - // return a.event.method === "UpdatedCandidatePosition"; - // }); - // expect(ev2.length).to.be.equal(1); const ev3 = events.filter((a) => {test/suites/dev-tanssi-relay/staking/test_staking_swap.ts (1)
85-92: Remove commented-out event assertions.Keeping the commented block adds noise now that those events are no longer asserted; consider deleting it.
🧹 Suggested cleanup
- // const ev1 = events.filter((a) => { - // return a.event.method === "IncreasedStake"; - // }); - // expect(ev1.length).to.be.equal(1); - // const ev2 = events.filter((a) => { - // return a.event.method === "UpdatedCandidatePosition"; - // }); - // expect(ev2.length).to.be.equal(1);test/suites/dev-tanssi-relay/staking/test_staking_session.ts (1)
84-91: Remove commented-out event assertions.Keeping the commented block adds noise now that those events are no longer asserted; consider deleting it.
🧹 Suggested cleanup
- // const ev1 = events.filter((a) => { - // return a.event.method === "IncreasedStake"; - // }); - // expect(ev1.length).to.be.equal(1); - // const ev2 = events.filter((a) => { - // return a.event.method === "UpdatedCandidatePosition"; - // }); - // expect(ev2.length).to.be.equal(1);pallets/pooled-staking/src/tests/candidates.rs (1)
204-236: Avoid temporary Vec allocation when extending expected events.
extend_from_slice(&vec![...])allocates a throwaway Vec. You can push each event (orextendfrom an array) to keep it allocation-free. This applies to the similar blocks below as well.♻️ Example refactor
- expected_events.extend_from_slice(&vec![ - Event::RequestedDelegate { - candidate: action.candidate, - delegator: action.delegator, - pool: ActivePoolKind::AutoCompounding, - pending: action.amount, - }, - Event::StakedAutoCompounding { - candidate: action.candidate, - delegator: action.delegator, - shares: action.amount / share, - stake: action.amount, - }, - Event::ExecutedDelegate { - candidate: action.candidate, - delegator: action.delegator, - pool: ActivePoolKind::AutoCompounding, - staked: action.amount, - released: 0, - }, - ]); + expected_events.push(Event::RequestedDelegate { + candidate: action.candidate, + delegator: action.delegator, + pool: ActivePoolKind::AutoCompounding, + pending: action.amount, + }); + expected_events.push(Event::StakedAutoCompounding { + candidate: action.candidate, + delegator: action.delegator, + shares: action.amount / share, + stake: action.amount, + }); + expected_events.push(Event::ExecutedDelegate { + candidate: action.candidate, + delegator: action.delegator, + pool: ActivePoolKind::AutoCompounding, + staked: action.amount, + released: 0, + });test/suites/dev-tanssi-relay/staking/test_staking_rewards_balanced.ts (1)
170-189: GuardpendingRewardsOption before dereferencing.If the storage is
None,toJSON()yieldsnulland the test throws instead of failing with a clear assertion. Add anisSomecheck (and unwrap) so failures are explicit. Please confirm the Option API (isSome/unwrap) in the@polkadot/typesversion used.♻️ Suggested guard
- const pendingRewards = await polkadotJs.query.pooledStaking.pendingRewards(); - const collatorPendingRewards = pendingRewards.toJSON().rewards[reward.accountId.toString()]; + const pendingRewards = await polkadotJs.query.pooledStaking.pendingRewards(); + expect(pendingRewards.isSome).toBe(true); + const collatorPendingRewards = pendingRewards.unwrap().toJSON().rewards[reward.accountId.toString()];pallets/pooled-staking/src/tests/rewards.rs (2)
119-123: Avoid hardcoded distribution delay.Using the configured delay keeps this test resilient if timing parameters change.
♻️ Suggested update
- crate::PendingRewards::<Runtime>::put(crate::pools::PendingRewards { - last_distribution: <Runtime as crate::Config>::RewardsDistributionTimer::now() - 2, - rewards: Default::default(), - }); + let delay = <Runtime as crate::Config>::RewardsDistributionSessionDelay::get(); + let now = <Runtime as crate::Config>::RewardsDistributionTimer::now(); + crate::PendingRewards::<Runtime>::put(crate::pools::PendingRewards { + last_distribution: now.saturating_sub(delay.saturating_add(1)), + rewards: Default::default(), + });
766-779: Advance blocks based on configured delay.This test currently assumes a single-block delay; using the config constant avoids brittle timing.
♻️ Suggested update
- // Let's move to next block, rewards should be distributed in on_initialize. - run_block(); - assert_eq!( - <Runtime as crate::Config>::RewardsDistributionTimer::now(), - block_number + 1 - ); + // Advance by the configured delay so distribution timing stays in sync with config. + let delay = <Runtime as crate::Config>::RewardsDistributionSessionDelay::get(); + for _ in 0..delay { + run_block(); + } + assert_eq!( + <Runtime as crate::Config>::RewardsDistributionTimer::now(), + block_number + delay + ); assert_eq!( crate::PendingRewards::<Runtime>::get(), Some(crate::pools::PendingRewards { - last_distribution: block_number + 1, + last_distribution: block_number + delay, rewards: Default::default(), // distribution occured, no rewards pending }) );pallets/pooled-staking/src/lib.rs (1)
411-425: Unbounded storage acknowledged but verify operational limits.The
PendingRewardsstorage is explicitly marked unbounded with the rationale that it's "indirectly bounded" by the distribution timer. However, if the timer period is long or many collators receive rewards, this map could grow substantially, affecting PoV size.Consider whether a hard cap on the number of pending entries is warranted as a safety measure, or ensure monitoring is in place to detect abnormal growth.
chains/orchestrator-relays/runtime/dancelight/src/tests/inflation_rewards.rs
Outdated
Show resolved
Hide resolved
Comment on lines
+177
to
+185
| const pendingRewards = await polkadotJs.query.pooledStaking.pendingRewards(); | ||
| const collatorPendingRewards = pendingRewards.toJSON().rewards[reward.accountId.toString()]; | ||
|
|
||
| // How much should the author have gotten? | ||
| // For now everything as we did not execute the pending operations | ||
| expect(reward.balance.toBigInt()).toBeGreaterThanOrEqual(expectedContainerReward - 1n); | ||
| expect(reward.balance.toBigInt()).toBeLessThanOrEqual(expectedContainerReward + 2n); | ||
| expect(stakingRewardedCollator.manualRewards).to.equal(reward.balance.toBigInt()); | ||
| expect(stakingRewardedCollator.autoCompoundingRewards).to.equal(0n); | ||
| expect(stakingRewardedDelegators.manualRewards).to.equal(0n); | ||
| expect(stakingRewardedDelegators.autoCompoundingRewards).to.equal(0n); | ||
|
|
||
| expect(collatorPendingRewards).to.equal(reward.balance.toJSON()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the test file to understand the context
cd test/suites/dev-tanssi-relay/staking && head -200 test_staking_rewards_non_balanced.ts | tail -50Repository: moondance-labs/tanssi
Length of output: 2878
🏁 Script executed:
# Check the `@polkadot/api` version in use
find . -type f -name "package.json" -o -name "package-lock.json" | head -5Repository: moondance-labs/tanssi
Length of output: 192
🏁 Script executed:
# Search for pendingRewards definition or type information
rg "pendingRewards" --type ts -B 2 -A 5Repository: moondance-labs/tanssi
Length of output: 8505
🏁 Script executed:
# Search for usage of Option type methods like isSome, unwrap, isNone
rg "\.isSome|\.unwrap|\.isNone" --type ts test/ | head -20Repository: moondance-labs/tanssi
Length of output: 1932
🏁 Script executed:
# Look for how Option is handled elsewhere in tests
rg "Option\<" --type ts test/ -B 2 -A 2 | head -40Repository: moondance-labs/tanssi
Length of output: 145
🏁 Script executed:
# Check package.json for polkadot/api version
cat test/package.json | grep -A 2 "@polkadot/api"Repository: moondance-labs/tanssi
Length of output: 160
🏁 Script executed:
# Search for PalletPooledStakingPoolsPendingRewards type definition to understand structure
rg "PalletPooledStakingPoolsPendingRewards" --type ts -B 2 -A 5 | head -40Repository: moondance-labs/tanssi
Length of output: 3546
Guard pendingRewards Option before accessing .rewards.
The return type is Option<PalletPooledStakingPoolsPendingRewards>, which can be None when rewards are cleared. Accessing .toJSON().rewards without checking will cause a runtime error, making the test flaky. Use an isSome guard before unwrapping.
Suggested fix
- const pendingRewards = await polkadotJs.query.pooledStaking.pendingRewards();
- const collatorPendingRewards = pendingRewards.toJSON().rewards[reward.accountId.toString()];
+ const pendingRewardsOpt = await polkadotJs.query.pooledStaking.pendingRewards();
+ expect(pendingRewardsOpt.isSome).toBe(true);
+ const pendingRewards = pendingRewardsOpt.unwrap().toJSON();
+ const collatorPendingRewards = pendingRewards.rewards[reward.accountId.toString()];📝 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.
Suggested change
| const pendingRewards = await polkadotJs.query.pooledStaking.pendingRewards(); | |
| const collatorPendingRewards = pendingRewards.toJSON().rewards[reward.accountId.toString()]; | |
| // How much should the author have gotten? | |
| // For now everything as we did not execute the pending operations | |
| expect(reward.balance.toBigInt()).toBeGreaterThanOrEqual(expectedContainerReward - 1n); | |
| expect(reward.balance.toBigInt()).toBeLessThanOrEqual(expectedContainerReward + 2n); | |
| expect(stakingRewardedCollator.manualRewards).to.equal(reward.balance.toBigInt()); | |
| expect(stakingRewardedCollator.autoCompoundingRewards).to.equal(0n); | |
| expect(stakingRewardedDelegators.manualRewards).to.equal(0n); | |
| expect(stakingRewardedDelegators.autoCompoundingRewards).to.equal(0n); | |
| expect(collatorPendingRewards).to.equal(reward.balance.toJSON()); | |
| const pendingRewardsOpt = await polkadotJs.query.pooledStaking.pendingRewards(); | |
| expect(pendingRewardsOpt.isSome).toBe(true); | |
| const pendingRewards = pendingRewardsOpt.unwrap().toJSON(); | |
| const collatorPendingRewards = pendingRewards.rewards[reward.accountId.toString()]; | |
| // How much should the author have gotten? | |
| // For now everything as we did not execute the pending operations | |
| expect(reward.balance.toBigInt()).toBeGreaterThanOrEqual(expectedContainerReward - 1n); | |
| expect(reward.balance.toBigInt()).toBeLessThanOrEqual(expectedContainerReward + 2n); | |
| expect(collatorPendingRewards).to.equal(reward.balance.toJSON()); |
🤖 Prompt for AI Agents
In `@test/suites/dev-tanssi-relay/staking/test_staking_rewards_non_balanced.ts`
around lines 177 - 185, The test accesses pendingRewards.toJSON().rewards
without checking the Option; guard the result of
polkadotJs.query.pooledStaking.pendingRewards() with its isSome/isNone before
unwrapping: call pendingRewards.isSome() and only read
pendingRewards.toJSON().rewards (and derive collatorPendingRewards) when it's
Some, otherwise treat collatorPendingRewards as undefined/null or an empty value
and adjust the assertion against reward (or assert pendingRewards.isSome() if
the test requires rewards to exist). Update references in the test to use the
safe-guarded value for collatorPendingRewards and avoid direct unwrapping of
pendingRewards.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pallets/pooled-staking/src/pools.rs`:
- Around line 532-562: The current distribute_accumulated_rewards function uses
core::mem::take(&mut pending_rewards.rewards) which clears all pending_rewards
up-front and loses unprocessed rewards if distribute_rewards::<T> fails; change
the logic to iterate over a borrowed list or drain entries one-by-one and only
remove each entry from pending_rewards.rewards after a successful
distribute_rewards::<T> call (or, on error, restore/leave the remaining entries
intact), i.e., avoid taking the whole map at start and instead process entries
atomically per candidate so pending_rewards.rewards retains unprocessed
candidates when an error is returned.
🧹 Nitpick comments (1)
chains/orchestrator-relays/runtime/dancelight/src/tests/inflation_rewards.rs (1)
115-115: Avoid hard-coded session indices in timer asserts.
UsingSession::current_index()will keep these tests robust if session flow changes.♻️ Suggested tweak (apply similarly to other asserts)
-assert_eq!(pending.last_distribution, 3u32, "timer should be up to date"); +assert_eq!( + pending.last_distribution, + Session::current_index(), + "timer should be up to date" +);Also applies to: 123-123, 179-179, 197-197
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pallets/pooled-staking/src/pools.rs`:
- Around line 541-566: The loop uses non-existent accessor methods and an
undefined variable `rewards`; replace calls to
`pending_rewards.rewards()`/`pending_rewards.get()` and `rewards.remove(...)`
with direct field accesses on the PendingRewards instance: collect keys from
`pending_rewards.rewards.keys().cloned().collect()`, fetch each reward with
`pending_rewards.rewards.get(&candidate).cloned().unwrap_or_default()`, and
remove the entry with `pending_rewards.rewards.remove(&candidate)`; keep the
call to `distribute_rewards::<T>(&candidate, reward)` and the weight updates
as-is.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pallets/pooled-staking/src/pools.rs`:
- Around line 608-619: Rename the test helper function
distribute_accumulated_rewards_immediatly to
distribute_accumulated_rewards_immediately and update all references to it
(e.g., in unit tests) to the new name; keep the same signature pub fn
distribute_accumulated_rewards_immediately<T: Config>() ->
DispatchResultWithPostInfo, preserve the #[cfg(test)] attribute and the internal
logic (using crate::PendingRewards::<T>::get/put and calling
distribute_accumulated_rewards::<T>(&mut pending)?). Ensure any test imports or
uses reference distribute_accumulated_rewards_immediately to avoid compilation
errors.
- Around line 532-573: The loop in distribute_accumulated_rewards removes
entries from the in-memory PendingRewards but does not persist those removals,
so a later retry (called by distribute_accumulated_rewards_on_timer) can re-read
already-committed per-candidate distributions; after each successful
distribute_rewards::<T>(&candidate, reward) call update the on-chain storage
representation of PendingRewards (persist pending_rewards) so the removed entry
is saved, or alternatively wrap the whole loop in a single storage transaction
to ensure all-or-nothing semantics; reference PendingRewards,
distribute_accumulated_rewards, distribute_rewards::<T>, and
distribute_accumulated_rewards_on_timer when making the persistence change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pallets/pooled-staking/src/pools.rs`:
- Around line 579-615: The timed-distribution uses the ? operator on
distribute_accumulated_rewards, which causes an early return on error before
adding the base DB write and distribution weight; replace the `let post_info =
result?;` pattern in distribute_accumulated_rewards_on_timer<T: Config>() with
an explicit match on `result` (e.g. `match result { Ok(post_info) => post_info,
Err(e) => { weight = weight.saturating_add(T::DbWeight::get().writes(1)); return
Err(e); } }`) so that the PendingRewards write weight
(T::DbWeight::get().writes(1)) is always added on both success and error paths
and then add the distribution weight from `post_info.actual_weight` only after
the Ok branch; reference functions/types:
distribute_accumulated_rewards_on_timer, distribute_accumulated_rewards,
PendingRewards, T::DbWeight, T::RewardsDistributionTimer.
♻️ Duplicate comments (1)
pallets/pooled-staking/src/pools.rs (1)
617-628: Typo:immediatly→immediately.
This was already flagged earlier; renaming would improve clarity (and update call sites).📝 Suggested rename
-pub fn distribute_accumulated_rewards_immediatly<T: Config>() -> DispatchResultWithPostInfo { +pub fn distribute_accumulated_rewards_immediately<T: Config>() -> DispatchResultWithPostInfo {
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pallets/pooled-staking/src/pools.rs`:
- Around line 509-529: The weight returned from accumulate_rewards currently
only includes PendingRewards read/write but omits DB operations performed by
T::Currency::resolve(&T::StakingAccount::get(), ...) which may mutate storage;
update accumulate_rewards to account for the resolve() weight (either by adding
its read/write weight to the returned weight or ensuring the caller
documents/charges for those DB ops). Specifically, adjust the final Ok(...)
return in accumulate_rewards to include the DB weight consumed by
T::Currency::resolve (or call a helper that returns that weight) in addition to
T::DbWeight::get().reads(1) + writes(1), referencing the functions/symbols
accumulate_rewards, T::Currency::resolve, T::StakingAccount::get, and
crate::PendingRewards::<T>::put.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@chains/orchestrator-relays/runtime/starlight/src/tests/inflation_rewards.rs`:
- Around line 157-192: Replace the weak assert_ne! balance check with a precise
assertion using the precomputed rewards_per_chain: after run_to_session(5u32)
fetch balance_after via System::account(&account).data.free and assert that
balance_after - balance_before == rewards_per_chain (or use >= rewards_per_chain
if other incidental changes may apply); update the assertion around the pending
rewards block accordingly so the test verifies the exact distributed amount
using the rewards_per_chain variable.
Comment on lines
157
to
192
| let all_rewards = RewardsPortion::get() * summary.inflation; | ||
| // rewards are shared between the 2 paras | ||
| let orchestrator_rewards = all_rewards / 2; | ||
| let candidate_rewards = RewardsCollatorCommission::get() * orchestrator_rewards; | ||
| let rewards_per_chain = all_rewards / 2; // rewards are shared between the 2 paras | ||
|
|
||
| // Rewards are now aggregated in storage and then distributed at the start of the next session. | ||
| // We'll thus check that the pending rewards contains DAVE rewards. | ||
|
|
||
| { | ||
| let pending = pallet_pooled_staking::PendingRewards::<Runtime>::get().expect("pending rewards to exist"); | ||
| assert_eq!(pending.last_distribution, 4u32, "timer should be up to date"); | ||
| let dave_rewards = pending.rewards.get(&account).expect("DAVE should have pending rewards"); | ||
|
|
||
| assert_eq!(*dave_rewards, rewards_per_chain, "dave should have pending rewards for 1 chain"); | ||
| } | ||
|
|
||
| let balance_after = System::account(&account).data.free; | ||
| assert_eq!( | ||
| candidate_rewards, | ||
| balance_after - balance_before, | ||
| "dave should get the correct reward portion" | ||
| balance_before, | ||
| balance_after, | ||
| "dave shouldn't have rewards in their account yet" | ||
| ); | ||
|
|
||
| // let's go to next session | ||
| run_to_session(5u32); | ||
|
|
||
| { | ||
| let pending = pallet_pooled_staking::PendingRewards::<Runtime>::get().expect("pending rewards to exist"); | ||
| assert_eq!(pending.last_distribution, 5u32, "timer should be up to date"); | ||
| assert!(pending.rewards.is_empty(), "rewards should be distributed including the latest ones, so the pending rewards should be empty"); | ||
| } | ||
|
|
||
| let balance_after = System::account(&account).data.free; | ||
| assert_ne!( | ||
| balance_before, | ||
| balance_after, | ||
| "dave should have rewards in their account now" | ||
| ); |
There was a problem hiding this comment.
Strengthen the post‑distribution balance assertion.
assert_ne! only checks that the balance changed; it won’t catch incorrect distribution amounts. Since rewards_per_chain is already computed, assert the exact delta (or use >= if other rewards could accrue between snapshots).
✅ Suggested fix (exact delta)
let balance_after = System::account(&account).data.free;
- assert_ne!(
- balance_before,
- balance_after,
- "dave should have rewards in their account now"
- );
+ assert_eq!(
+ rewards_per_chain,
+ balance_after - balance_before,
+ "dave should receive the expected rewards after distribution"
+ );🤖 Prompt for AI Agents
In `@chains/orchestrator-relays/runtime/starlight/src/tests/inflation_rewards.rs`
around lines 157 - 192, Replace the weak assert_ne! balance check with a precise
assertion using the precomputed rewards_per_chain: after run_to_session(5u32)
fetch balance_after via System::account(&account).data.free and assert that
balance_after - balance_before == rewards_per_chain (or use >= rewards_per_chain
if other incidental changes may apply); update the assertion around the pending
rewards block accordingly so the test verifies the exact distributed amount
using the rewards_per_chain variable.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@chains/integration-tests/dancebox-runtime-tests/src/tests/integration_test.rs`:
- Around line 3766-3773: The test currently calls
pallet_pooled_staking::PendingRewards::<Runtime>::get().expect(...) which will
panic if storage is uninitialized; change it to tolerate None by checking the
Option first (e.g., use if let Some(pending) =
pallet_pooled_staking::PendingRewards::<Runtime>::get() {
assert!(pending.rewards.get(&account).is_none(), "..."); } else { /* no pending
rewards is acceptable */ } or use map_or to assert a true condition). This keeps
the check for the account's absence of rewards
(pending.rewards.get(&account).is_none()) while avoiding an expect panic when
PendingRewards is None.
Comment on lines
+3766
to
+3773
| { | ||
| let pending = pallet_pooled_staking::PendingRewards::<Runtime>::get() | ||
| .expect("pending rewards to exist"); | ||
| assert!( | ||
| pending.rewards.get(&account).is_none(), | ||
| "DAVE shouldn't have pending rewards at session start" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "PendingRewards" pallets/pooled-staking/src/lib.rs -A 3 -B 1Repository: moondance-labs/tanssi
Length of output: 325
🏁 Script executed:
rg -n "PendingRewards" pallets/pooled-staking/src/ --type rust -A 2 -B 1 | head -100Repository: moondance-labs/tanssi
Length of output: 6942
🏁 Script executed:
# Search for where PendingRewards is initialized or accessed
rg -n "PendingRewards.*insert|PendingRewards.*put\|StorageValue.*PendingRewards" pallets/pooled-staking/src/Repository: moondance-labs/tanssi
Length of output: 47
🏁 Script executed:
# Check the integration test setup and context around line 3766
sed -n '3700,3800p' chains/integration-tests/dancebox-runtime-tests/src/tests/integration_test.rs | cat -nRepository: moondance-labs/tanssi
Length of output: 4621
Make the initial PendingRewards check tolerant to None.
The PendingRewards storage is not initialized at genesis and only created lazily when the pallet's reward functions are invoked. At this test point (session 4), expect() will panic if rewards haven't accrued yet, masking the actual test intent. The pallet code itself uses defensive patterns (unwrap_or_else(), if let Some(), pattern matching) when accessing PendingRewards. The test should follow the same approach:
Safer check
- {
- let pending = pallet_pooled_staking::PendingRewards::<Runtime>::get()
- .expect("pending rewards to exist");
- assert!(
- pending.rewards.get(&account).is_none(),
- "DAVE shouldn't have pending rewards at session start"
- );
- }
+ {
+ if let Some(pending) = pallet_pooled_staking::PendingRewards::<Runtime>::get() {
+ assert!(
+ pending.rewards.get(&account).is_none(),
+ "DAVE shouldn't have pending rewards at session start"
+ );
+ }
+ }📝 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.
Suggested change
| { | |
| let pending = pallet_pooled_staking::PendingRewards::<Runtime>::get() | |
| .expect("pending rewards to exist"); | |
| assert!( | |
| pending.rewards.get(&account).is_none(), | |
| "DAVE shouldn't have pending rewards at session start" | |
| ); | |
| } | |
| { | |
| if let Some(pending) = pallet_pooled_staking::PendingRewards::<Runtime>::get() { | |
| assert!( | |
| pending.rewards.get(&account).is_none(), | |
| "DAVE shouldn't have pending rewards at session start" | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@chains/integration-tests/dancebox-runtime-tests/src/tests/integration_test.rs`
around lines 3766 - 3773, The test currently calls
pallet_pooled_staking::PendingRewards::<Runtime>::get().expect(...) which will
panic if storage is uninitialized; change it to tolerate None by checking the
Option first (e.g., use if let Some(pending) =
pallet_pooled_staking::PendingRewards::<Runtime>::get() {
assert!(pending.rewards.get(&account).is_none(), "..."); } else { /* no pending
rewards is acceptable */ } or use map_or to assert a true condition). This keeps
the check for the account's absence of rewards
(pending.rewards.get(&account).is_none()) while avoiding an expect panic when
PendingRewards is None.
Contributor
Coverage Report@@ Coverage Diff @@
## master jeremy-pooled-staking-aggregate-rewards-distrubution +/- ##
========================================================================================
+ Coverage 73.85% 73.86% +0.01%
Files 568 568
+ Lines 82793 82887 +94
========================================================================================
+ Hits 61141 61219 +78
+ Misses 21652 21668 +16
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Accumulates pending rewards in storage to effectively do the distribution later (once per session in dancelight).