Skip to content
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

Replace Currency->fungible trait migration for time-release pallet #1818

Merged
merged 14 commits into from
Jan 5, 2024

Conversation

mattheworris
Copy link
Collaborator

@mattheworris mattheworris commented Dec 18, 2023

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

  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

  • Chain spec updated
  • Custom RPC OR Runtime API added/changed? Updated js/api-augment.
  • Design doc(s) updated
  • Tests added
  • Benchmarks added
  • Weights updated

@mattheworris mattheworris self-assigned this Dec 18, 2023
Copy link

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

Copy link

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

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

codecov bot commented Dec 18, 2023

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.

@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Dec 18, 2023
@github-actions github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Dec 20, 2023
@mattheworris mattheworris force-pushed the capcity-fungible-trait-time-release branch from 23c3d8c to 33a70c6 Compare December 20, 2023 21:29
@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented and removed metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented labels Dec 20, 2023
for time-release pallet

Initial code changes
Split from PR #1779
@mattheworris mattheworris force-pushed the capcity-fungible-trait-time-release branch from fcabe1b to e449930 Compare December 20, 2023 22:45
@mattheworris mattheworris requested a review from a team December 20, 2023 22:52
@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Dec 20, 2023
@github-actions github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Dec 21, 2023
@mattheworris mattheworris marked this pull request as ready for review December 21, 2023 15:39
@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Dec 21, 2023
@github-actions github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Dec 21, 2023
Copy link
Collaborator

@aramikm aramikm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

@aramikm aramikm requested a review from a team December 21, 2023 22:28
@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Dec 21, 2023
@github-actions 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
@github-actions 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
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
Copy link
Collaborator

@aramikm aramikm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@github-actions github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Jan 4, 2024
@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Jan 4, 2024
Copy link
Collaborator

@shannonwells shannonwells left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

enddynayn commented Jan 5, 2024

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

Balances::transfer becomes Balances::transfer_allow_death

@mattheworris
Copy link
Collaborator Author

mattheworris commented Jan 5, 2024

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.

Copy link
Collaborator

@JoeCap08055 JoeCap08055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no blockers for me; minor doc comment suggestion

@github-actions github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Jan 5, 2024
} else {
Self::update_lock(who, locked);
Self::update_freeze(who, frozen)?;
Copy link
Collaborator

@enddynayn enddynayn Jan 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Jan 5, 2024
@enddynayn
Copy link
Collaborator

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

Copy link
Collaborator

@enddynayn enddynayn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇 Awesome!

Copy link
Collaborator

@saraswatpuneet saraswatpuneet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@mattheworris mattheworris merged commit 35c2346 into main Jan 5, 2024
30 checks passed
@mattheworris mattheworris deleted the capcity-fungible-trait-time-release branch January 5, 2024 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metadata-changed Metadata has changed since the latest full release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore: Update makefile target ci-local to include js [Capacity] replace currency trait with fungible trait
7 participants