-
Notifications
You must be signed in to change notification settings - Fork 361
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
stakingv3: rank dapps in-between tiers #1240
Conversation
/bench shibuya pallet-dapp-staking-v3 |
Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/9051969615. |
Benchmark job failed. |
/bench shibuya-dev pallet-dapp-staking-v3 |
Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/9052059142. |
Benchmarks have been finished. |
/bench shibuya-dev pallet-dapp-staking-v3 |
Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/9052156819. |
Benchmarks have been finished. |
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.
Sorry for the delay!
Left some comments we can discuss, didn't check tests yet.
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.
Went over the tests this time as well, so this is full review from me.
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!
I guess benchmarks still need to be run.
It'd be good to get another review from @ashutoshvarma or @PierreOssun .
@ermalkaleci could you please open up an issue here, and explain how the existing struct has changed, how to know which struct to use (storage version), and how to calculate the new reward? |
/bench astar-dev,shibuya-dev,shiden-dev pallet_dapp_staking_v3 |
Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/9387947006. |
Benchmarks have been finished. |
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.
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.
LGTM
@@ -289,6 +296,7 @@ pub mod pallet { | |||
beneficiary: T::AccountId, | |||
smart_contract: T::SmartContract, | |||
tier_id: TierId, | |||
rank: Rank, |
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.
Hi,
IMHO, it was better to add the new field at the end to be backwards compatible with existing clients listening to your events.
Changing the order of fields introduces a breaking change for events consumers.
Adding a new field at the end doesn't necessarily break the consumer: the new field is ignored but it still work.
Hoping this comment can be helpful for the future.
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.
You should be able to query by key. Any reason why you are querying by index?
In order to preserve
u8
for DAppTierRewards we can use each byte of u8 fortier
andrank
. This way there's no need for migration and it's backward compatible.Since u8 has 2 bytes
0xff
0b1111_1111
we can use the 1st byte fortier
and 2nd byte forrank
. Rank goes from0..10
(higher is better). Meaning if you just enter a tier your rank is0
and more staking will increase your ranking as you become close to next tier.Currently all values will be from
0..3
sorank
will be zero. Rank will take place once we deploy it.The simplest solution for reward distribution between ranks will be to define an additional reward per rank. Let’s say Tier 2 reward portion is 50K ASTR and slot capacity is 5 then reward per slot will be 10K ASTR, therefor rank reward will be
rank_reward = max(slot_reward, tier_remaining_reward) / 10 = 1K
. If you classify for rank 0 then you get 10K (slot reward), if you classify rank 3 you get10K + 3 * rank_reward = 13K
. All the reward to support ranked tier comes from tier reward portion itself. If tier has 5 slots with 3 occupied then reward of 2 slots will be used for ranking. e.i. slots occupied[rank_0, rank_3, rank_10]
then calculated reward will be[10k, 13K, 20K]
. In cases when there’s not enough remaining reward to satisfy full reward per rank then it will be adjust based on remaining reward, so rank reward will be less than 1K. If all slots are occupied then all dapps will make equal 10K each (slot reward).