Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Nomination Pool Commission #13128
Nomination Pool Commission #13128
Conversation
This was referenced
rossbulat
added
B3-apinoteworthy
kianenigma
added
B7-runtimenoteworthy
| // Test for `max_increase` throttling. | ||
| // | ||
| // Throttled if the attempted increase in commission is greater than `max_increase`. | ||
| if (*to).saturating_sub(commission_as_percent) > t.max_increase { |
There was a problem hiding this comment.
Check against global max as well?
There was a problem hiding this comment.
No need, the commission is bounded to global max on 754. The "true" commission is determined here in commission_and_payee() as a payout is being made.
There was a problem hiding this comment.
Seeing the work on runtime APIs by @gpestana , maybe we can introduce another one to get the "true" commission capped at the global max.
There was a problem hiding this comment.
I think that runtime API would be useful yes. Happy to implement it once we merge this PR, or even include the runtime API in #13119 returning always 0 until this PR is merged. If PR#13119 is merged before this one (which I think it will be the case), you could merge and implement the logic in this PR.
The call_state method could be something along the lines of NominationPoolsApi_pool_comission(pool_id) -> Perbill
| // Test for `max_increase` throttling. | ||
| // | ||
| // Throttled if the attempted increase in commission is greater than `max_increase`. | ||
| if (*to).saturating_sub(commission_as_percent) > t.max_increase { |
There was a problem hiding this comment.
I think that runtime API would be useful yes. Happy to implement it once we merge this PR, or even include the runtime API in #13119 returning always 0 until this PR is merged. If PR#13119 is merged before this one (which I think it will be the case), you could merge and implement the logic in this PR.
The call_state method could be something along the lines of NominationPoolsApi_pool_comission(pool_id) -> Perbill
| /// | ||
| /// If `current.0` is larger than the updated max commission value, `current.0` will also be | ||
| /// updated to the new maximum. This will also register a `throttle_from` update. | ||
| fn try_update_max(&mut self, new_max: Perbill) -> DispatchResult { |
There was a problem hiding this comment.
nit but I wonder if it would be cleaner to return a pallet error here instead of a DispatchError and then transform to dispatch error in the callable.
(this suggestion is valid for all the other method impls that return a dispatch error)
There was a problem hiding this comment.
I guess either way would work, I noticed in the staking pallet DispatchError is also used in "do" functions outside the callable function.
If this convention is worthwhile perhaps a separate issue that covers all the staking-related pallets might be good?
There was a problem hiding this comment.
I think returning DispatchError in functions that have #[pallet::call] and pallet error otherwise is slightly better.
Co-authored-by: Gonçalo Pestana <g6pestana@gmail.com>
Co-authored-by: Gonçalo Pestana <g6pestana@gmail.com>
| .unwrap_or(Perbill::zero()); | ||
| assert_eq!(commission_as_percent, Perbill::from_percent(50)); | ||
|
|
||
| // update commission only. |
There was a problem hiding this comment.
I would expect to see a payout after each variation here in order to fully test this.
| Some((Perbill::from_percent(10), 901)) | ||
| )); | ||
|
|
||
| assert_eq!( |
There was a problem hiding this comment.
might be better to call this after each test or bundle of related tests.
| // Run to block 3 | ||
| run_blocks(2); | ||
|
|
||
| // Now try to increase commission by 1% and provide an initial payee. This should |
There was a problem hiding this comment.
we can always change the payee regardless of max and throttle, right? basically, we if the commission value is not changed, all other checks can be bypassed.
There was a problem hiding this comment.
Throttling yes - it will always allow a payee update if the current commission is provided along with it. throttling() returns false early on in the function if the commission is unchanged.
It would however fail if a new commission higher than max (or subject to being throttled) is provided along with a new payee.
| Error::<Runtime>::CommissionExceedsMaximum | ||
| ); | ||
|
|
||
| assert_eq!( |
There was a problem hiding this comment.
ditto, I think checking events at the end of the tests is reasonable when the tests is by all means a "unti" tests, testing one scenario. Your tests tend to me much longer, and at least I find it hard to connect the events with everything that happened in the test.
| pool_id: 1, | ||
| current: Some((Perbill::from_percent(75), 900)), | ||
| }, | ||
| Event::PoolMaxCommissionUpdated { |
There was a problem hiding this comment.
So now, I kinda see that we are missing a PoolCommissionUpdated to 50?
There was a problem hiding this comment.
An event for that was left out as UIs could determine there was a commission change just be listening to the max commission changed event (then compare with current commission in the UI). But yes I can see now there is an argument for including it here too.
There was a problem hiding this comment.
No major issues found, perhaps https://github.com/paritytech/substrate/pull/13128/files?diff=unified&w=0#r1070602032 is the only one.
Some suggestions about docs:
- try and avoid writing duplicate facts. For example, the fact that the commission can be changed by root should ideally be mentioned in the description of root role, and perhaps the top level
Commissionstruct. I don't have better examples here, but the ideas is that it is almost always better to say: "Read X to see who can change the commission" rather than saying it on the spot. - We should update the top level crate doc with all the detailed information that pool operators want to know about the commission as well so that it can act as a source of truth.
|
This pull request has been mentioned on Polkadot Forum. There might be relevant details there: https://forum.polkadot.network/t/the-future-of-polkadot-staking/1848/1 |
rossbulat
requested review from
shawntabrizi,
athei,
acatangiu,
sorpaas,
andresilva,
koute and
a team
as code owners
rossbulat
changed the title
rossbulat
force-pushed
the
rb-nomination-pool-commission-2
branch
from
9ee6fcf
to
acad3e4
Compare
|
The CI pipeline was cancelled due to failure one of the required jobs. |
This PR adds commission functionality to Nomination Pools (was #12511).
New structs:
CommissionandCommissionChangeRate.New storage item:
GlobalMaxCommission: Option<Perbill>.New calls:
set_commission,set_max_commission, andset_commission_change_rate.polkadot companion: paritytech/polkadot#6264
fixes #11672.