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鈥檒l 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

Conversation

@cwgoes
Copy link
Contributor

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)
@codecov

This comment has been minimized.

Copy link

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 Dec 14, 2018

@cwgoes cwgoes force-pushed the cwgoes/mclaren-mcl33-first-pass branch from e7e30e8 to c0babcc Dec 14, 2018

@ValarDragon ValarDragon self-requested a review Dec 18, 2018

@cwgoes cwgoes force-pushed the cwgoes/mclaren-mcl33-first-pass branch from 67a555f to ff6ffe5 Jan 2, 2019

@cwgoes cwgoes force-pushed the cwgoes/mclaren-mcl33-first-pass branch from 57b1f5d to 11515c2 Jan 4, 2019

@cwgoes cwgoes requested a review from zramsay as a code owner Jan 4, 2019

@cwgoes cwgoes force-pushed the cwgoes/mclaren-mcl33-first-pass branch 8 times, most recently from ba26c9b to d5404bf Jan 5, 2019

@cwgoes cwgoes force-pushed the cwgoes/mclaren-mcl33-first-pass branch from 63a589f to c9a4978 Jan 14, 2019

@alexanderbez
Copy link
Contributor

left a comment

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

Show resolved Hide resolved 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

alexanderbez and others added some commits Jan 15, 2019

Update x/distribution/keeper/allocation.go
Co-Authored-By: cwgoes <cwgoes@pluranimity.org>
Update x/distribution/keeper/calculation.go
Co-Authored-By: cwgoes <cwgoes@pluranimity.org>

cwgoes added some commits Jan 15, 2019

@bneiluj

This comment has been minimized.

Copy link

commented Jan 15, 2019

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

@cwgoes

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link

commented Jan 15, 2019

Got it. Thanks.

@cwgoes cwgoes referenced this pull request Jan 15, 2019

Closed

F1: Queriers #3307

@rigelrozanski
Copy link
Member

left a comment

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.
Show resolved Hide resolved 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
@rigelrozanski
Copy link
Member

left a comment

馃弫 馃弫 馃弫 馃弾
馃弲

@jackzampolin
Copy link
Contributor

left a comment

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

@cwgoes cwgoes merged commit 2942f83 into develop Jan 16, 2019

12 of 13 checks passed

codecov/patch 18.46% of diff hit (target 55.64%)
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: localnet Your tests passed on CircleCI!
Details
ci/circleci: setup_dependencies Your tests passed on CircleCI!
Details
ci/circleci: test_cover Your tests passed on CircleCI!
Details
ci/circleci: test_sim_gaia_fast Your tests passed on CircleCI!
Details
ci/circleci: test_sim_gaia_import_export Your tests passed on CircleCI!
Details
ci/circleci: test_sim_gaia_multi_seed Your tests passed on CircleCI!
Details
ci/circleci: test_sim_gaia_nondeterminism Your tests passed on CircleCI!
Details
ci/circleci: test_sim_gaia_simulation_after_import Your tests passed on CircleCI!
Details
ci/circleci: upload_coverage Your tests passed on CircleCI!
Details
codecov/project 55.8% (+0.16%) compared to eff1f7c
Details

@cwgoes cwgoes deleted the cwgoes/mclaren-mcl33-first-pass branch Jan 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.