-
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
Feat/dyn tier thresholds #1306
Feat/dyn tier thresholds #1306
Conversation
/runtime-upgrade-test runtime=shibuya |
Runtime upgrade test is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/10108041768. |
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.
Nice, well done!
Did a quick review and left some comments.
Will do a more detailed review tomorrow.
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.
2 more comments, otherwise LGTM.
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.
Did a thorough review now - just a few small comments. 🙈
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.
LGTM
@@ -1258,6 +1258,7 @@ pub type Executive = frame_executive::Executive< | |||
>; | |||
|
|||
parameter_types! { | |||
// percentages below are calulated based on total issuance at the time when dApp staking v3 was launched (8.4B) |
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.
Might be worth considering adding more checks for this into the try-runtime check 🙂
For another PR though.
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.
Are you thinking of sanity checks in pre_upgrade
, like:
#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
let tier_thresholds: Result<BoundedVec<TierThreshold, T::NumberOfTiers>, _> =
BoundedVec::try_from(TierThresholds::get().to_vec());
assert!(tier_thresholds.is_ok());
...
}
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.
Both pre & post.
E.g. take current thresholds (active ones) in pre-upgrade, send them to post upgrade.
In post-upgrade, after you derive the new thresholds, ensure they are e.g. within 10% of the old ones.
Minimum allowed line rate is |
@ermalkaleci any more comments here? |
I'm reviewing now. The only concern is to double check percentage |
Pull Request Summary
New tier thresholds as percentage of the total issuance. Should be merged after v5.42.2 is deployed to Astar/Shiden.
Closes #1295
Types: TierThreshold enum updated
For
TierThreshold
enum: previous DynamicTvlAmount & FixedTvlAmount variants were deleted and new ones are added instead: DynamicPercentage & FixedPercentage. They support Perbill percentages and are intended to be used in the same way as the previous ones for TVL amounts. Genesis configurations, mocks and tests were updated accordingly.Migrations
Two migrations are introduced:
number_of_slots
is removed andtier_thresholds
is translated from BoundedVec<TierThreshold, NT> to BoundedVec<Balance, NT>.tier_thresholds
is updated for each runtime with calculated percentage based on the the total issuance when dApp staking v3 was launched.try-runtime
resultsshibuya
Check list