Migrate Currency trait to Fungible trait #3306
8000Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update migrates the Moonbeam parachain staking system from using balance locks to a new freeze-based mechanism for restricting staked funds. It introduces new freeze reason types, migration extrinsics, helper functions, and storage tracking for migration status. All runtime configurations, tests, benchmarks, and helper utilities are updated to use freezes instead of locks, with comprehensive migration and compatibility logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ParachainStakingPallet
participant BalancesPallet
participant Storage
User->>ParachainStakingPallet: migrate_locks_to_freezes_batch(accounts, is_collator)
loop for each account
ParachainStakingPallet->>Storage: Check migration status
alt Not migrated
ParachainStakingPallet->>BalancesPallet: Remove old lock
ParachainStakingPallet->>BalancesPallet: Set freeze (StakingCollator or StakingDelegator)
ParachainStakingPallet->>Storage: Mark as migrated
else Already migrated
Note right of ParachainStakingPallet: Skip migration
end
end
ParachainStakingPallet-->>User: DispatchResult
sequenceDiagram
participant User
participant ParachainStakingPallet
participant BalancesPallet
User->>ParachainStakingPallet: join_candidates / delegate / bond_more
ParachainStakingPallet->>ParachainStakingPallet: freeze_extended(account, amount, is_collator)
ParachainStakingPallet->>BalancesPallet: Check and migrate lock if needed
alt Migration needed
ParachainStakingPallet->>BalancesPallet: Remove lock
ParachainStakingPallet->>BalancesPallet: Set freeze
else Already migrated
ParachainStakingPallet->>BalancesPallet: Set freeze
end
ParachainStakingPallet-->>User: Result
Possibly related PRs
Suggested labels
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
WASM runtime size check:Compared to target branchMoonbase runtime: 2368 KB (+4 KB) Moonbeam runtime: 2356 KB (+4 KB) Moonriver runtime: 2356 KB (no changes) ✅ Compared to latest release (runtime-3701)Moonbase runtime: 2368 KB (+312 KB compared to latest release) Moonbeam runtime: 2356 KB (+308 KB compared to latest release) Moonriver runtime: 2356 KB (+304 KB compared to latest release) |
Coverage Report@@ Coverage Diff @@
## master snowmead/migrate-currency-trait-to-fungible +/- ##
===============================================================================
+ Coverage 74.25% 74.30% +0.05%
+ Files 415 416 +1
+ Lines 103355 103893 +538
===============================================================================
+ Hits 76738 77196 +458
+ Misses 26617 26697 +80
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (2)
runtime/moonbase/src/weights/pallet_parachain_staking.rs (1)
20-40: 💡 Verification agent🧩 Analysis chain
Benchmark parameters are insufficient for production use
The benchmarking was performed with:
- Only 5 steps and 5 repeats (too low for accurate weight measurements)
- On a development machine (MacBook Pro)
- With unknown CPU specifications
For production deployments, weights should be generated with at least 50 steps and 20 repeats on reference hardware to ensure accurate and stable weight measurements.
🏁 Script executed:
#!/bin/bash # Check if there are any benchmarking configuration files or documentation fd -e md -e toml -e json . | xargs rg -l "benchmark|weight" | head -20Length of output: 693
Adjust Benchmark Parameters for Production Deployments
The autogenerated weights for
pallet_parachain_stakingwere generated with only 5 steps and 5 repeats on a local development machine. This setup is too low and non-representative for production use. Please:
- Re-run benchmarks using at least 50 steps and 20 repeats
- Execute on reference hardware (e.g., CI build agents or standardized lab machines) with documented CPU specs
- Update or add a benchmarking guide in the repository (e.g., in
README.mdor a dedicated docs file) detailing the recommended parameters and hardware requirementsLocations to update:
runtime/moonbase/src/weights/pallet_parachain_staking.rs(benchmark invocation flags)- Repository documentation (new or existing Markdown file)
runtime/moonbeam/src/weights/pallet_parachain_staking.rs (1)
20-40: 💡 Verification agent🧩 Analysis chain
Verify benchmark accuracy with reduced steps and repeats.
The benchmark parameters show
STEPS: 5, REPEAT: 5, which are relatively low values. Consider using higher values (e.g., 50 steps, 20 repeats) for production weight generation to ensure more accurate weight measurements.
🏁 Script executed:
#!/bin/bash # Check if these are standard benchmark parameters for this project fd -e rs "weights" --exec grep -l "STEPS.*REPEAT" {} \; | xargs grep -A 2 -B 2 "STEPS.*REPEAT"Length of output: 4560
Align benchmark parameters with project standard
The auto-generated weight file
runtime/moonbeam/src/weights/pallet_parachain_staking.rsis usingSTEPS: 5, REPEAT: 5, whereas all other pallets in this codebase (e.g.,pallets/xcm-transactor/src/weights.rs) benchmark withSTEPS: 50, REPEAT: 20. Running benchmarks with such low values can significantly impact the accuracy and stability of weight measurements.Please regenerate
pallet_parachain_staking.rsusing--steps 50 --repeat 20(or higher) for production-grade weights, or provide a clear justification for intentionally using reduced parameters.
- File needing update:
• runtime/moonbeam/src/weights/pallet_parachain_staking.rs (currentlySTEPS: 5, REPEAT: 5)
♻️ Duplicate comments (3)
pallets/parachain-staking/src/benchmarks.rs (2)
130-132: Same silent-error pattern duplicatedThe earlier comment applies here as well – propagate the
Resultinstead of discarding it.-// Use set_balance to set the account's balance directly -let _ = T::Currency::set_balance(&acc, initial_balance); +T::Currency::set_balance(&acc, initial_balance) + .expect("balance must be force-set for benchmark preparation");
1846-1847: Same double-reference issue for delegator balance check-<T::Currency as Inspect<T::AccountId>>::balance(&delegator) > initial_stake_amount, +<T::Currency as Inspect<T::AccountId>>::balance(delegator) > initial_stake_amount,runtime/moonriver/src/weights/pallet_parachain_staking.rs (1)
20-40:⚠️ Potential issueSame benchmarking concerns apply to Moonriver runtime
This file has the same issues as the Moonbase weights:
- Insufficient steps (5) and repeats (5)
- Generated on development hardware
- Unknown CPU specifications
Production weights must be generated on reference hardware with proper benchmarking parameters.
🧹 Nitpick comments (36)
test/suites/dev/moonbase/test-pov/test-precompile-over-pov2.ts (1)
74-76: Avoid brittle inline-snapshot forproofSize.Hard-coding the exact
block.proofSizemakes the test fragile against benign runtime tweaks (e.g. weight db updates) that don’t alter the contract logic. Consider asserting an upper bound instead:-expect(block.proofSize).toMatchInlineSnapshot(`109344`); +// keep a small safety margin under MAX_ETH_POV_PER_TX +expect(block.proofSize).to.be.at.most(110_000);This still guards against regressions but resists minor noise in proof size.
test/suites/dev/moonbase/test-staking/test-staking-locks4.ts (1)
7-8: Filename & title are now inconsistent.The suite title moved from “Locks” ➜ “Freezes”, but the file is still named
test-staking-locks4.ts. Renaming the file (and any imports/reference paths) will prevent confusion for future maintainers.E.g.
test-staking-freezes4.ts.runtime/moonbase/tests/integration_test.rs (1)
212-212: UpdatedFreezesmax_size reflects new freeze reasons
The change from the previous hard-coded size toSome(73)correctly accounts for the expanded set ofRuntimeFreezeReasonvariants after migrating from locks to freezes. Consider centralizing this value—e.g. deriving it from the enum’s variant count—to avoid future drift.test/README.md (1)
22-24: Specify language in fenced code block
The new build instructions block should declare its language for clarity:- ``` - cargo build --release --features metadata-hash - ``` + ```bash + cargo build --release --features metadata-hash + ```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
22-22: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
test/suites/dev/moonbase/test-staking/test-staking-locks2.ts (1)
22-26: Minor: use BigInt-aware equality helper
stakingFreezeandMIN_GLMR_STAKINGarebigints.
Chai’sequalassertion works with BigInt, butto.be.equaloccasionally widens toanyand loses type-safety.
For consistency across the suite you may prefer:- expect(stakingFreeze).to.be.equal(MIN_GLMR_STAKING); + expect(stakingFreeze).equal(MIN_GLMR_STAKING);Not blocking, purely stylistic.
test/suites/dev/moonbase/test-staking/test-genesis.ts (1)
18-22: Mixed assertion styles may confuse newcomersHere we pass a message as the second parameter to
expectand then chain.toBe(), which is Jest-style:expect(value, "msg").toBe(expected);Most other files use Chai (
expect(value).equal(expected)).
Consider standardising to one flavour to avoid subtle mismatches in behaviour / typings.test/suites/dev/moonbase/test-staking/test-staking-locks6.ts (1)
49-50: Consider asserting the number of freezes as wellYou already assert that the delegated amount remains frozen.
Adding an assertion on the count of staking freezes (e.g.expect(await getNumberOfDelegatorFreezes(...)).to.equal(1)) would protect against silent regressions where multiple redundant freezes are created for the same delegator.test/suites/dev/moonbase/test-locks/test-locks-multiple-locks.ts (1)
69-71: Nit: replace magic constant with helper to count freezesYou are already computing the delegator freeze amount.
For symmetry & future proofing, assert the freeze count as well (usinggetNumberOfDelegatorFreezes) instead of relying on the earlier manualfrozenBalancecomparison.test/suites/dev/moonbase/test-staking/test-staking-locks9.ts (1)
108-115: Style – keep camelCase for local variables
freeze_countbreaks the otherwise consistent camelCase style (maxDelegationsPerDelegator,stakingFreeze, …).
Rename tofreezeCountfor readability.-const freeze_count = await getNumberOfDelegatorFreezes(...); -expect(freeze_count).to.be.equal(1, "Should have 1 freeze"); +const freezeCount = await getNumberOfDelegatorFreezes(...); +expect(freezeCount).to.be.equal(1, "Should have 1 freeze");test/suites/dev/moonbase/test-staking/test-staking-locks.ts (1)
41-43: Strengthen the assertion by also validating the freeze type/countThe test currently proves that a positive freeze for the delegator exists and that its amount equals
MIN_GLMR_DELEGATOR.
However, it would still pass if multiple freezes are present (e.g. an unrelated pallet added one).
Adding a count-check makes the expectation tighter and protects against regressions introduced by other pallets.const delegatorFreeze = await getDelegatorStakingFreeze(randomAccount.address as `0x${string}`, context); expect(delegatorFreeze > 0n, "Missing freeze").to.be.true; expect(delegatorFreeze).to.be.equal(MIN_GLMR_DELEGATOR); + +const freezeCount = await context.helpers.getNumberOfDelegatorFreezes( + randomAccount.address as `0x${string}`, + context, +); +expect(freezeCount).to.equal( + 1, + "Exactly one ParachainStaking freeze should exist for a fresh delegation", +);test/suites/dev/moonbase/test-staking/test-staking-locks7.ts (2)
44-46: Minor naming / clarity nit – avoid shadowing generic “freeze”
const freeze = …is quickly followed by a laterfreeze_countvariable.
Renaming the first todelegatorFreeze(matching the helper) improves readability and eliminates shadowing in longer diffs.-const freeze = await getDelegatorStakingFreeze(randomAccount.address as `0x${string}`, context); -expect(freeze).to.be.equal(MIN_GLMR_DELEGATOR, "Freeze should have been added"); +const delegatorFreeze = await getDelegatorStakingFreeze( + randomAccount.address as `0x${string}`, + context, +); +expect(delegatorFreeze).to.equal( + MIN_GLMR_DELEGATOR, + "Freeze should have been added", +);
68-72: Prefer camel-case for local variables
freeze_count→freezeCountkeeps the codebase consistent with prevailing JS/TS camel-case style.-const freeze_count = await getNumberOfDelegatorFreezes(...); -expect(freeze_count).to.be.equal( +const freezeCount = await getNumberOfDelegatorFreezes(...); +expect(freezeCount).to.equal( 0, "Freeze should have been removed after executing revoke" );test/suites/dev/moonbase/test-staking/test-staking-locks12.ts (2)
91-99: Guard against premature loop exitIf, for any reason, the
freezes.multiresponse contains more thantopDelegators.lengthstaking freezes (e.g., duplicate or stale entries), the currentelsebranch passes silently.
Adding an explicit upper-bound assertion clarifies intent:-else { - expect(numDelegatorFreezes).to.eq(topDelegators.length, "More delegations than expected"); +else { + expect(numDelegatorFreezes).to.eq( + topDelegators.length, + "Unexpected number of staking freezes", + ); break; }
127-136: Mirror optimisation for bottom-delegator checkThe same
multibatching optimisation applies here; duplicating the logic keeps the suite execution time predictable.test/suites/dev/moonbase/test-staking/test-staking-locks11.ts (2)
60-62: Camel-case local variables for consistency-const freeze_count = await getNumberOfDelegatorFreezes(...); -expect(freeze_count).to.be.equal(1, "Should have 1 freeze"); +const freezeCount = await getNumberOfDelegatorFreezes(...); +expect(freezeCount).to.equal(1, "Should have 1 freeze");
87-88: ```diff
-const freeze_count_after = await getNumberOfDelegatorFreezes(...);
-expect(freeze_count_after).to.be.equal(0, "Freeze should be removed when delegator is bumped out");
+const freezeCountAfter = await getNumberOfDelegatorFreezes(...);
+expect(freezeCountAfter).to.equal(0, "Freeze should be removed when delegator is bumped out");</blockquote></details> <details> <summary>test/suites/dev/moonbase/test-staking/test-staking-locks8.ts (2)</summary><blockquote> `72-77`: **Nit – align variable naming & remove redundant assertions** `freeze_count` → `freezeCount` Also, `expect(freezeCount).to.equal(1)` already implies non-zero, so the earlier equality check is sufficient. ```diff -const freeze_count = await getNumberOfDelegatorFreezes(...); -expect(freeze_count).to.be.equal(1, "Should have 1 freeze"); -const freeze_amount = await getDelegatorStakingFreeze(...); -expect(freeze_amount).to.be.equal(2n * MIN_GLMR_DELEGATOR); +const freezeCount = await getNumberOfDelegatorFreezes(...); +expect(freezeCount).to.equal(1, "Should have 1 freeze"); +const freezeAmount = await getDelegatorStakingFreeze(...); +expect(freezeAmount).to.equal(2n * MIN_GLMR_DELEGATOR);
101-106: ```diff
-const freeze_count = await getNumberOfDelegatorFreezes(...);
-expect(freeze_count).to.be.equal(1, "Should still have freeze for remaining delegation");
-const stakingFreeze = await getDelegatorStakingFreeze(...);
-expect(stakingFreeze).to.be.equal(MIN_GLMR_DELEGATOR, "Should still have freeze for remaining delegation");
+const freezeCount = await getNumberOfDelegatorFreezes(...);
+expect(freezeCount).to.equal(1, "Should still have freeze for remaining delegation");
+const stakingFreeze = await getDelegatorStakingFreeze(...);
+expect(stakingFreeze).to.equal(MIN_GLMR_DELEGATOR, "Should still have freeze for remaining delegation");</blockquote></details> <details> <summary>test/suites/dev/moonbase/test-staking/test-staking-locks10.ts (3)</summary><blockquote> `15-16`: **File name no longer matches the test intent** The suite title was updated to reference *Freezes*, but the file is still named `test-staking-locks10.ts`. Renaming the file keeps the spec list clean and prevents future confusion when searching for freeze-related tests. ```diff - test-staking-locks10.ts + test-staking-freezes10.ts
71-78: Assertion message can be passed toexpect, notequalChai allows the optional message to be supplied as the second parameter to
expect, which reads a little cleaner:- expect(delegatorFreezeCount).to.be.equal( - 1, - `Should have only 1 delegator freeze, got ${delegatorFreezeCount}` - ); + expect( + delegatorFreezeCount, + `Should have only 1 delegator freeze, got ${delegatorFreezeCount}` + ).to.equal(1);Purely cosmetic—no functional impact.
86-90: Same minor style nit for the BigInt assertion- expect(stakingFreeze, `Unexpected amount for freeze`).to.be.equal( - 2n * MIN_GLMR_DELEGATOR - ); + expect( + stakingFreeze, + "Unexpected amount for freeze" + ).to.equal(2n * MIN_GLMR_DELEGATOR);Again, this is just syntactic polish; the existing assertion works.
pallets/parachain-staking/src/tests.rs (5)
27-37: Import list can be slimmed & made clearer
FreezeReasonandquery_freeze_amountare now imported fromcrate::palletandcrate::mockrespectively, which is fine, but the call-sites normally have to wrap the enum withinto()every time.
Creating a local alias once would remove the repetitive conversion noise and shorten the call-sites:-use crate::mock::{ - inflation_configs, query_freeze_amount, roll_blocks, roll_to, roll_to_round_begin, +use crate::mock::{ + inflation_configs, query_freeze_amount as q_frozen, roll_blocks, roll_to, + roll_to_round_begin, @@ - EnableMarkingOffline, Error, Event, FreezeReason, InflationDistributionInfo, Range, + EnableMarkingOffline, Error, Event, FreezeReason as FR, InflationDistributionInfo, Range,This lets each test call
q_frozen(acct, &FR::StakingDelegator.into())once, improving readability.
(Feel free to ignore if you prefer the current explicit style.)
5673-5680: Tiny readability tweak for repeated freeze checksThe same
FreezeReason::StakingDelegator.into()literal is passed twice in a row.
Declaring it once at the top of the block keeps the assertion lines compact:-assert_eq!( - query_freeze_amount(6, &FreezeReason::StakingDelegator.into()), - 40 -); +let staking = &FreezeReason::StakingDelegator.into(); +assert_eq!(query_freeze_amount(6, staking), 40);
7969-7989: Test name doesn’t match the behaviour
freezing_zero_amount_thaws_freezenever freezes zero – it freezes1then thaws it.
Either rename the test or actually callset_freeze(reason, …, 0)to document the edge-case implied by the title.-fn freezing_zero_amount_thaws_freeze() { +fn thaw_after_zero_freeze() { // or keep the name but freeze 0
7993-8031: Minor duplication in freeze-reason handlingSame suggestion as above: introduce a
let staking = &FreezeReason::StakingDelegator.into();once and reuse it for the threequery_freeze_amountchecks to de-duplicate the calls.
No functional impact, purely readability.
9092-9095: Hard-coded magic weight will break on the next re-benchmark
expected_on_initis a literal3036604790. When weights are regenerated the test will silently fail until the constant is hunted down and updated.Prefer asserting against the computed
expected_weightthat is already derived a few lines below and delete the extra literal:-let expected_on_init = 3036604790; -assert_eq!(Weight::from_parts(expected_on_init, 51554), weight); +assert_eq!(Weight::from_parts(expected_weight, 51554), weight);This keeps the test self-adjusting when new benchmark data is plugged in.
pallets/parachain-staking/src/benchmarks.rs (1)
2321-2375: Benchmark verification could assert full migration effectsThe benchmark only checks that
MigratedDelegatorsis populated. Consider additionally:
- Asserting that the legacy lock (
T::Currency::locks(delegator)) no longer exists.- Verifying that the new freeze exists with the expected amount/freeze-reason.
- Ensuring weight limits are respected when
x = 100.Augmenting the
verifyblock strengthens confidence that the migration extrinsic does the entire job.precompiles/parachain-staking/src/mock.rs (1)
110-113: Avoid self-referential associated-type indirection
Self::RuntimeFreezeReasonis defined just below, soVariantCountOf<Self::RuntimeFreezeReason>compiles, but the indirection is unnecessary and can confuse readers. Simplify:-type MaxFreezes = VariantCountOf<Self::RuntimeFreezeReason>; +type MaxFreezes = VariantCountOf<RuntimeFreezeReason>;(Same applies if other associated types reference
Self::RuntimeFreezeReason.)pallets/parachain-staking/src/test_lazy_migration.rs (3)
99-107: Remove trailing whitespace.There's trailing whitespace after the semicolon on line 102.
fn assert_freeze_amount_and_no_lock(account: AccountId, expected_amount: u128, is_collator: bool) { let freeze_reason = get_freeze_reason(is_collator); assert_eq!(query_freeze_amount(account, &freeze_reason), expected_amount); - + // Verify no corresponding lock remains let lock_id = if is_collator { COLLATOR_LOCK_ID } else { DELEGATOR_LOCK_ID }; assert!(!Balances::locks(&account).iter().any(|lock| lock.id == lock_id)); }
210-210: Remove unnecessary empty line.This empty line doesn't add to readability and can be removed.
386-386: Remove unnecessary empty line.This empty line doesn't add to readability and can be removed.
pallets/parachain-staking/src/types.rs (2)
465-471: Consider logging errors from freeze operations.While the freeze/thaw logic is correct, using
let _discards potential errors. Consider logging these for debugging purposes:// Update the freeze to the new total amount if self.bond.is_zero() { - let _ = <Pallet<T>>::thaw_extended(&who, true); + if let Err(e) = <Pallet<T>>::thaw_extended(&who, true) { + log::debug!("Failed to thaw collator freeze: {:?}", e); + } } else { - let _ = <Pallet<T>>::freeze_extended(&who, self.bond.into(), true); + if let Err(e) = <Pallet<T>>::freeze_extended(&who, self.bond.into(), true) { + log::debug!("Failed to update collator freeze: {:?}", e); + } }
1476-1520: Well-implemented freeze management for delegators.The method correctly:
- Validates stakable balance on increase
- Includes sanity check to prevent freezing more than total bond
- Properly sets/removes freezes based on the total amount
Consider applying the same error logging suggestion for line 1516 as mentioned in the previous comment.
test/helpers/staking-freezes.ts (2)
10-38: Consider improving type safety in freeze iteration.While the logic is correct, the use of
as anybypasses TypeScript's type system. Consider defining proper types for better maintainability:interface Freeze { id: { toHuman(): unknown; }; amount: { toBigInt(): bigint; }; } interface FreezeId { ParachainStaking?: "StakingCollator" | "StakingDelegator"; } export async function getStakingFreezes(account: `0x${string}`, context: DevModeContext) { const api = context.polkadotJs(); // Get all freezes for the account const freezes = await api.query.balances.freezes(account) as Freeze[]; let collatorFreeze = 0n; let delegatorFreeze = 0n; // Iterate through freezes to find ParachainStaking freezes for (const freeze of freezes) { if (api.events.parachainStaking && freeze.id) { // Check if this is a ParachainStaking freeze const freezeId = freeze.id.toHuman() as FreezeId; if (freezeId?.ParachainStaking === "StakingCollator") { collatorFreeze = freeze.amount.toBigInt(); } else if (freezeId?.ParachainStaking === "StakingDelegator") { delegatorFreeze = freeze.amount.toBigInt(); } } } return { collator: collatorFreeze, delegator: delegatorFreeze, total: collatorFreeze + delegatorFreeze }; }
110-160: Consider reducing code duplication in freeze counting functions.Both functions duplicate the freeze iteration logic. Consider refactoring to reduce duplication:
/** * Get freeze counts by type */ async function getFreezeCounts(account: `0x${string}`, context: DevModeContext) { const api = context.polkadotJs(); const freezes = await api.query.balances.freezes(account) as Freeze[]; let collatorCount = 0; let delegatorCount = 0; for (const freeze of freezes) { if (api.events.parachainStaking && freeze.id) { const freezeId = freeze.id.toHuman() as FreezeId; if (freezeId?.ParachainStaking === "StakingCollator") { collatorCount++; } else if (freezeId?.ParachainStaking === "StakingDelegator") { delegatorCount++; } } } return { collatorCount, delegatorCount }; } export async function getNumberOfCollatorFreezes(account: `0x${string}`, context: DevModeContext): Promise<number> { const counts = await getFreezeCounts(account, context); return counts.collatorCount; } export async function getNumberOfDelegatorFreezes(account: `0x${string}`, context: DevModeContext): Promise<number> { const counts = await getFreezeCounts(account, context); return counts.delegatorCount; }This reduces duplication while maintaining the same public API.
pallets/parachain-staking/src/lib.rs (1)
707-718: Add tracking for storage cleanup.The migration storage is correctly implemented. Consider adding a TODO with a target version for removal.
#[pallet::storage] /// Temporary storage to track candidates that have been migrated from locks to freezes. + /// TODO: Remove this storage after all accounts have been migrated (target: v1.x.x) /// This storage should be removed after all accounts have been migrated. pub(crate) type MigratedCandidates<T: Config> = StorageMap<_, Twox64Concat, T::AccountId, (), OptionQuery>; #[pallet::storage] /// Temporary storage to track delegators that have been migrated from locks to freezes. + /// TODO: Remove this storage after all accounts have been migrated (target: v1.x.x) /// This storage should be removed after all accounts have been migrated. pub(crate) type MigratedDelegators<T: Config> = StorageMap<_, Twox64Concat, T::AccountId, (), OptionQuery>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
.mcp.json(1 hunks)pallets/parachain-staking/src/benchmarks.rs(6 hunks)pallets/parachain-staking/src/inflation.rs(1 hunks)pallets/parachain-staking/src/lib.rs(16 hunks)pallets/parachain-staking/src/mock.rs(8 hunks)pallets/parachain-staking/src/test_lazy_migration.rs(1 hunks)pallets/parachain-staking/src/tests.rs(6 hunks)pallets/parachain-staking/src/types.rs(5 hunks)precompiles/parachain-staking/src/lib.rs(1 hunks)precompiles/parachain-staking/src/mock.rs(4 hunks)precompiles/parachain-staking/src/tests.rs(3 hunks)runtime/moonbase/src/lib.rs(4 hunks)runtime/moonbase/src/weights/pallet_parachain_staking.rs(39 hunks)runtime/moonbase/tests/integration_test.rs(1 hunks)runtime/moonbeam/src/lib.rs(4 hunks)runtime/moonbeam/src/weights/pallet_parachain_staking.rs(40 hunks)runtime/moonbeam/tests/integration_test.rs(1 hunks)runtime/moonriver/src/lib.rs(4 hunks)runtime/moonriver/src/weights/pallet_parachain_staking.rs(39 hunks)runtime/moonriver/tests/integration_test.rs(1 hunks)test/README.md(1 hunks)test/helpers/index.ts(1 hunks)test/helpers/staking-freezes.ts(1 hunks)test/suites/dev/moonbase/test-locks/test-locks-multiple-locks.ts(3 hunks)test/suites/dev/moonbase/test-pov/test-precompile-over-pov2.ts(1 hunks)test/suites/dev/moonbase/test-referenda/test-referenda-submit.ts(2 hunks)test/suites/dev/moonbase/test-staking/test-delegate-with-auto-compound7.ts(1 hunks)test/suites/dev/moonbase/test-staking/test-genesis.ts(2 hunks)test/suites/dev/moonbase/test-staking/test-rewards-auto-compound-pov.ts(1 hunks)test/suites/dev/moonbase/test-staking/test-staking-locks.ts(3 hunks)test/suites/dev/moonbase/test-staking/test-staking-locks10.ts(3 hunks)test/suites/dev/moonbase/test-staking/test-staking-locks11.ts(3 hunks)test/suites/dev/moonbase/test-staking/test-staking-locks12.ts(3 hunks)test/suites/dev/moonbase/test-staking/test-staking-locks2.ts(2 hunks)test/suites/dev/moonbase/test-staking/test-staking-locks4.ts(1 hunks)test/suites/dev/moonbase/test-staking/test-staking-locks6.ts(3 hunks)test/suites/dev/moonbase/test-staking/test-staking-locks7.ts(3 hunks)test/suites/dev/moonbase/test-staking/test-staking-locks8.ts(3 hunks)test/suites/dev/moonbase/test-staking/test-staking-locks9.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (14)
test/suites/dev/moonbase/test-staking/test-staking-locks6.ts (1)
test/helpers/staking-freezes.ts (1)
getDelegatorStakingFreeze(99-102)
test/suites/dev/moonbase/test-staking/test-staking-locks2.ts (1)
test/helpers/staking-freezes.ts (1)
getCollatorStakingFreeze(88-91)
test/suites/dev/moonbase/test-locks/test-locks-multiple-locks.ts (1)
test/helpers/staking-freezes.ts (1)
getDelegatorStakingFreeze(99-102)
test/suites/dev/moonbase/test-staking/test-genesis.ts (1)
test/helpers/staking-freezes.ts (1)
getCollatorStakingFreeze(88-91)
test/suites/dev/moonbase/test-staking/test-staking-locks9.ts (1)
test/helpers/staking-freezes.ts (2)
getNumberOfDelegatorFreezes(139-160)getDelegatorStakingFreeze(99-102)
test/suites/dev/moonbase/test-staking/test-staking-locks11.ts (1)
test/helpers/staking-freezes.ts (1)
getNumberOfDelegatorFreezes(139-160)
test/suites/dev/moonbase/test-staking/test-staking-locks10.ts (1)
test/helpers/staking-freezes.ts (2)
getNumberOfDelegatorFreezes(139-160)getDelegatorStakingFreeze(99-102)
test/suites/dev/moonbase/test-referenda/test-referenda-submit.ts (1)
test/helpers/staking-freezes.ts (1)
getDelegatorStakingFreeze(99-102)
test/suites/dev/moonbase/test-staking/test-staking-locks8.ts (1)
test/helpers/staking-freezes.ts (2)
getNumberOfDelegatorFreezes(139-160)getDelegatorStakingFreeze(99-102)
test/suites/dev/moonbase/test-staking/test-staking-locks12.ts (1)
test/helpers/staking-freezes.ts (1)
getDelegatorStakingFreeze(99-102)
pallets/parachain-staking/src/tests.rs (1)
pallets/parachain-staking/src/mock.rs (8)
query_freeze_amount(588-590)roll_blocks(310-316)roll_to(299-307)roll_to_round_begin(321-332)roll_to_round_end(336-347)set_author(577-580)set_block_author(583-585)default(193-218)
runtime/moonriver/src/weights/pallet_parachain_staking.rs (2)
pallets/parachain-staking/src/lib.rs (10)
get(578-580)get(2550-2552)candidate_bond_more(1152-1158)schedule_candidate_bond_less(1163-1177)execute_candidate_bond_less(1182-1188)set_candidate_bond_to_zero(1675-1683)cancel_candidate_bond_less(1193-1199)delegate_with_auto_compound(1212-1231)mint_collator_reward(2439-2452)migrate_locks_to_freezes_batch(1497-1516)runtime/moonbase/src/weights/pallet_parachain_staking.rs (8)
candidate_bond_more(333-344)schedule_candidate_bond_less(347-355)execute_candidate_bond_less(371-382)set_candidate_bond_to_zero(396-407)cancel_candidate_bond_less(410-418)delegate_with_auto_compound(780-795)mint_collator_reward(831-839)migrate_locks_to_freezes_batch(895-906)
runtime/moonbase/src/weights/pallet_parachain_staking.rs (3)
pallets/parachain-staking/src/lib.rs (10)
get(578-580)get(2550-2552)candidate_bond_more(1152-1158)schedule_candidate_bond_less(1163-1177)execute_candidate_bond_less(1182-1188)set_candidate_bond_to_zero(1675-1683)cancel_candidate_bond_less(1193-1199)delegate_with_auto_compound(1212-1231)mint_collator_reward(2439-2452)migrate_locks_to_freezes_batch(1497-1516)runtime/moonbase/src/lib.rs (2)
get(238-251)get(812-815)precompiles/parachain-staking/src/lib.rs (5)
candidate_bond_more(599-610)schedule_candidate_bond_less(614-625)execute_candidate_bond_less(629-644)cancel_candidate_bond_less(648-657)delegate_with_auto_compound(660-701)
pallets/parachain-staking/src/types.rs (1)
pallets/parachain-staking/src/lib.rs (3)
freeze_extended(1603-1621)thaw_extended(1627-1649)get_delegator_stakable_balance(1933-1937)
🪛 markdownlint-cli2 (0.17.2)
test/README.md
22-22: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (39)
runtime/moonbeam/tests/integration_test.rs (1)
233-238:Freezesmax_size update LGTM.The bump from
37➜73matches the additional bytes introduced by the newFreezeId + Reasonfields. No other StorageInfo entries changed, so the assertion remains coherent.runtime/moonriver/tests/integration_test.rs (1)
235-239: Approve updatedFreezesstorage size
The maximumFreezesstorage size has been increased fromSome(37)toSome(73)to reflect the newVariantCountOf<RuntimeFreezeReason>. This aligns with the migration from locks to freezes.pallets/parachain-staking/src/inflation.rs (1)
19-19: Switch toInspecttrait is correct
Replacing the deprecatedCurrencyimport withfungible::Inspectaligns this module with the freeze-based balance handling. No further changes needed.test/suites/dev/moonbase/test-staking/test-rewards-auto-compound-pov.ts (1)
105-109: UpdaterefTimethreshold to 216ms
TherefTimeupper limit and comment have been adjusted from214_000_000_000to216_000_000_000to account for new weight measurements. This change is consistent with observed block timings.test/helpers/index.ts (1)
31-31: Expose newstaking-freezeshelpers
Addingexport * from "./staking-freezes";makes the new freeze-based helper utilities available across tests.precompiles/parachain-staking/src/lib.rs (1)
29-38: ConfirmConfig::Currencysatisfies the newInspectboundThe alias now assumes that
<Runtime as pallet_parachain_staking::Config>::Currencyimplementsfungible::Inspect.
Please double-check the pallet’sConfigin the same branch to ensure theCurrencyassociated type indeed has anInspectbound (and no longer aCurrencybound only). A missing bound will compile-fail silently inside this precompile.If the pallet already declares
type Currency: Inspect<AccountId> + …;feel free to ignore this note.
.mcp.json (1)
1-13: Runtime path sanity-checkThe command points two directories above the repo root:
"../../polkadot-mcp-server/target/debug/polkadot-mcp"That binary & its
config.tomlwill not exist in CI containers or fresh clones unless the adjacent project is fetched & built beforehand.
Consider one of:- "command": "../../polkadot-mcp-server/target/debug/polkadot-mcp", + "command": "polkadot-mcp", // rely on $PATHor turn the path into a workspace member/out-dir that the build pipeline actually produces.
precompiles/parachain-staking/src/tests.rs (1)
688-689: Updated gas expectations – make sure weights were re-benchmarkedThe hard-coded gas figures (
267 586 506,271 882 065,137 000 000) changed.
Please confirm these numbers come from a freshcargo run --bin pallet-test-runtime --features=runtime-benchmarks(or your internal benchmarking script). Stale values will break automatically-weighted runtimes once another pallet touches the same storage keys.If these are provisional, drop the
.expect_cost(..)lines until final weights land.Also applies to: 756-758, 820-822
test/suites/dev/moonbase/test-locks/test-locks-multiple-locks.ts (1)
59-61: Double-check thefrozenfield name
AccountData on Substrate networks traditionally exposesmiscFrozen/feeFrozen.
A unifiedfrozenfield is not present in upstream types and may break when the API types auto-generate.
Verify that the runtime actually exposesdata.frozen; otherwise switch tomiscFrozen.max(feeFrozen).runtime/moonbase/src/lib.rs (2)
349-350: LGTM! Correct freeze configuration for balance migration.The configuration properly enables freeze functionality by using
RuntimeFreezeReasonand dynamically settingMaxFreezesto the variant count. This is the correct approach for supporting the migration from locks to freezes.
860-860: Correctly integrates freeze support into parachain staking.The addition of
RuntimeFreezeReasonto the staking config (line 860) and exposing theFreezeReasontrait inconstruct_runtime(line 1443) properly completes the freeze integration for the parachain staking pallet.Also applies to: 1443-1443
runtime/moonbeam/src/lib.rs (1)
343-344: Freeze configuration correctly mirrors moonbase runtime.The changes properly implement freeze support across all necessary components:
- Balances pallet freeze configuration (lines 343-344)
- Parachain staking freeze reason (line 848)
- FreezeReason trait exposure (line 1423)
This maintains consistency with the moonbase runtime configuration.
Also applies to: 848-848, 1423-1423
runtime/moonbase/src/weights/pallet_parachain_staking.rs (1)
150-173: Storage access patterns correctly reflect the migration stateThe addition of reads to
MigratedCandidatesand writes toFreezeswhile maintaining reads toLocksindicates proper lazy migration handling. The weight calculations now account for the additional storage operations needed during the migration period.runtime/moonriver/src/weights/pallet_parachain_staking.rs (1)
895-906:❓ Verification inconclusive
Migration weights are consistent across runtimes
The
migrate_locks_to_freezes_batchimplementation is consistent between Moonbase and Moonriver, with similar weight calculations and storage operations. The slight variations in weight values are expected due to runtime differences.Verify that both runtimes use the same migration batch size limits:
🏁 Script executed:
#!/bin/bash # Compare migration implementations across runtimes rg -B 2 -A 2 "TooLowCandidateCountWeightHintJoinCandidates.*100" pallets/parachain-staking/src/Length of output: 97
🏁 Script executed:
#!/bin/bash # Extract and compare `migrate_locks_to_freezes_batch` across Moonriver and Moonbase runtimes find runtime -type f -name "pallet_parachain_staking.rs" | while read file; do echo "=== $file ===" rg -n -A5 "fn migrate_locks_to_freezes_batch" "$file" || echo "No match in $file" echo doneLength of output: 1717
#!/bin/bash # Extract full `migrate_locks_to_freezes_batch` implementation from Moonbase and Moonriver runtimes for file in runtime/moonbase/src/weights/pallet_parachain_staking.rs runtime/moonriver/src/weights/pallet_parachain_staking.rs; do echo "=== $file ===" sed -n '/fn migrate_locks_to_freezes_batch/,/^[[:space:]]*}/p' "$file" echo done
Verify identical batch size constraints for migrate_locks_to_freezes_batch
While the implementations in Moonbase and Moonriver share the same structure and expected weight formula (with only environment-driven variations in the numeric weight parts), I did not find an explicit limit or constant that bounds the
xparameter (the batch size) in either runtime. Please confirm that both Moonbase and Moonriver enforce the same maximum batch size to avoid discrepancy in migration behavior.Affected files:
- runtime/moonbase/src/weights/pallet_parachain_staking.rs
- runtime/moonriver/src/weights/pallet_parachain_staking.rs
Check for any related configuration constant (e.g. a
MaxLocksMigrationBatchSizeor similar in the pallet’sConfigtrait) or type bound that constrainsxin both runtimes.runtime/moonriver/src/lib.rs (4)
55-55: LGTM!The import is correctly added to support the dynamic freeze count configuration.
351-352: Good use of VariantCountOf for dynamic freeze count.The configuration correctly enables freezes with typed reasons and dynamically sets the maximum number of freezes based on the enum variant count. This is more maintainable than hardcoding the value.
819-819: LGTM!The RuntimeFreezeReason type is correctly configured for the parachain staking pallet.
1433-1433: LGTM!The FreezeReason trait is correctly exposed in the runtime construction.
pallets/parachain-staking/src/test_lazy_migration.rs (2)
424-449: Good edge case coverage!Testing migration with zero balance is an important edge case that ensures the migration logic handles all scenarios correctly.
1-482: Comprehensive test coverage for lazy migration.The test suite thoroughly covers various migration scenarios including edge cases, batch operations, and state preservation. The helper functions are well-designed for test clarity.
runtime/moonbeam/src/weights/pallet_parachain_staking.rs (1)
886-908: LGTM! Weight function correctly implements linear scaling.The new
migrate_locks_to_freezes_batchweight function properly accounts for the linear complexity of migrating multiple accounts, with appropriate storage read/write counts per account.pallets/parachain-staking/src/types.rs (2)
19-23: LGTM! Clean removal of lock-related imports.The removal of
COLLATOR_LOCK_IDandDELEGATOR_LOCK_IDimports is consistent with the migration from locks to freezes.
444-448: Correct implementation of freeze for bond increase.The code properly freezes the total bond amount (current + additional) using the
freeze_extendedhelper with the collator flag set totrue. The error mapping toInsufficientBalancemaintains API compatibility.pallets/parachain-staking/src/mock.rs (6)
27-27: LGTM! Appropriate imports for freeze functionality.The addition of
InspectFreezeandVariantCountOftraits supports the new freeze-based implementation.
52-52: Correct addition of FreezeReason to pallet exports.Exposing
FreezeReasonis necessary for the runtime to use the staking-specific freeze reasons.
110-112: Proper freeze configuration for balances pallet.The configuration correctly sets up freeze support with appropriate type associations and uses
VariantCountOfto dynamically set the maximum number of freezes based on the freeze reason variants.
153-153: LGTM! Required freeze reason type for staking pallet.
587-590: Clean implementation of freeze query helper.The helper function properly uses the
InspectFreezetrait to query freeze amounts by reason, providing a direct replacement for the previous lock querying functionality.
616-698: Comprehensive test updates for freeze migration.All test assertions have been properly updated to use freeze queries instead of lock queries, with correct freeze reason construction for both collators and delegators.
test/helpers/staking-freezes.ts (1)
40-103: Well-structured utility functions.The helper functions provide a clean and consistent API for freeze queries. Consider applying the same type improvements to
getNumberOfFreezesas suggested for the main function.pallets/parachain-staking/src/lib.rs (9)
66-67: LGTM!Test module properly added and gated with
#[cfg(test)].
88-91: LGTM!Imports correctly added for freeze functionality while maintaining backward compatibility.
123-133: LGTM!Currency trait correctly extended with freeze functionality. Deprecation comment appropriately marks old traits for future removal.
209-217: LGTM!FreezeReason enum properly defined with clear distinction between collator and delegator freezes.
1603-1673: LGTM!Lazy migration helper functions are well-implemented with proper lock cleanup and freeze management.
1722-1722: LGTM!Correctly uses freeze_extended for collator staking.
2036-2046: LGTM!Mint functions correctly updated to use
mint_intowith proper account existence checks.Also applies to: 2430-2435, 2445-2450, 2465-2497
1888-1889: LGTM!Correctly uses thaw_extended to release frozen funds when leaving.
Also applies to: 1895-1896, 1915-1916
1933-1944: LGTM!Stakable balance functions correctly updated to use freeze-based calculations.
test/suites/dev/moonbase/test-referenda/test-referenda-submit.ts
Outdated
Show resolved
Hide resolved
test/suites/dev/moonbase/test-staking/test-delegate-with-auto-compound7.ts
Outdated
Show resolved
Hide resolved
Moonbase Weight Difference Report
Moonriver Weight Difference Report
Moonbeam Weight Difference Report
|
This reverts commit a9c61e7.
- Change balance_frozen_extended to return Option<Balance> and read
directly from storage instead of using freeze APIs
- Move test_lazy_migration.rs into tests/ subdirectory for better
organization
- Simplify mint() function to return Result for better error handling
- Update documentation to clarify "frozen" balance terminology
- Add TODO for reverting PoV weight limits post-migration
…ze amount respectively
| } | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
| Ok(()) | |
| Ok(Pays::No.into()) |
Usually, for lazy migrations, we refund fees when there are no errors to avoid charging legitimate callers.
Migrate Parachain Staking from Currency to Fungible Trait
This PR migrates the parachain staking pallet from the deprecated
Currencytrait to the modernFungibletrait, replacing the old lock-based system with a freeze-based mechanism.We used freeze operations instead of holds to keep the same behaviour as did locks.
Key Changes
Trait Migration: Updated the pallet to use
Fungibletrait withMutateFreezeinstead ofLockableCurrencyset_lock/remove_lockwithset_freeze/thawoperationsFreezeReasonenum withStakingCollatorandStakingDelegatorvariantsLazy Migration: Implemented automatic migration that converts accounts during their next interaction
MigratedCandidatesandMigratedDelegatorsstorage to track migration statusBatch Migration Extrinsic: Added
migrate_locks_to_freezes_batch(call index 33)Helper Functions: Added migration-aware wrappers
freeze_extended: Checks and migrates locks before freezingthaw_extended: Removes locks before thawingbalance_frozen_extended: Migrates and returns frozen balanceRuntime Impact
When the runtime is upgraded:
Testing
test_lazy_migration.rsAfter the migration, users who previously queried
Balances.Locksfor lock identifiers [stkngcol,stkngdel], should now queryBalances.Freezeswith the following freeze reasons [StakingCollator,StakingDelegator] instead. The frozen balance shown inSystem.Accountremains unaffected by this change.