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

nomination-pools: add optional commission #11672

Closed
kianenigma opened this issue Jun 15, 2022 · 8 comments · Fixed by #13128
Closed

nomination-pools: add optional commission #11672

kianenigma opened this issue Jun 15, 2022 · 8 comments · Fixed by #13128
Assignees
Labels
U4-some_day_maybe Issue might be worth doing eventually. Z2-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase.

Comments

@kianenigma
Copy link
Contributor

Currently, we don't have commission, and we prefer to keep the code simple and ship without this. Nonetheless, it is reasonable to add an optional commission for pool operators.

@kianenigma kianenigma added U4-some_day_maybe Issue might be worth doing eventually. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be. Z2-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. labels Jun 15, 2022
@Szegoo
Copy link
Contributor

Szegoo commented Jun 26, 2022

I have a couple of questions about this issue.

If I understand this correctly by pool operators you mean all the pool roles inside the PoolRoles struct?

  • Do all of the administrative roles get the commission? Maybe we could make choosing who receives a commission configurable(e.g setting that only the nominator gets the commission). Also, do all of them get the same share of the commission? That should probably also be configurable.
  • I assume that the commission is taken from the earned rewards.

@kianenigma
Copy link
Contributor Author

Do all of the administrative roles get the commission? Maybe we could make choosing who receives a commission configurable(e.g setting that only the nominator gets the commission). Also, do all of them get the same share of the commission? That should probably also be configurable.

Yeah it can probably be configurable.

I assume that the commission is taken from the earned rewards.

Correct.


Note that I would not start working on this yet, as Pools are not launched yet in Polkadot and it is more reasonable to keep them simple at the beginning.

@Szegoo
Copy link
Contributor

Szegoo commented Jun 29, 2022

Ok, I will start working on this when the pools are launched in Polkadot.

@kianenigma kianenigma removed the Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be. label Aug 16, 2022
@kianenigma
Copy link
Contributor Author

kianenigma commented Aug 16, 2022

Some notes, I propose we re-think the commission and not make it be a "free forever commission" system like staking. Instead, each pool member can set two commission values:

struct Commission {
  current: Percent
  max_ever: Option<Percent>,
} 

impl Default for Commission {
  fn default() -> Self {
    current: Percent::zero(),
    max_ever: None,
  }
}

with the following rules:

  1. current can NEVER be set to a value more than max_ever.
  2. max_ever can never be increased.

@kianenigma
Copy link
Contributor Author

A rate limit on the maximum change is also a good idea.

@Szegoo
Copy link
Contributor

Szegoo commented Sep 23, 2022

Instead, each pool member can set two commission values:

Shouldn't these two values be set by the pool operators?

@kianenigma
Copy link
Contributor Author

Instead, each pool member can set two commission values:

Shouldn't these two values be set by the pool operators?

indeed, I meant to say that.

@kianenigma
Copy link
Contributor Author

kianenigma commented Oct 13, 2022

Having a rate change as well, this is the commission struct that has to be stored/updated per-pool (new field of BondedPools):

struct Commission {
  // current commission percentage. 
  current: Perbill,
  // If set, the commission can never go above this. 
  // 
  // If set, this value can never be updated to a higher amount. The only way to change this is to fully dismantle the pool. 
  max_ever: Option<Perbill>,
  // If set, a change is allowed if `CommissionChange::update` returns `true`.
  max_change: Option<CommissionChange>
 
} 

struct CommissionChange {
  // pool operator sets this only. Expresses: `Perbill` allowed to change every `BlockNumber` blocks. 
  pub change_rate: (Perbill, BlockNumber),
  
  // these are updated by the chain.
  previous: Perbill,
  pervious_set_at: BlockNumber
}

impl CommissionChange {
  // return true if the proposed change is allowed. 
  fn update(&mut self, new: Perbill) -> bool {
    let now = system::block_number();
    let duration = now = self.pervious_set_at;
    let allowed_change = self.change_rate.0 * (self.change_rate.1 / duration);
    let proposed_change = new - previous;
    
    if proposed_change <= allowed_change {
      self.previous = new;
      self.pervious_set_at = now;
      true
    } else {
      false 
    } 
  } 
} 

impl Commission {
  fn update(&mut self, new: Perbill) {
    if self.max_ever.map_or(false, |m| new > m) { // err } 
    if self.max_change.map_or(false, |c| !c.update(new)) { // err } 
    // change is okay -- allowed.
    self.current = new; 
  }

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
U4-some_day_maybe Issue might be worth doing eventually. Z2-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants