-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Replace Currency->fungible migration for capacity pallet #1779
chore: Replace Currency->fungible migration for capacity pallet #1779
Conversation
97d61f3
to
1dad909
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments. I'm a bit worried about the total weight on test-net since it's around 40% of the block capacity + time-release changes alone and if another migration comes on top of those then it would be getting closer to the limit.
print(f"Connected to {chain.name}: {chain.chain} v{chain.version}") | ||
|
||
sender_uri = os.getenv('SENDER_URI', '//Alice') | ||
sender = Keypair.create_from_uri(sender_uri) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might work with testnet but who is going to pay for upgrades on main-net?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If more than 90% of the accounts need to be upgraded, then it is a free transaction. See here:
https://github.com/paritytech/polkadot-sdk/blob/f93f461acc24022bf7ef07c5913e620121776708/substrate/frame/balances/src/lib.rs#L685
…' of https://github.com/LibertyDSNP/frequency into 942-capacity-replace-currency-trait-with-fungible-trait
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Some small comments but no blockers. Amazing work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will run the migration as well but I wanted to get these comments in first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is blocking because of the referenced PR - I think we should discuss the implications of this comment first:
Freezes are essentially the same as locks, except that they overlap with Holds. Since Holds are designed to be infallibly slashed, this means that any logic using a Freeze must handle the possibility of the frozen amount being reduced, potentially to zero. A permissionless function should be provided in order to allow bookkeeping to be updated in this instance.
This PR adds no such permissionless function. If I'm reading this correctly, the frozen amount is somehow reduced to zero particularly through slashing, then the unlocks will need to be reduced or cleared. I think this is a nonzero possibility because a lot of Providers, who will be staking, will also be running their own nodes. Maybe they would use different collator keys than staking keys, and maybe they wouldn't. I'm not sure what such a function would look like or by what mechanism it would be called.
Re: reducible_balance
- based on my reading, this is <= balance
, and the only reason ever to use this is in operations where we don't need to keep the system account alive, and there are no instances of that here.
- Read through Deprecate
Currency
; introduce holds and freezing intofungible
traits paritytech/substrate#12951 to understand context and check that Currency traits were properly replaced with fungible traits. - Check impact of changing to set_freeze() and thaw() which can now fail and make sure all error states are propagated correctly without possibility for panic
- Check if balance() is used correctly, or should be changed to reducible_balance(). The calculations evaluating the Existential Deposit (ED) have been updated and Parity comments indicate that reducible_balance() is most likely the value needed.
- Ensure that the migration weights calculations are correct. (comments relating to this added already)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As agreed in the standup parking lot, let's create a new issue for the question I raised, which includes writing some tests to check the behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Tremendous effort. It is recommended that we release these heavier migration included PR's separately so that it would not overflow the block size on testnet.
for time-release pallet Initial code changes Split from PR #1779
for time-release pallet Initial code changes Split from PR #1779
…1818) # Goal The goal of this PR is to replace the `Currency` trait with the `fungible` trait in the `time-release` pallet. This work was split from PR #1779. Closes #942 Closes #1833 # Discussion The following Parity issues/PRs were used as references for changes: [Deprecate Currency - PR 12951](paritytech/substrate#12951) (Explanation of necessary changes.) [FRAME: Move pallets over to use fungible traits](paritytech/polkadot-sdk#226) (Issue to track Parity's efforts to update their pallets.) [pallet vesting / update to use fungible](https://github.com/paritytech/polkadot-sdk/pull/1760/files) (Example of some necessary changes.) [Fungibles: migrate Democracy pallet](paritytech/polkadot-sdk#1861) (Example of storage migration for Locks->Freezes.) # Changes - Replaced traits as needed: Use `tokens::fungible::` - `InspectFungible` for `balance()` and `reducible_balance()` - `InspectFreeze` for `balance_frozen()` - `Mutate` for `set_balance()`, `mint_into()` - `MutateFreeze` for `set_freeze()`, and `thaw()` - Added `pub enum FreezeReason` to support `freezes` - Updated error handling as `set_freeze()` and `thaw()` can fail, so errors needed to be propagated. - Updated runtime pallet configs to use BalancesMaxXXXXXs - Updated tests with `.expect()` where `set_freeze()` or `thaw()` can fail. - `FreezeIdentifier` and `RuntimeFreezeReason` configured with defaults. - Updated time-release pallet to propagate errors from set_freeze/thaw. - Added v2 migration to TimeRelease. # Storage Migrations The value of `BalancesMaxFreezes` has been updated, which will impact the storage of the Balances pallet by changing `T::MaxFreezes`, see the code here: [substrate/frame/balances/src/lib.rs:480](https://github.com/paritytech/polkadot-sdk/blob/release-polkadot-v1.1.0/substrate/frame/balances/src/lib.rs#L480) ```rust /// Freeze locks on account balances. #[pallet::storage] pub type Freezes<T: Config<I>, I: 'static = ()> = StorageMap< _, Blake2_128Concat, T::AccountId, BoundedVec<IdAmount<T::FreezeIdentifier, T::Balance>, T::MaxFreezes>, ValueQuery, >; ``` The previous value of `T::MaxFreezes` was `0` so no data could be stored in `Freezes`, therefore no storage migration for `Freezes` is needed for this change. Even if there was data in storage, it would only need to be migrated if `T::MaxFreezes` is *decreased*. However, the current chain has data in `Locks` that needs to migrated to `Freezes`. Testing has shown that these `Locks` will no longer be accessible once the new traits are in place. The `Balances` pallet is configured to store account data using the `System` pallet. Therefore, these two pallets must be included when using `try-runtime` for testing. The migration for `TimeRelease` will access its storage to determine which accounts have `Locks` that need to be translated to `Freezes`. Then, the old `Currency` trait is used to remove the `Locks` and the new `fungible` trait is used to set the `Freeze`. # How to Review - [x] Read through [Deprecate Currency - PR 12951](paritytech/substrate#12951) to understand context and check that Currency traits were properly replaced with fungible traits. - [ ] Check impact of changing to `set_freeze()` and `thaw()` which can now fail and make sure all error states are propagated correctly without possibility for `panic` - [ ] Check if `balance()` is used correctly, or should be changed to `reducible_balance()`. The calculations evaluating the Existential Deposit (ED) have been updated and Parity comments indicate that `reducible_balance()` is most likely the value needed. - [ ] Ensure that the migration weights calculations are correct. (Please let me know if you would like to walk through the migration code path together). # How to Test Runtime Migrations [Install the CLI version of try-runtime](https://paritytech.github.io/try-runtime-cli/try_runtime/#installation), then run try-runtime to test the migration against Frequency Rococo: ```bash cargo build --release --features frequency-rococo-testnet,try-runtime && \ try-runtime --runtime ./target/release/wbuild/frequency-runtime/frequency_runtime.wasm on-runtime-upgrade live --uri wss://rpc.rococo.frequency.xyz:443 --pallet TimeRelease --pallet Balances --pallet System ``` Alternatively, you can use the non-release version for faster compiles: ```bash cargo build --features frequency-rococo-testnet,try-runtime && \ try-runtime --runtime ./target/debug/wbuild/frequency-runtime/frequency_runtime.wasm on-runtime-upgrade live --uri wss://rpc.rococo.frequency.xyz:443 --pallet TimeRelease --pallet Balances --pallet System ``` And for testing on main-net: ```bash cargo build --features frequency,try-runtime && \ try-runtime --runtime ./target/debug/wbuild/frequency-runtime/frequency_runtime.wasm on-runtime-upgrade live --uri wss://1.rpc.frequency.xyz:443 --pallet Balances --pallet System --pallet TimeRelease ``` Testing with a snapshot: ```bash # create a snapshot (or use existing one) try-runtime create-snapshot --uri https:://rpc.rococo.frequency.xyz:443 testnet-all-pallets.state # use the testnet snapshot cargo build --features frequency-rococo-testnet,try-runtime && \ try-runtime --runtime ./target/debug/wbuild/frequency-runtime/frequency_runtime.wasm on-runtime-upgrade \ snap --path testnet-all-pallets.state # use the mainnet snapshot cargo build --features frequency,try-runtime && \ try-runtime --runtime ./target/debug/wbuild/frequency-runtime/frequency_runtime.wasm on-runtime-upgrade \ snap --path mainnet-all-pallets.state ``` You should see output like this: ```bash [2023-12-20T22:06:50Z INFO runtime::time-release] Running pre_upgrade... [2023-12-20T22:06:50Z INFO runtime::time-release] Finish pre_upgrade for 6 records [2023-12-20T22:06:50Z INFO runtime::time-release] Running storage migration... [2023-12-20T22:06:50Z INFO runtime::time-release] 🔄 Time Release Locks->Freezes migration started [2023-12-20T22:06:50Z INFO runtime::time-release] 🔄 migrated account 0x702cfcc9149d3c6f65728d3a5312d66a83fb70ed942cedb8e6450f4198ce7a77, amount:1000000000 [2023-12-20T22:06:50Z INFO runtime::time-release] 🔄 migrated account 0xce3bcb8ac19cdeb3ee14173be5f474292ff11ae56d4d0fa3f2bdaf24b4ef5842, amount:10000000000000 [2023-12-20T22:06:50Z INFO runtime::time-release] 🔄 migrated account 0x041a99f3614052bdd5b0aed6ed5805f592aacbcc0d5a443821f3b4339c44c11f, amount:400000050 [2023-12-20T22:06:50Z INFO runtime::time-release] 🔄 migrated account 0x26db9b4eeb5b5d511abd26903a25578f355e34e33316aeb2d34e846045cc7e45, amount:10000000000 [2023-12-20T22:06:50Z INFO runtime::time-release] 🔄 migrated account 0xc8d6262ff9fc322e59bcd36a36e310cfc7c50134e309a82f4330648e2eff7368, amount:1411 [2023-12-20T22:06:50Z INFO runtime::time-release] 🔄 migrated account 0x485dc3b17bcebba3013e47150e588c941a1f9778378367125e98a2e8f140325e, amount:3000000000 [2023-12-20T22:06:50Z INFO runtime::time-release] total accounts migrated from locks to frozen 6 [2023-12-20T22:06:50Z INFO runtime::time-release] 🔄 Time Release migration finished [2023-12-20T22:06:50Z INFO runtime::time-release] Time Release Migration calculated weight = Weight { ref_time: 4525000000, proof_size: 0 } [2023-12-20T22:06:50Z INFO runtime::time-release] ✅ migration post_upgrade checks passed ``` The total weight calculated for the TimeRelease migration on testnet: ```bash [2023-12-18T14:50:36Z INFO runtime::time-release] total accounts migrated from locks to frozen 6 [2023-12-18T14:50:36Z INFO runtime::time-release] 🔄 Time Release migration finished [2023-12-18T14:50:36Z INFO runtime::time-release] Time Release Migration calculated weight = Weight { ref_time: 4525000000, proof_size: 0 } [2023-12-18T14:50:36Z INFO runtime::time-release] ✅ migration post_upgrade checks passed ``` The total weight calculated for the TimeRelease migration on mainnet: ```bash [2023-12-18T14:57:04Z INFO runtime::time-release] 🔄 Time Release migration finished [2023-12-18T14:57:04Z INFO runtime::time-release] Time Release Migration calculated weight = Weight { ref_time: 16525000000, proof_size: 0 } [2023-12-18T14:57:04Z INFO runtime::time-release] ✅ migration post_upgrade checks passed ``` # Upgrade Notes 1. `scripts/upgrade_accounts.py` should be executed to ensure that all accounts have been upgraded before running the migration. This step may be a no-op, if all accounts have previously been upgraded. # Checklist - [x] Chain spec updated - [ ] Custom RPC OR Runtime API added/changed? Updated js/api-augment. - [ ] Design doc(s) updated - [ ] Tests added - [ ] Benchmarks added - [ ] Weights updated --------- Co-authored-by: Matthew Orris <--help> Co-authored-by: Aramik <aramikm@gmail.com> Co-authored-by: Robert La Ferla <robert@onemsg.co>
Goal
The goal of this PR is to replace the
Currency
trait with thefungible
trait in thecapacity
pallet.Closes #1532
Discussion
The following Parity issues/PRs were used as references for changes:
Deprecate Currency - PR 12951 (Explanation of necessary changes.)
FRAME: Move pallets over to use fungible traits (Issue to track Parity's efforts to update their pallets.)
pallet vesting / update to use fungible (Example of some necessary changes.)
Fungibles: migrate Democracy pallet (Example of storage migration for Locks->Freezes.)
Changes
.cargo-deny.toml
Added "multiformats" forcid
crate to fix warning.vscode/settings.json
Added script that sets up the source map for the rust std library files for debugging.Cargo.lock
cargo updated.e2e/package-lock.json
npm updated.tokens::fungible::
InspectFungible
forbalance()
andreducible_balance()
InspectFreeze
forbalance_frozen()
Mutate
forset_balance()
,mint_into()
MutateFreeze
forset_freeze()
, andthaw()
pub enum FreezeReason
to supportfreezes
set_freeze()
andthaw()
can fail, so errors needed to be propagated..expect()
whereset_freeze()
orthaw()
can fail.FreezeIdentifier
andRuntimeFreezeReason
configured with defaults.Storage Migrations
The value of
BalancesMaxFreezes
has been updated, which will impact the storage of the Balances pallet by changingT::MaxFreezes
, see the code here: substrate/frame/balances/src/lib.rs:480The previous value of
T::MaxFreezes
was0
so no data could be stored inFreezes
, therefore no storage migration forFreezes
is needed for this change. Even if there was data in storage, it would only need to be migrated ifT::MaxFreezes
is decreased.However, the current chain has data in
Locks
that needs to migrated toFreezes
. Testing has shown that theseLocks
will no longer be accessible once the new traits are in place.The
Balances
pallet is configured to store account data using theSystem
pallet. Therefore, these two pallets must be included when usingtry-runtime
for testing.The migration for
Capacity
will access its storage to determine which accounts haveLocks
that need to be translated toFreezes
. Then, the oldCurrency
trait is used to remove theLocks
and the newfungible
trait is used to set theFreeze
.How to Review
set_freeze()
andthaw()
which can now fail and make sure all error states are propagated correctly without possibility forpanic
balance()
is used correctly, or should be changed toreducible_balance()
. The calculations evaluating the Existential Deposit (ED) have been updated and Parity comments indicate thatreducible_balance()
is most likely the value needed.How to Test Runtime Migrations
Install the CLI version of try-runtime, then run try-runtime to test the migration against Frequency Rococo:
cargo build --release --features frequency-rococo-testnet,try-runtime && \ try-runtime --runtime ./target/release/wbuild/frequency-runtime/frequency_runtime.wasm on-runtime-upgrade live --uri wss://rpc.rococo.frequency.xyz:443 -pallet Capacity --pallet Balances --pallet System
Alternatively, you can use the non-release version for faster compiles:
cargo build --features frequency-rococo-testnet,try-runtime && \ try-runtime --runtime ./target/debug/wbuild/frequency-runtime/frequency_runtime.wasm on-runtime-upgrade live --uri wss://rpc.rococo.frequency.xyz:443 -pallet Capacity --pallet Balances --pallet System
You should see output like this:
The total weight calculated for the Capacity migration on testnet:
[2023-12-18T14:50:36Z INFO runtime::capacity] total accounts migrated from locks to freezes: 347 [2023-12-18T14:50:36Z INFO runtime::capacity] 🔄 Capacity Locks->Freezes migration finished [2023-12-18T14:50:36Z INFO runtime::capacity] Capacity Migration calculated weight = Weight { ref_time: 260375000000, proof_size: 0 } [2023-12-18T14:50:36Z INFO runtime::capacity] ✅ migration post_upgrade checks passed
The total weight calculated for the Capacity migration on main net:
[2023-12-18T14:57:04Z INFO runtime::capacity] 🔄 Capacity Locks->Freezes migration finished [2023-12-18T14:57:04Z INFO runtime::capacity] Capacity Migration calculated weight = Weight { ref_time: 1625000000, proof_size: 0 } [2023-12-18T14:57:04Z INFO runtime::capacity] ✅ migration post_upgrade checks passed
Upgrade Notes
scripts/upgrade_accounts.py
should be executed to ensure that all accounts have been upgraded before running the migration.Checklist