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

Staking rate targeting and specific rewards. #2882

Merged
merged 43 commits into from
Jul 24, 2019
Merged

Conversation

thiolliere
Copy link
Contributor

@thiolliere thiolliere commented Jun 17, 2019

I_NPoS

The total payout is calculated using I_NPoS function defined in http://research.web3.foundation/en/latest/polkadot/Token%20Economics/#payment-details

I_NPoS is implemented using some a piecewise linear function. This piecewise linear function has been generated with a test and is tested not to differ from actual float implementation with an error of 0.2%

Staking api

  • The previous era reward accumulated at each new session is removed as reward is now computed from P_NPoS
  • a root call add_points_to_validator is added so ppl rewarded can call it from inherent mecanism.

TODO:

  • add inherent call to add_points_to_validator so ppl are actually rewarded:
    • In the branch of validity checking, we reward:

    • In the branch of block production, we reward:

      • the block producer for producing a (non-uncle) block in the relay chain,
      • the block producer for each reference to a previously unreferenced uncle, and
      • the producer of each referenced uncle block.
  • traitify timestamp instead of direct use, note that contract and aura are using it directly as well
  • timestamp trait: what api is to be used, returning 0 if timestamp is not yet inserted ?

related to #2781

@thiolliere thiolliere added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jun 17, 2019
@gavofyork
Copy link
Member

@AlistairStewart could you take a look (and/or anyone else that knows how this should work)

srml/staking/src/lib.rs Outdated Show resolved Hide resolved
srml/staking/src/lib.rs Outdated Show resolved Hide resolved
srml/staking/src/lib.rs Outdated Show resolved Hide resolved
srml/staking/src/lib.rs Outdated Show resolved Hide resolved
srml/staking/src/lib.rs Outdated Show resolved Hide resolved
srml/staking/src/lib.rs Outdated Show resolved Hide resolved
srml/staking/src/lib.rs Outdated Show resolved Hide resolved
srml/staking/src/lib.rs Outdated Show resolved Hide resolved
srml/staking/src/lib.rs Outdated Show resolved Hide resolved
@gavofyork
Copy link
Member

seems broadly decent to me.

@gavofyork gavofyork added A6-seemsok and removed A0-please_review Pull request needs code review. labels Jul 19, 2019
@gavofyork
Copy link
Member

@thiolliere doesn't build?

@gavofyork
Copy link
Member

@AlfonsoCev could you have a quick check that these code changes look like they have the correct logic?

@gavofyork
Copy link
Member

@kianenigma are you happy with this?


/// PNPoS: the target total payout to all validators (and their nominators) per era.
///
/// The value take into consideration desired interest rate and inflation rate (see module doc)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// The value take into consideration desired interest rate and inflation rate (see module doc)
/// The values taken into consideration are desired interest rate and inflation rate (see module doc)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(maybe my suggestion, but the sentence is ambiguous)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I improved a bit, actually it meant that I_NPoS function is defined using some constant which are the desired interest rate on ideal inflation rate and desired inflation rate


// This is guarantee not to overflow on whatever values.
// `num` must be inferior to `den` otherwise it will be reduce to `den`.
fn multiply_by_rational<N>(value: N, num: u32, den: u32) -> N
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be used in the future into support. Looks like something that other modules can also use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think having just this functions as is is interesting or should we provide a structure like RationalU32 { num: u32, den: u32 }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on your recommendation I think a RationalU32 with proper Mul impls will cover what I recommended. 👍

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't know the details of the math portion, but seems to be reasonably tested. Otherwise looks good.

thiolliere and others added 2 commits July 24, 2019 14:02
Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>
@gavofyork gavofyork merged commit 15ccaa4 into master Jul 24, 2019
@gavofyork gavofyork deleted the gui-reward-inflation branch July 24, 2019 17:25
@AlfonsoCev
Copy link

@thiolliere Observation is on inflation.rs, on the definition of function I_full: for full generality, we should have that I_full is equal to I_left when x <= x_ideal, and equal to I_right otherwise. Right now the division is set at x=0.5, which is the current value of the parameter x_ideal, but since we defined that parameter we should use it.

@gnunicorn gnunicorn modified the milestones: 2.1, Polkadot Mar 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants