Replace Currency->fungible trait migration for time-release pallet by mattheworris · Pull Request #1818 · frequency-chain/frequency

mattheworris · 2023-12-18T20:50:55Z

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 (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

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

	/// 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

Read through Deprecate Currency - PR 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, 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 TimeRelease --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 TimeRelease --pallet Balances --pallet System

And for testing on main-net:

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:

# 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:

[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:

[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:

[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

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

github-actions · 2023-12-18T20:52:13Z

⏳ Running benchmarks and calculating weights. DO NOT MERGE! A new commit will be added upon completion...

github-actions · 2023-12-18T21:22:41Z

✅ Finished running benchmarks. Updated weights have been committed to this PR branch in commit e77de65.

codecov · 2023-12-18T21:54:48Z

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (14d4388) 82.37% compared to head (9e2b91f) 82.70%.

Files	Patch %	Lines
pallets/time-release/src/migration/v2.rs	94.89%	5 Missing ⚠️
pallets/time-release/src/lib.rs	97.22%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1818      +/-   ##
==========================================
+ Coverage   82.37%   82.70%   +0.33%     
==========================================
  Files          53       54       +1     
  Lines        4090     4192     +102     
==========================================
+ Hits         3369     3467      +98     
- Misses        721      725       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

for time-release pallet Initial code changes Split from PR #1779

…capcity-fungible-trait-time-release

… error for Charlie

aramikm

Ran the try-runtime and from migration perspective everything looked good to me except total weights. Which based on current calculation if we only use the NORMAL_DISPATCH_RATIO in a block (375000000000) we would get 98% full which is a bit close to what I'm comfortable with. One solution might be to combine v2 and v3 migrations on capacity pallet since both of them iterate on StakingAccountLedger

>>> test_release = 4525000000
>>> test_schemas= 25000000000
>>> test_capacity=260375000000
>>> test_capacity_2=78200000000
>>> (test_release+test_schemas+test_capacity+test_capacity_2) / 500000000000 * 100
73.61999999999999
>>> (test_release+test_schemas+test_capacity+test_capacity_2) / 375000000000 * 100
98.16

Will recommend somebody else with more context about time-release pallet take another look on that part.

pallets/time-release/src/lib.rs

…rget

pallets/time-release/README.md

pallets/time-release/src/lib.rs

aramikm

Great effort!

Tested:

General review of the code
Ran the try-runtime migration against the snapshots of test-net and main-net

Co-authored-by: Robert La Ferla <robert@onemsg.co>

pallets/time-release/src/migration/v2.rs

shannonwells

Lots of good work here.

I reviewed the code for correctness
I ran two of try-runtime - against mainnet and testnet.

Agree regarding deferring this to another release.

enddynayn · 2024-01-05T17:56:19Z

I think we might need to update Currency::transfer?
paritytech/substrate#12951

Balances::transfer becomes Balances::transfer_allow_death

pallets/time-release/src/lib.rs

mattheworris · 2024-01-05T18:08:05Z

I think we might need to update Currency::transfer? paritytech/substrate#12951

Balances::transfer becomes Balances::transfer_allow_death

Here we are using the fungible::transfer which comes from Mutate not the Balances trait.
frame/support/src/traits/tokens/fungible/regular.rs

I think using fungible:transfer is correct as per the comment following the name change in 12951:

In general, it's now better to use the fungible trait API for Balances rather than messing with the dispatchables directly.

JoeCap08055

no blockers for me; minor doc comment suggestion

enddynayn · 2024-01-05T18:29:52Z

pallets/time-release/src/lib.rs

+			Self::delete_freeze(who)?;
 		} else {
-			Self::update_lock(who, locked);
+			Self::update_freeze(who, frozen)?;


Not worth the change but good to know that if we set the update_freeze frozen amount to zero it is the same thing as calling thaw. We can simplify this by removing the if statement and keeping update_freeze.

enddynayn · 2024-01-05T19:25:11Z

👍 🚀 Amazing work. Thanks for having a really good commit message. I think this is going to be useful for other migrations. 🥇

enddynayn

🥇 Awesome!

saraswatpuneet

lgtm!

mattheworris self-assigned this Dec 18, 2023

demisx pushed a commit that referenced this pull request Dec 18, 2023

Update weights for PR #1818

e77de65

github-actions bot added the metadata-changed Metadata has changed since the latest full release label Dec 18, 2023

mattheworris mentioned this pull request Dec 19, 2023

Investigate fungible trait Holds and Freezes overlap. #1819

Closed

6 tasks

github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Dec 20, 2023

mattheworris force-pushed the capcity-fungible-trait-time-release branch from 23c3d8c to 33a70c6 Compare December 20, 2023 21:29

chore: Replace Currency->fungible: update pallet_capacity with fungible

e449930

for time-release pallet Initial code changes Split from PR #1779

mattheworris force-pushed the capcity-fungible-trait-time-release branch from fcabe1b to e449930 Compare December 20, 2023 22:45

Merge branch 'main' of https://github.com/LibertyDSNP/frequency into …

fc5c0b7

…capcity-fungible-trait-time-release

mattheworris requested a review from a team December 20, 2023 22:52

Matthew Orris added 2 commits December 20, 2023 15:53

fix: cargo update

da65739

fix: refactor migration test to execute without feature try-runtime

595b553

github-actions bot added the metadata-changed Metadata has changed since the latest full release label Dec 20, 2023

fix: update ensure_can_withdraw to use can_withdraw; fix mock balance…

f97e24d

… error for Charlie

github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Dec 21, 2023

mattheworris marked this pull request as ready for review December 21, 2023 15:39

mattheworris requested a review from wilwade as a code owner December 21, 2023 15:39

github-actions bot added the metadata-changed Metadata has changed since the latest full release label Dec 21, 2023

addressed some storage migration concerns

2a841d3

github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Dec 21, 2023

aramikm reviewed Dec 21, 2023

View reviewed changes

aramikm requested a review from a team December 21, 2023 22:28

github-actions bot added the metadata-changed Metadata has changed since the latest full release label Dec 21, 2023

github-actions bot added metadata-changed Metadata has changed since the latest full release and removed metadata-changed Metadata has changed since the latest full release labels Jan 2, 2024

mattheworris commented Jan 2, 2024

View reviewed changes

pallets/time-release/src/lib.rs Outdated Show resolved Hide resolved

fix: rename lock/unlock references to freeze/thaw; update ci-local ta…

cabb0f6

…rget

github-actions bot added metadata-changed Metadata has changed since the latest full release and removed metadata-changed Metadata has changed since the latest full release labels Jan 3, 2024

rlaferla reviewed Jan 3, 2024

View reviewed changes

pallets/time-release/README.md Outdated Show resolved Hide resolved

pallets/time-release/README.md Outdated Show resolved Hide resolved

pallets/time-release/src/lib.rs Outdated Show resolved Hide resolved

aramikm approved these changes Jan 4, 2024

View reviewed changes

fix: Apply suggestions from code review

b6b2ab9

Co-authored-by: Robert La Ferla <robert@onemsg.co>

github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Jan 4, 2024

saraswatpuneet reviewed Jan 4, 2024

View reviewed changes

pallets/time-release/src/migration/v2.rs Show resolved Hide resolved

saraswatpuneet reviewed Jan 4, 2024

View reviewed changes

pallets/time-release/src/migration/v2.rs Outdated Show resolved Hide resolved

github-actions bot added the metadata-changed Metadata has changed since the latest full release label Jan 4, 2024

shannonwells approved these changes Jan 4, 2024

View reviewed changes

JoeCap08055 reviewed Jan 5, 2024

View reviewed changes

pallets/time-release/src/lib.rs Outdated Show resolved Hide resolved

JoeCap08055 approved these changes Jan 5, 2024

View reviewed changes

rlaferla approved these changes Jan 5, 2024

View reviewed changes

fix: address PR comments

9e2b91f

github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Jan 5, 2024

enddynayn reviewed Jan 5, 2024

View reviewed changes

github-actions bot added the metadata-changed Metadata has changed since the latest full release label Jan 5, 2024

enddynayn approved these changes Jan 5, 2024

View reviewed changes

saraswatpuneet approved these changes Jan 5, 2024

View reviewed changes

mattheworris merged commit 35c2346 into main Jan 5, 2024

mattheworris deleted the capcity-fungible-trait-time-release branch January 5, 2024 20:07

mattheworris mentioned this pull request Feb 5, 2024

Upgrade Accounts after upgrading Currency trait to fungible. #1868

Closed

4 tasks

mattheworris mentioned this pull request Jun 24, 2024

Pallet Unit Tests concerning Balances: Modify mock.rs to have ED > 1, to test balances less than ED. #1852

Closed

2 tasks

Conversation

mattheworris commented Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Goal

Discussion

Changes

Storage Migrations

How to Review

How to Test Runtime Migrations

Upgrade Notes

Checklist

Uh oh!

github-actions bot commented Dec 18, 2023

Uh oh!

github-actions bot commented Dec 18, 2023

Uh oh!

codecov bot commented Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aramikm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aramikm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shannonwells left a comment

Choose a reason for hiding this comment

Uh oh!

enddynayn commented Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mattheworris commented Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoeCap08055 left a comment

Choose a reason for hiding this comment

Uh oh!

enddynayn Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enddynayn commented Jan 5, 2024

Uh oh!

enddynayn left a comment

Choose a reason for hiding this comment

Uh oh!

saraswatpuneet left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mattheworris commented Dec 18, 2023 •

edited

Loading

codecov bot commented Dec 18, 2023 •

edited

Loading

aramikm left a comment •

edited

Loading

enddynayn commented Jan 5, 2024 •

edited

Loading

mattheworris commented Jan 5, 2024 •

edited

Loading

enddynayn Jan 5, 2024 •

edited

Loading