Feat/separate unlock chunks #1753 #1763

shannonwells · 2023-11-07T02:14:11Z

Goal

The goal of this PR is to separate unstaking unlock chunks into their own storage and do a storage migration which includes staking type, for convenience of the planned Provider Boost feature so that another storage migration is not needed for that feature. There are multiple advantages to this separation discussed in the linked issue.

Closes #1753 . Please see the linked issue for more details about this proposed change.

Discussion

A stake will require 1 extra read of unstaking unlocks from before, in order to set the account token lock correctly. This was not noted in the original issue.

As noted in the original issue, an unstake will require 1 extra read and 1 extra write from before.

Effectively changes the name of StakingAccountDetails to just StakingDetails while splitting the storage.

StakingAccountLedger now stores StakingDetails.

The migration uses the translate function of the IterableStorageMap trait.

The implementation design document was updated to make it more clear that details of actual ProviderBoost staking rewards are undecided and unspecified.

Since token locks are being touched, please review with an eye toward bugs with token locks and test coverage.

To Verify

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 -p Capacity

You should see at something like:

[2023-11-15T01:32:16Z INFO  runtime::capacity] 🔄 StakingAccountLedger migration started
[2023-11-15T01:32:16Z INFO  runtime::capacity] migrated 1
[2023-11-15T01:32:16Z INFO  runtime::capacity] migrated 2
...
[2023-11-15T01:32:16Z INFO  runtime::capacity] migrated 335
[2023-11-15T01:32:16Z INFO  runtime::capacity] migrated 336
[2023-11-15T01:32:16Z INFO  runtime::capacity] 🔄 migration finished
[2023-11-15T01:32:16Z INFO  runtime::capacity] Migration calculated weight = Weight { ref_time: 75725000000, proof_size: 0 }
[2023-11-15T01:32:16Z INFO  runtime::capacity] ✅ migration post_upgrade checks passed

The errors afterward are unrelated to the migration, having to do with a runtime difference between local and rococo, and can be ignored.

Checklist

Storage migration added
Storage migration tested via try_runtime
Chain spec updated
~~Custom RPC OR Runtime API added/changed? Updated js/api-augment.~~
Design doc(s) updated
Tests added
Benchmarks updated (need to add requested changes)
Weights updated (need to rerun with requested changes)

codecov · 2023-11-07T02:28:58Z

Codecov Report

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

Comparison is base (9691055) 87.62% compared to head (f0675d1) 87.70%.

Files	Patch %	Lines
pallets/capacity/src/migration/v2.rs	85.71%	5 Missing ⚠️
pallets/capacity/src/lib.rs	98.63%	1 Missing ⚠️
pallets/capacity/src/types.rs	96.87%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1763      +/-   ##
==========================================
+ Coverage   87.62%   87.70%   +0.08%     
==========================================
  Files          52       53       +1     
  Lines        4346     4410      +64     
==========================================
+ Hits         3808     3868      +60     
- Misses        538      542       +4

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

shannonwells · 2023-11-14T01:28:09Z

pallets/capacity/src/lib.rs

+			},
+		)?;
+		let staking_account = Self::get_staking_account_for(staker).unwrap_or_default();
+		let total_locked = staking_account.active.saturating_add(total_unlocking);


total_locked will be zero if they've already completely unstaked (in which case staking_account.active is the default of 0), and all unstake unlocks have thawed (in which case total_unlocking never gets modified from 0).

@aramikm note here unwrap_or_default to takes care of the case described above so staking_account.active would be 0.

github-actions · 2023-11-14T22:49:00Z

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

aramikm

Great effort! Checked migrations and added some comments. Was not able to contextually check the math for changes since I don't have much context.

pallets/capacity/src/benchmarking.rs

pallets/capacity/src/lib.rs

pallets/capacity/src/migration/v2.rs

github-actions · 2023-11-14T23:17:16Z

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

aramikm

Looks great!

github-actions · 2023-11-15T21:58:55Z

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

github-actions · 2023-11-30T21:12:37Z

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

github-actions · 2023-11-30T21:32:14Z

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

github-actions · 2023-11-30T22:02:44Z

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

enddynayn · 2023-11-30T22:21:35Z

pallets/capacity/src/lib.rs

