Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Tvl pool staking #14775

Draft
wants to merge 31 commits into
base: master
Choose a base branch
from
Draft

Tvl pool staking #14775

wants to merge 31 commits into from

Conversation

PieWol
Copy link

@PieWol PieWol commented Aug 15, 2023

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context, including:

paritytech/polkadot-sdk#155

  • What does this PR do?
  • This PR introduces the TotalValueLocked of all nom-pools
  • Why are these changes needed?
  • they are requested in issue 14566
  • How were these changes implemented and what do they affect?
  • Its just an extra StorageValue with an effect on execution time on every flow of funds in and out of the nom-pools.

Use Github semantic linking to address any open issues this PR relates to or closes.

Closes paritytech/polkadot-sdk#155

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Aug 15, 2023

User @PieWol, please sign the CLA here.

frame/nomination-pools/src/lib.rs Outdated Show resolved Hide resolved
frame/nomination-pools/src/lib.rs Outdated Show resolved Hide resolved
frame/nomination-pools/src/lib.rs Outdated Show resolved Hide resolved
frame/nomination-pools/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 3194 to 3195
expected_tvl += T::Staking::total_stake(&bonded_pool.bonded_account())
.expect("all pools must have total stake");
Copy link
Member

Choose a reason for hiding this comment

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

Rather than spread the try_state logic for expected TVL across multiple areas of the try-state hook, could you please move it all together?

It's fine to iterate over pools again for this purpose.

Copy link
Author

Choose a reason for hiding this comment

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

I put the TVL checks into one place now but the check for TVL integrity is now done via the PoolMembers instead of all BondingPools. I would love some feedback on the way it's done now.

frame/nomination-pools/src/migration.rs Outdated Show resolved Hide resolved
frame/nomination-pools/src/migration.rs Outdated Show resolved Hide resolved
frame/nomination-pools/src/migration.rs Outdated Show resolved Hide resolved
@liamaharon liamaharon added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. T1-runtime This PR/Issue is related to the topic “runtime”. labels Aug 17, 2023
@@ -724,4 +724,99 @@ pub mod v5 {
Ok(())
}
}

/// This migration accumulates and initializes the [`TotalValueLocked`] for all pools.
pub struct VersionUncheckedMigrateV5ToV6<T>(sp_std::marker::PhantomData<T>);
Copy link
Member

Choose a reason for hiding this comment

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

I hope nobody is us using nomination pools on parachains, but we should mention in the migration that it is using lots of weight and may not be suitable for parachains.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for pointing it out. How do you think this mention should be done? Just a comment in the code?

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3424542

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store nomination pool total value locked on-chain
5 participants