-
Notifications
You must be signed in to change notification settings - Fork 381
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
dApp staking v3 - part 3 #1036
dApp staking v3 - part 3 #1036
Conversation
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 good!
minor nitpicks and some questions.
Just skimmed over the tests but haven't looked into them properly, will do that in final PR.
} | ||
} | ||
|
||
const STAKING_SERIES_HISTORY: u32 = 3; |
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.
From the technical doc, I'm assuming this'll be configurable therefore used as generic parameter below.
If not, then it should be hardcoded IMO.
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.
It won't be configurable, it will be hardcoded like this.
I do mention in the technical docs it will be exactly 3 since we don't need more.
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 see, I got confused over many other history depth
.
If we need to cover this for only three elements (and three scenarios), then why use BoundedVec
?
I mean, can't we simply use three struct variables like s0, s1, s2
?
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.
Let me answer with a thought (or code?) experiment - how would you implement stake
or unstake
using 3 separate attributes? How would that code look like?
If I'm not mistaken, it would be if within if within if, etc.
pallets/dapp-staking-v3/src/types.rs
Outdated
/// Used to remove past entries, in case vector is full. | ||
fn prune(&mut self) { | ||
// Prune the oldest entry if we have more than the limit | ||
if self.0.len() > STAKING_SERIES_HISTORY.saturating_sub(1) as usize { |
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.
Prune the oldest entry if we have more than the limit
Can you explain why we have to subtract one from bound value here?
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.
Good questions, and good catch!
It made sense with stake
, but it doesn't anymore with unstake
.
Not sure why I used sub 1
instead of just comparing to the max allowed size, it's changed now.
I made a test to cover this (it breaks on the current code), and fixed the issue.
Minimum allowed line rate is |
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.
Great work and test coverage Just some small comments.
Also I know you have given a lot of context around every function but still some more would help.
I was not able to go through majority of test, I will do it in the next PR.
// TODO: this can be perhaps optimized so we prune entries which are very old. | ||
// However, this makes the code more complex & more error prone. | ||
// If kept like this, we always make sure we cover the history, and we never exceed it. |
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.
The vec
would has at-most 3 entries. so I'm not sure how effective this optimisation would be.
imo, we should perhaps revisit this after benchmarking and there is a serious regression.
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.
The only possible optimization would be reduced PoV (but entry is quite small anyhow).
The "drawback" of the current solution is that very old entries might need some time to get cleaned up. In reality, for active contracts this won't happen. And even if it does, the effect is minuscule and extremely situational.
E.g. image a contract being staked on in era 5
, then again staked on in era 6
, and once again in era 20
.
Realistically, we only need information for the last 3 eras, but using the current logic we'll keep all values inside the Series
. Optimized solution would be to recognize that latest is 20
, and that we only need values up to 18
. Vector size would be reduced to 2, instead of 3.
.map_err(|err| match err { | ||
AccountLedgerError::InvalidPeriod => { | ||
Error::<T>::UnclaimedRewardsFromPastPeriods | ||
} | ||
AccountLedgerError::UnavailableStakeFunds => Error::<T>::UnavailableStakeFunds, | ||
AccountLedgerError::NoCapacity => Error::<T>::TooManyStakeChunks, | ||
// Defensive check, should never happen | ||
_ => Error::<T>::InternalStakeError, | ||
})?; |
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 we expect to have conversion b/wAccountLedgerError
and DispatchError, then IMO we should implement From
trait over the type.
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 did think of that, but for different AccountLedgerError
values, we get different pallet errors, depending on the context (stake
or unstake
functions).
// TODO: not sure why the mess with type happens here, I can check it later | ||
let era_length: BlockNumber = | ||
<<Test as pallet_dapp_staking::Config>::StandardEraLength as sp_core::Get<_>>::get(); | ||
let voting_period_length_in_eras: EraNumber = | ||
<<Test as pallet_dapp_staking::Config>::StandardErasPerVotingPeriod as sp_core::Get<_>>::get( | ||
); |
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.
It would be much easier to simply extract the const into separate parameter types
parameter_types! {
pub const StandardEraLength: u64 = 10;
pub const StandardErasPerVotingPeriod: u32 = 8;
}
and then use it as
let era_length: BlockNumber = StandardEraLength::get();
let voting_period_length_in_eras: EraNumber = StandardErasPerVotingPeriod::get();
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 could do that but honestly I prefer fetching them from pallet. Might be a bit uglier but I know 100% that I'm using the same thing pallet is using.
The comment I made here refers to compiler forcing me to select which Get<_>
implementation to use (hence the sp_core::Get<_>
notation).
/// Advance blocks until next period type has been reached. | ||
pub(crate) fn _advance_to_next_period_type() { | ||
let period_type = ActiveProtocolState::<Test>::get().period_type(); | ||
while ActiveProtocolState::<Test>::get().period_type() == period_type { | ||
run_for_blocks(1); | ||
} | ||
} |
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.
Why do we need this pub function here? I didn't see any references for it.
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 used it for one test, but then changed it later. If I don't use it by when I'm done implementing everything, I'll remove it.
smart_contract: &MockSmartContract, | ||
amount: Balance, | ||
) { | ||
// TODO: this is a huge function - I could break it down, but I'm not sure it will help with readability. |
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.
Kindly do that, It's quite hard to follow 😆
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 see about doing that when finalizing PR. The ugly thing is that I'll end up with functions taking lots of parameters...
#[derive(Encode, Decode, MaxEncodedLen, Clone, Copy, Debug, PartialEq, Eq, TypeInfo)] | ||
pub struct PeriodInfo { | ||
#[codec(compact)] | ||
pub number: PeriodNumber, |
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.
Docstring to explain the role of period number would help here. For me it's not very clear right now.
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 can add that, sure. But I'm not sure how can I explain it?
It's the period number; number of the ongoing period.
Perhaps the broader context is missing when only looking at this struct.
Later I will add a more detailed explanation of the functionality into the PR.
For now, I'd suggest to refer to the forum post and/or design docs.
If you have a good suggestions, I'll add that.
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.
@gitofdeepanshu thanks for the comments!
.map_err(|err| match err { | ||
AccountLedgerError::InvalidPeriod => { | ||
Error::<T>::UnclaimedRewardsFromPastPeriods | ||
} | ||
AccountLedgerError::UnavailableStakeFunds => Error::<T>::UnavailableStakeFunds, | ||
AccountLedgerError::NoCapacity => Error::<T>::TooManyStakeChunks, | ||
// Defensive check, should never happen | ||
_ => Error::<T>::InternalStakeError, | ||
})?; |
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 did think of that, but for different AccountLedgerError
values, we get different pallet errors, depending on the context (stake
or unstake
functions).
// TODO: not sure why the mess with type happens here, I can check it later | ||
let era_length: BlockNumber = | ||
<<Test as pallet_dapp_staking::Config>::StandardEraLength as sp_core::Get<_>>::get(); | ||
let voting_period_length_in_eras: EraNumber = | ||
<<Test as pallet_dapp_staking::Config>::StandardErasPerVotingPeriod as sp_core::Get<_>>::get( | ||
); |
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 could do that but honestly I prefer fetching them from pallet. Might be a bit uglier but I know 100% that I'm using the same thing pallet is using.
The comment I made here refers to compiler forcing me to select which Get<_>
implementation to use (hence the sp_core::Get<_>
notation).
/// Advance blocks until next period type has been reached. | ||
pub(crate) fn _advance_to_next_period_type() { | ||
let period_type = ActiveProtocolState::<Test>::get().period_type(); | ||
while ActiveProtocolState::<Test>::get().period_type() == period_type { | ||
run_for_blocks(1); | ||
} | ||
} |
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 used it for one test, but then changed it later. If I don't use it by when I'm done implementing everything, I'll remove it.
smart_contract: &MockSmartContract, | ||
amount: Balance, | ||
) { | ||
// TODO: this is a huge function - I could break it down, but I'm not sure it will help with readability. |
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 see about doing that when finalizing PR. The ugly thing is that I'll end up with functions taking lots of parameters...
#[derive(Encode, Decode, MaxEncodedLen, Clone, Copy, Debug, PartialEq, Eq, TypeInfo)] | ||
pub struct PeriodInfo { | ||
#[codec(compact)] | ||
pub number: PeriodNumber, |
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 can add that, sure. But I'm not sure how can I explain it?
It's the period number; number of the ongoing period.
Perhaps the broader context is missing when only looking at this struct.
Later I will add a more detailed explanation of the functionality into the PR.
For now, I'd suggest to refer to the forum post and/or design docs.
If you have a good suggestions, I'll add that.
} | ||
} | ||
|
||
const STAKING_SERIES_HISTORY: u32 = 3; |
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.
Let me answer with a thought (or code?) experiment - how would you implement stake
or unstake
using 3 separate attributes? How would that code look like?
If I'm not mistaken, it would be if within if within if, etc.
// TODO: this can be perhaps optimized so we prune entries which are very old. | ||
// However, this makes the code more complex & more error prone. | ||
// If kept like this, we always make sure we cover the history, and we never exceed it. |
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.
The only possible optimization would be reduced PoV (but entry is quite small anyhow).
The "drawback" of the current solution is that very old entries might need some time to get cleaned up. In reality, for active contracts this won't happen. And even if it does, the effect is minuscule and extremely situational.
E.g. image a contract being staked on in era 5
, then again staked on in era 6
, and once again in era 20
.
Realistically, we only need information for the last 3 eras, but using the current logic we'll keep all values inside the Series
. Optimized solution would be to recognize that latest is 20
, and that we only need values up to 18
. Vector size would be reduced to 2, instead of 3.
* dApp Staking v3 Part1 * Feat/dapp staking v3 phase2 (#991) * Phase2 progress * Adapt for 0.9.43 * Abstract sparse vector type * Further modifications * claim unlocked functionality WIP * claim unlocked tested * Relock unlocking * Check era when adding chunk * Custom error for some types * Additional type tests - WIP * More type tests * Review comments * Additional changes * dApp staking v3 - part 3 (#1036) * dApp staking v3 - PR3 * SingularStakingInfo * SingularStakingInfo tests * Stake work * Stake & lots of tests * TODOs, renaming, improvements * Refactoring & cleanup * More fixes * Rework series * Series tests * Minor adjustment * stake test utils & first test * Stake test * stake extrinsic tests * Era & Period change logic * Update era/period transition in mock & tests * on_init tests & refactoring * Unstake & some minor improvements * Lots of type tests for unstake * More tests * More types test * Testing utils for unstake, some TODOs * Unstake tests * Minor adjustments * Fixes * Additional scenario * Address review comments * Correct unstake amount * Tests * dapp staking v3 - part 4 (#1053) * EraRewardSpan * Initial version of claim_staker_reward * Tests * Test utils for claim-staker * Bug fixes, improvements * Claim improvements & some tests * Refactoring in progress * Refactoring continued * Refactoring progress * Refactoring finished * Bonus rewards * Docs & some minor changes * Comments, tests, improved coverage * Tier params & config init solution * Tier reward calculation WIP * Tier assignemnt * Minor cleanup * Claim dapp rewards * Claim dapp reward tests * unstake from unregistered call * Extra traits * fixes * Extra calls * Refactoring * More refactoring, improvements, TODO solving * Local integration * Genesis config * Add forcing call * try runtime build fix * Minor changes * Minor * Formatting * Benchmarks INIT * Compiling benchmarks * Fix * dapp tier calculation benchmark * Measured tier assignment * Decending rewards in benchmarks * Series refactoring & partial tests * Comments, minor changes * Tests, improvements * More tests, some minor refactoring * Formatting * More benchmarks & experiments, refactoring * Readme, docs * Minor renaming, docs * More docs * More docs * Minor addition * Review comment fixes & changes * Minor change * Review comments * Update frontier to make CI pass * Fix for cleanup * dapp staking v3 - part 5 (#1087) * EraRewardSpan * Initial version of claim_staker_reward * Tests * Test utils for claim-staker * Bug fixes, improvements * Claim improvements & some tests * Refactoring in progress * Refactoring continued * Refactoring progress * Refactoring finished * Bonus rewards * Docs & some minor changes * Comments, tests, improved coverage * Tier params & config init solution * Tier reward calculation WIP * Tier assignemnt * Minor cleanup * Claim dapp rewards * Claim dapp reward tests * unstake from unregistered call * Extra traits * fixes * Extra calls * Refactoring * More refactoring, improvements, TODO solving * Local integration * Genesis config * Add forcing call * try runtime build fix * Minor changes * Minor * Formatting * Benchmarks INIT * Compiling benchmarks * Fix * dapp tier calculation benchmark * Measured tier assignment * Decending rewards in benchmarks * Series refactoring & partial tests * Comments, minor changes * Tests, improvements * More tests, some minor refactoring * Formatting * More benchmarks & experiments, refactoring * Readme, docs * Minor renaming, docs * More docs * More docs * Minor addition * Review comment fixes & changes * Minor change * Review comments * Update frontier to make CI pass * dApp staking v3 - part 5 * Make check work * Limit map size to improve benchmarks * Fix for cleanup * Minor fixes, more tests * More fixes & test * Minor changes * More tests * More tests, some clippy fixes * Finished with type tests for now * Log, remove some TODOs * Changes * Bug fix for unstake era, more tests * Formatting, extra test * Unregistered unstake, expired cleanup, test utils & tests * Cleanup tests, minor fixes * force tests * Remove TODOs * Tier assignment test WIP * Finish dapp tier assignment test, fix reward calculation bug * Some renaming, test utils for era change WIP * Fix minor issues, propagaste renaming * More checks * Finish on_init tests * Comments, docs, some benchmarks * Benchmark progress * More benchmarks * Even more benchmarks * All extrinsics benchmarked * Expand tests * Comment * Missed error test * Review comments * Pallet inflation (#1091) * Pallet inflation * Hooks * Functionality, mock & benchmarks * Empty commit since it seems last push didn't work properly * Tests, fixes * More tests, minor fixes&changes * Zero divison protection, frontier update * More tests, minor changes * Integration & renaming * Genesis integration, more tests * Final integration * Cleanup deps * Division with zero test * Comments * More minor changes * Improve test coverage * Integrate into pallet-timestamp * Remove timestamp * Fix * review comments * toml format * dApp Staking v3 - part 6 (#1093) * dApp staking v3 part 6 * Minor refactor of benchmarks * Weights integration * Fix * remove orig file * Minor refactoring, more benchmark code * Extract on_init logic * Some renaming * More benchmarks * Full benchmarks integration * Testing primitives * staking primitives * dev fix * Integration part1 * Integration part2 * Reward payout integration * Replace lock functionality with freeze * Cleanup TODOs * More negative tests * Frozen balance test * Zero div * Docs for inflation * Rename is_active & add some more docs * More docs * pallet docs * text * scripts * More tests * Test, docs * Review comment * dApp staking v3 - part 7 (#1099) * dApp staking v3 part 6 * Minor refactor of benchmarks * Weights integration * Fix * remove orig file * Minor refactoring, more benchmark code * Extract on_init logic * Some renaming * More benchmarks * Full benchmarks integration * Testing primitives * staking primitives * dev fix * Integration part1 * Integration part2 * Reward payout integration * Replace lock functionality with freeze * Cleanup TODOs * More negative tests * Frozen balance test * Zero div * Docs for inflation * Rename is_active & add some more docs * More docs * pallet docs * text * scripts * More tests * Test, docs * Review comment * Runtime API * Changes * Change dep * Comment * Formatting * Expired entry cleanup * Cleanup logic test * Remove tier labels * fix * Improve README * Improved cleanup * Review comments * Docs * Formatting * Typo * Fix - bonus reward update ledger (#1102) * Bonus claim reduced contract_stake_count * Extra test * dApp Staking Migration (#1101) * dApp Staking Migration * Events, benchmark prep * benchmarks * Benchmarks & mock * weights * limit * Use extrinsic call * Solved todos, docs, improvements * Maintenance mode, on_runtime_upgrade logic * Tests, refactoring * Finish * More tests * Type cleanup * try-runtime checks * deps * Improve docs * Fixes & try-runtime testing modifications * Fix test * repeated * Minor improvements * Improvements * taplo * Minor rename * Feat/dapp staking v3 precompiles (#1096) * Init commit * All legacy calls covered * v2 interface first definition * Rename * Resolve merge errors * TODO * v2 init implementation * Prepared mock * todo * Migration to v2 utils * More adjustments * Finish adapting implementation to v2 * Mock & fixes * Primitive smart contract * Fix dsv3 test * Prepare impl & mock * Remove redundant code, adjust mock & prepare tests * Tests & utils * Test for legacy getters/view functions * More legacy tests * v1 interface covered with tests * Minor refactor, organization, improvements * v2 tests * Cleanup TODOs * More tests * Updates * docs * Fixes * Address review comments * Adjustments * Audit comments * Fix mock * FMT * Review comments * Fix for incorrect freeze amount (#1111) * dApp Staking v3 - Shibuya integration (#1109) * dApp staking v3 - Shibuya integration * Init * Fix build * Fix find/replace doc mess * Migration & disable * More integration * Progress * Adjusted integration * Finished integration * Additional modifications & cleanup * Move comment * Fixes * Shibuya integration tests fix & proxy * Renaming * Integration test fixes & legacy support * Adjust for benchmarks * Remove chain-extension, small updates * fixes * Partial weights * Minor changes * Benchmark fixes * dApp staking weights * Weights, deps * Remove redundant storage item * Inflation params, resolve TODOs * Optimize lengthy benchmark * Integration test * Sort out more TODOs * Benchmark optimization * Fix seed * Remove spec version bump * Fix integration test * Weights update
Pull Request Summary
This is the 3rd part of the
dApp staking v3
implementation.Major Features
stake
- covers staking locked funds on smart contractsunstake
- covers unstaking staked funds from smart contractson_initialize
- coversera
andperiod
transitions (in the future might be replaced byScheduler
)PR Overview
Approach
Given the functionality's complexity, the approach is taken to abstract as much of the lower level
functionality into
structs
andenums
.Removals
LockedChunk
since we decided to only reward stakers, not lockersAdditions
ProtocolState
(holds general info about dApp staking) received various member functions/methodsAccountLedger
expanded with support for staking & unstakingStakeAmount
enum is intended to encapsulate stake amount - will be more integrated in the futureSingularStakingInfo
struct is used to cover information about particular stakers stake on a specific smart contractContractStakingInfo
struct is used to cover information about total stake on contract in a particular era/periodContractStakingInfoSeries
is a composite struct used to cover multiple era/period combinationsChanges
PeriodType
has been split into the enum of same name and structPeriodInfo
to cover more attributesTests