@@ -206,9 +214,15 @@ pub mod pallet {
 	pub type EpochLength<T: Config> =
 		StorageValue<_, BlockNumberFor<T>, ValueQuery, EpochLengthDefault<T>>;

+	#[pallet::storage]
+	#[pallet::getter(fn get_unstake_unlocking_for)]
+	pub type UnstakeUnlocks<T: Config> =


why not name it UnlockChunks?

enddynayn · 2023-11-30T22:26:45Z

pallets/capacity/src/types.rs

@@ -237,3 +178,48 @@ pub struct EpochInfo<BlockNumber> {
 	/// The block number when this epoch started.
 	pub epoch_start: BlockNumber,
 }
+
+/// A BoundedVec containing UnlockChunks
+pub type UnlockChunkList<T> = BoundedVec<


why do we need a new type for this? There is already UnstakeUnlock.

UnstakeUnlock is the name for the storage. UnlockChunkList is just an alias for readability.

I think this makes it less readable why do we need it? We do not do this anywhere else? This type does not really does not have an impl.

Rehash of our conversation earlier: the UnlockChunkList (not the greatest name I know) is used in the 3 fns in types.rs that take it as a parameter and also referenced by the declaration of UnstakeUnlocks.

pallets/capacity/src/lib.rs

enddynayn · 2023-11-30T22:51:27Z

pallets/capacity/src/lib.rs

+		}
+		let staking_account = Self::get_staking_account_for(staker).unwrap_or_default();
+		let total_locked = staking_account.active.saturating_add(total_unlocking);
+		if total_locked.is_zero() {


I think we should keep the set delete method interface

delete_staking_account set_staking_account

I am concern that someone is going to use these directly at some point and mess up the accounting. That why I wrapped does in a function.

I'm not sure what you think will be "used directly." and how someone would mess up the accounting, nor how an encapsulation would fix that. I think encapsulating one-liner functions that are called only from one place is not a good way to prevent others from making calls they should not make. If anything it communicates that the function is intended to be called by multiple functions.

As for update/delete/set staking account, keeping all that logic together is not possible because it was called for both staking and unstaking. The refactor meant that the 'set_staking_account' logic, which still has locking logic, is called only on a stake. Now withdraw_unstake no longer changes the staking account. To me it's clearer and simpler to leave the logic only where it's used, and the do_withdraw_unstake function is pretty short and easy to read. I see no benefit in making this function call at least 2 new, short functions.

Ok can we please wrap these in their own functions. It will make it easier for updating for the new fungible trait. We use set_lock with WithdrawReason::all twice. Its definitely nicer.

ah ok, I will do a follow-up then. I did do a small refactor that lets you call set_staking_account which deletes it if there is a zero active balance, but all that does now is set the staking account, and it's called in 3 places. The original is renamed to be more like what it does, set_staking_account_and_lock

enddynayn · 2023-11-30T22:54:48Z

pallets/capacity/src/lib.rs

+		let amount_withdrawn = unlock_chunks_reap_thawed::<T>(&mut unlocks, current_epoch);
+		ensure!(!amount_withdrawn.is_zero(), Error::<T>::NoThawedTokenAvailable);
+		if unlocks.is_empty() {
+			UnstakeUnlocks::<T>::set(staker, None);


I think we should wrap these too in a method and not use it directly it. It makes easier for updates and changes.

enddynayn · 2023-11-30T22:56:38Z

pallets/capacity/src/lib.rs

+			.min(proposed_amount)
+	}
+
+	pub(crate) fn do_withdraw_unstaked(


I like how you created this function and simplified the do_withdraw_unstaked.

enddynayn · 2023-12-01T01:02:37Z

pallets/capacity/src/lib.rs

+		Ok(actual_unstaked_amount)
+	}
+
+	fn add_unlock_chunk(


set_unlock_chunk or to be consistent with your naming set_unstake_unlock_chunk. Although, this is the reason why I like UnlockChunk instead of unstake_unlock because it seems more natural.

enddynayn · 2023-12-01T01:07:15Z

pallets/capacity/src/lib.rs

-		T::Currency::remove_lock(STAKING_ID, &staker);
-		StakingAccountLedger::<T>::remove(&staker);
+	/// Sets staking account details after a deposit
+	fn set_staking_account_and_lock(


I like this function

enddynayn

👍

shannonwells added the change/storage-migration PR has a Storage Migration label Nov 7, 2023

shannonwells force-pushed the feat/separate-unlock-chunks-#1753 branch 3 times, most recently from 094a613 to 1c35112 Compare November 14, 2023 00:45

shannonwells commented Nov 14, 2023

View reviewed changes

shannonwells added documentation Improvements or additions to documentation benchmark Extrensic benchmark needed change/major Major Changes in this PR and removed benchmark Extrensic benchmark needed labels Nov 14, 2023

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

shannonwells marked this pull request as ready for review November 14, 2023 17:01

shannonwells requested a review from wilwade as a code owner November 14, 2023 17:01

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 Nov 14, 2023

aramikm reviewed Nov 14, 2023

View reviewed changes

demisx pushed a commit that referenced this pull request Nov 14, 2023

Update weights for PR #1763

1df7c3f

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

shannonwells pushed a commit that referenced this pull request Nov 15, 2023

Update weights for PR #1763

bda5834

shannonwells force-pushed the feat/separate-unlock-chunks-#1753 branch from 1df7c3f to a6f250e Compare November 15, 2023 01:35

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 Nov 15, 2023

aramikm approved these changes Nov 15, 2023

View reviewed changes

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 Nov 15, 2023

shannonwells mentioned this pull request Nov 28, 2023

adding schema names support #1784

Merged

5 tasks

shannonwells added 2 commits November 30, 2023 13:10

respond to PR review comments

2324c2a

Run benchmarks [run-benchmarks capacity]

495f095

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

shannonwells added 2 commits November 30, 2023 13:27

add a bit about how the current version works

9b1f0a7

fix comments

3ccc599

Update weights for PR #1763

2b6f740

enddynayn reviewed Nov 30, 2023

View reviewed changes

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

enddynayn reviewed Nov 30, 2023

View reviewed changes

pallets/capacity/src/lib.rs Outdated Show resolved Hide resolved

enddynayn reviewed Nov 30, 2023

View reviewed changes

remove try_mutate, fix set_staking_account

f0675d1

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

shannonwells enabled auto-merge (squash) December 1, 2023 00:03

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

wilwade requested review from saraswatpuneet and enddynayn December 1, 2023 00:40

enddynayn reviewed Dec 1, 2023

View reviewed changes

enddynayn approved these changes Dec 1, 2023

View reviewed changes

shannonwells merged commit afa4490 into main Dec 1, 2023
30 checks passed

shannonwells deleted the feat/separate-unlock-chunks-#1753 branch December 1, 2023 01:28

shannonwells mentioned this pull request Apr 19, 2024

StorageMigration for StakingTargetDetails #1729

Closed

shannonwells added the Provider Boosting label Jul 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/separate unlock chunks #1753 #1763

Feat/separate unlock chunks #1753 #1763

shannonwells commented Nov 7, 2023 •

edited

Loading

codecov bot commented Nov 7, 2023 •

edited

Loading

shannonwells Nov 14, 2023

shannonwells Nov 14, 2023

github-actions bot commented Nov 14, 2023

aramikm left a comment

github-actions bot commented Nov 14, 2023

aramikm left a comment

github-actions bot commented Nov 15, 2023

github-actions bot commented Nov 30, 2023

github-actions bot commented Nov 30, 2023

github-actions bot commented Nov 30, 2023

enddynayn Nov 30, 2023

enddynayn Nov 30, 2023 •

edited

Loading

shannonwells Nov 30, 2023

enddynayn Dec 1, 2023 •

edited

Loading

shannonwells Dec 1, 2023

enddynayn Nov 30, 2023 •

edited

Loading

shannonwells Nov 30, 2023 •

edited

Loading

enddynayn Dec 1, 2023 •

edited

Loading

shannonwells Dec 1, 2023 •

edited

Loading

enddynayn Nov 30, 2023

enddynayn Nov 30, 2023

enddynayn Dec 1, 2023 •

edited

Loading

enddynayn Dec 1, 2023

enddynayn left a comment

Feat/separate unlock chunks #1753 #1763

Feat/separate unlock chunks #1753 #1763

Conversation

shannonwells commented Nov 7, 2023 • edited Loading

Goal

Discussion

To Verify

Checklist

codecov bot commented Nov 7, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 14, 2023

aramikm left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 14, 2023

aramikm left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 15, 2023

github-actions bot commented Nov 30, 2023

github-actions bot commented Nov 30, 2023

github-actions bot commented Nov 30, 2023

Choose a reason for hiding this comment

enddynayn Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enddynayn Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enddynayn Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

shannonwells Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

enddynayn Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

shannonwells Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enddynayn Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enddynayn left a comment

Choose a reason for hiding this comment

shannonwells commented Nov 7, 2023 •

edited

Loading

codecov bot commented Nov 7, 2023 •

edited

Loading

enddynayn Nov 30, 2023 •

edited

Loading

enddynayn Dec 1, 2023 •

edited

Loading

enddynayn Nov 30, 2023 •

edited

Loading

shannonwells Nov 30, 2023 •

edited

Loading

enddynayn Dec 1, 2023 •

edited

Loading

shannonwells Dec 1, 2023 •

edited

Loading

enddynayn Dec 1, 2023 •

edited

Loading