Skip to content
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

R4R: F1 fee distribution #3099

Merged
merged 46 commits into from
Jan 16, 2019
Merged

R4R: F1 fee distribution #3099

merged 46 commits into from
Jan 16, 2019

Conversation

cwgoes
Copy link
Contributor

@cwgoes cwgoes commented Dec 13, 2018

Closes #2916

Implement the F1 spec in the "simple form" (inflation is left separate for the time being).

This PR is structured to be reviewed as a sequence of commits, commit-by-commit.
Each is independent (builds / passes simulations successfully, except for the import/export test which I left for the end).

  1. The first commit removes old fee distribution code which will no longer be used
  2. The second commit updates PENDING.md ( 😉 )
  3. The third commit implements the basic types, file layout, begin block allocation, and supply tracking.
  4. The fourth commit implements all the requisite hooks, period tracking, withdrawals
  5. The fifth commit implements a few bugfixes and simulation testing.

Saved for a separate PR: queriers and CLI commands to view state (mostly reincarnated from last time). I can add that as a PR to this one if we want to wait on those to merge, but easier to review just the substance first I think.

In my view, this version of fee distribution compares favorably to our current one: it is not approximate (so e.g. the whole class of concerns illuminated by #2764 are eliminated), it completely separates withdrawal for separate users (no delegator or validator can cause another to be forced to withdraw fees, thus eliminating the class of concerns of #2914), it makes it far easier for us to change the distribution function per-validator (so e.g. #2525 and #3023 can more easily be addressed), it is implementationally simpler, and it is more amenable to future improvements (see the spec).

One concern is potential error introduced by utilizing decimals - although F1 is analytically exact, we cannot use arbitrary-precision rational numbers in the state machine and must make do with decimals.

The validator update delay renders fee payments slightly inaccurate (e.g. delegations will be paid a block before their validator actually signs), but I don't think this impacts the implementation. The most complex / nuanced part is probably dealing with slashes, which must be iterated through on withdrawal.

F1 has not yet been tested on a testnet. Depending on results of review, given our current timeline, this seems like a candidate for "Red Wedding" (#3079).

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added entries in PENDING.md with issue #
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@cwgoes cwgoes added wip C:x/distribution distribution module related labels Dec 13, 2018
@codecov
Copy link

codecov bot commented Dec 13, 2018

Codecov Report

Merging #3099 into develop will increase coverage by 0.16%.
The diff coverage is 18.46%.

@@            Coverage Diff             @@
##           develop   #3099      +/-   ##
==========================================
+ Coverage    55.64%   55.8%   +0.16%     
==========================================
  Files          134     132       -2     
  Lines         9746    9488     -258     
==========================================
- Hits          5423    5295     -128     
+ Misses        3980    3858     -122     
+ Partials       343     335       -8

@cwgoes cwgoes force-pushed the cwgoes/mclaren-mcl33-first-pass branch 4 times, most recently from a6964b6 to e7e30e8 Compare December 14, 2018 15:20
@cwgoes cwgoes force-pushed the cwgoes/mclaren-mcl33-first-pass branch from e7e30e8 to c0babcc Compare December 14, 2018 15:23
@ValarDragon ValarDragon self-requested a review December 18, 2018 20:50
@cwgoes cwgoes force-pushed the cwgoes/mclaren-mcl33-first-pass branch from 67a555f to ff6ffe5 Compare January 2, 2019 23:48
@cwgoes cwgoes force-pushed the cwgoes/mclaren-mcl33-first-pass branch from 57b1f5d to 11515c2 Compare January 4, 2019 03:28
@cwgoes cwgoes requested a review from zramsay as a code owner January 4, 2019 03:28
@cwgoes cwgoes force-pushed the cwgoes/mclaren-mcl33-first-pass branch 8 times, most recently from ba26c9b to d5404bf Compare January 5, 2019 20:01
@cwgoes cwgoes force-pushed the cwgoes/mclaren-mcl33-first-pass branch from c627e11 to 63a589f Compare January 14, 2019 15:42
@cwgoes cwgoes force-pushed the cwgoes/mclaren-mcl33-first-pass branch from 63a589f to c9a4978 Compare January 14, 2019 15:43
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Did a first run through and left some comments. Overall, looks really good 👍

x/distribution/keeper/allocation.go Outdated Show resolved Hide resolved
x/distribution/keeper/allocation.go Show resolved Hide resolved
x/distribution/keeper/calculation.go Outdated Show resolved Hide resolved
x/distribution/keeper/calculation.go Outdated Show resolved Hide resolved
cmd/gaia/app/export.go Show resolved Hide resolved
x/distribution/keeper/calculation.go Outdated Show resolved Hide resolved
x/distribution/types/store.go Outdated Show resolved Hide resolved
x/distribution/keeper/calculation.go Outdated Show resolved Hide resolved
x/distribution/keeper/key.go Show resolved Hide resolved
x/distribution/types/fee_pool.go Show resolved Hide resolved
@bneiluj
Copy link

bneiluj commented Jan 15, 2019

So there will be no need to implement a specific Auto Withdraw + Delegate fee script for validators ?

@cwgoes
Copy link
Contributor Author

cwgoes commented Jan 15, 2019

So there will be no need to implement a specific Auto Withdraw + Delegate fee script for validators?

This instantiation of F1 does not include automatic inflation calculation, so re-bonding will still be necessary. Future versions may implement inflation per the spec, which would render it unnecessary.

@bneiluj
Copy link

bneiluj commented Jan 15, 2019

Got it. Thanks.

@cwgoes cwgoes mentioned this pull request Jan 15, 2019
Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

Generally a very clean implementation 👏

Main considerations:

  • currently the file organization is unintuitively shuffled for me - within keeper, we really ought to reorg functions into files relating to either objects (aka delegation.go, validator.go) or abci calls (tx.go, end_block.go etc. )
  • use of sdk.Dec for staking should be removed (see comment)
  • lack of NewXxx functions
  • there is a missing test which would ease my heart.

x/distribution/types/genesis.go Show resolved Hide resolved
x/distribution/types/store.go Outdated Show resolved Hide resolved
x/distribution/types/store.go Outdated Show resolved Hide resolved
x/distribution/genesis.go Show resolved Hide resolved
x/distribution/types/store.go Outdated Show resolved Hide resolved
x/distribution/keeper/calculation.go Outdated Show resolved Hide resolved
x/distribution/keeper/calculation_test.go Outdated Show resolved Hide resolved
x/distribution/keeper/calculation_test.go Outdated Show resolved Hide resolved
x/distribution/keeper/keeper.go Show resolved Hide resolved
x/distribution/keeper/keeper.go Show resolved Hide resolved
Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

🏁 🏁 🏁 🏎
🏅

Copy link
Member

@jackzampolin jackzampolin left a comment

Choose a reason for hiding this comment

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

Ready for merge! Great work @cwgoes and @cosmos/cosmossdk team!

@cwgoes cwgoes merged commit 2942f83 into develop Jan 16, 2019
@cwgoes cwgoes deleted the cwgoes/mclaren-mcl33-first-pass branch January 17, 2019 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/distribution distribution module related T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename "accum" in distribution
8 participants