Add delegation scheduled requests summary#3680
Conversation
…ons flag in round transitions
…stsSummaryMap Replace the boolean PendingRevocations flag (StorageDoubleMap storing ()) with DelegationScheduledRequestsSummaryMap storing DelegationAction<BalanceOf<T>>. This tracks both pending revocations (Revoke(bond)) and the aggregated total of pending decreases (Decrease(total)) per (collator, delegator) pair. get_rewardable_delegators now reads only fixed-size summary values instead of variable-length request queues, while the execution queue storage (DelegationScheduledRequests) is preserved for timing and ordering.
📝 WalkthroughWalkthroughIntroduces DelegationScheduledRequestsSummaryMap storage item to track aggregated pending delegation actions (Revoke/Decrease) per (collator, delegator) pair. Updates delegation request operations and reward calculation logic to populate and read from this summary map instead of performing per-delegator aggregation at calculation time. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pallets/parachain-staking/src/tests.rs (1)
7397-7401: Consider adding a Decrease-case summary entry in the negative test fordelegation_request_revoke_exists.
This would explicitly validate that the new summary map returnsfalsewhen the stored action isDecrease.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallets/parachain-staking/src/tests.rs` around lines 7397 - 7401, In the negative test delegation_request_revoke_exists, add an explicit DelegationScheduledRequestsSummaryMap::<Test>::insert(...) entry using DelegationAction::Decrease (e.g., same keys used for the existing Revoke insert) so the test also asserts that the summary map check for a revoke returns false when the stored action is Decrease; update the assertions to ensure the revoke-existence check remains false for that Decrease entry.pallets/parachain-staking/src/delegation_requests.rs (1)
324-345: Consider a self-heal fallback if the summary entry is missing or not a Decrease.
Right now this branch is a no-op in that case, which can leave the summary map desynced fromscheduled_requestsif the map was not correctly backfilled. A lightweight fallback is to recompute the remaining decrease total fromscheduled_requestswhenentryis absent or non-decrease.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallets/parachain-staking/src/delegation_requests.rs` around lines 324 - 345, The current mutate_exists closure on DelegationScheduledRequestsSummaryMap silently no-ops if the summary entry is missing or not a Decrease; add a self-heal fallback that recomputes the remaining decrease total from scheduled_requests and updates the summary entry accordingly. Specifically, inside the closure for DelegationScheduledRequestsSummaryMap::mutate_exists(&collator, &delegator, |entry| { ... }), detect when entry.is_none() or entry is Some(...) but not DelegationAction::Decrease, then iterate the scheduled_requests storage for (collator, delegator) (the same scheduled_requests map used elsewhere in this module) to sum all pending Decrease amounts, subtract the current requested amount (the amount variable) using saturating arithmetic, and then set *entry to None if the remaining total is zero or to Some(DelegationAction::Decrease(remaining)) otherwise; keep the existing behavior when entry is already a Decrease by applying the same saturating_sub logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pallets/parachain-staking/src/delegation_requests.rs`:
- Around line 324-345: The current mutate_exists closure on
DelegationScheduledRequestsSummaryMap silently no-ops if the summary entry is
missing or not a Decrease; add a self-heal fallback that recomputes the
remaining decrease total from scheduled_requests and updates the summary entry
accordingly. Specifically, inside the closure for
DelegationScheduledRequestsSummaryMap::mutate_exists(&collator, &delegator,
|entry| { ... }), detect when entry.is_none() or entry is Some(...) but not
DelegationAction::Decrease, then iterate the scheduled_requests storage for
(collator, delegator) (the same scheduled_requests map used elsewhere in this
module) to sum all pending Decrease amounts, subtract the current requested
amount (the amount variable) using saturating arithmetic, and then set *entry to
None if the remaining total is zero or to
Some(DelegationAction::Decrease(remaining)) otherwise; keep the existing
behavior when entry is already a Decrease by applying the same saturating_sub
logic.
In `@pallets/parachain-staking/src/tests.rs`:
- Around line 7397-7401: In the negative test delegation_request_revoke_exists,
add an explicit DelegationScheduledRequestsSummaryMap::<Test>::insert(...) entry
using DelegationAction::Decrease (e.g., same keys used for the existing Revoke
insert) so the test also asserts that the summary map check for a revoke returns
false when the stored action is Decrease; update the assertions to ensure the
revoke-existence check remains false for that Decrease entry.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pallets/parachain-staking/src/benchmarks.rspallets/parachain-staking/src/delegation_requests.rspallets/parachain-staking/src/lib.rspallets/parachain-staking/src/tests.rs
Coverage Report@@ Coverage Diff @@
## master manuel/delegation-scheduled-requests-summary-map +/- ##
===================================================================================
Coverage 77.10% 77.10% 0.00%
Files 389 389
+ Lines 76943 76972 +29
===================================================================================
+ Hits 59323 59349 +26
+ Misses 17620 17623 +3
|
What does it do?
Introduces a new
DelegationScheduledRequestsSummaryMap(StorageDoubleMap<collator, delegator, DelegationAction>) that maintains a pre-aggregated summary of pending actions per(collator, delegator)pair:
Revoke(bond)— stored when a revocation is pending.Decrease(total)— aggregated sum of all pending decrease amounts.This summary map is kept in sync on every schedule, cancel, and execute operation in
delegation_requests.rs. Round-transition functions (get_rewardable_delegators,delegation_request_revoke_exists)now perform O(1) lookups per delegator into the summary map instead of iterating the full requests list.