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: Supply Module #4255

Merged
merged 176 commits into from
Jun 28, 2019
Merged

R4R: Supply Module #4255

merged 176 commits into from
Jun 28, 2019

Conversation

fedekunze
Copy link
Collaborator

@fedekunze fedekunze commented May 2, 2019

Closes #3972
closes #3628
closes #4472

Full Spec: #4599

This PR creates a new x/supply module that passively keeps track of the total supply of the network. It introduces the concept of a ModuleAccount, which is a module/pool that holds or mint coins. The ModuleAccount address is created from the hash of a root string (aka ModuleAccount.Name).

Additionally, the supply Keeper has bank functionalities to transfer coins from and to an ModuleAccount using only its name.

With respect to breaking changes, this PR gets rid of all the pools of coins from store and replaces each of them with a ModuleAccount. The pools added/affected by this change are:

  • auth: FeeCollectionKeeper replaced for a fee collector BaseAccount (to avoid import cycles)
  • staking: the Pool was replaced for a NotBonded (held in staking as opposed to before) and Bonded ModuleAccount
  • governance: deposit and burn addresses: for a gov ModuleAccount
  • distribution: created a module ModuleAccount which controls the flow of coins into (from the fee collector acc) and out of the module store (to withdraw addresses).
  • mint: created a module ModuleMinterAccount that is allowed to mint coins before redistributing them to the fee collector account.

TODOs:

  • Fix simulation and invariance
  • Fix tests
  • NotBondedPool account should represent the coins only held in x/staking

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry: clog add [section] [stanza] [message]

  • 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)

@fedekunze fedekunze mentioned this pull request May 2, 2019
5 tasks
@codecov
Copy link

codecov bot commented May 2, 2019

Codecov Report

Merging #4255 into master will increase coverage by 0.2%.
The diff coverage is 62.75%.

@@            Coverage Diff            @@
##           master    #4255     +/-   ##
=========================================
+ Coverage   54.62%   54.83%   +0.2%     
=========================================
  Files         260      272     +12     
  Lines       16538    17020    +482     
=========================================
+ Hits         9034     9333    +299     
- Misses       6858     7001    +143     
- Partials      646      686     +40

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.

Good job @fedekunze! I left some initial feedback. I'm also still a bit confused as to the "AC" of this PR. I thought the supply keeper would be the module keeping track of all sorts of supplies (ie. the staking keeper will not store bonded/unbonded supply internally)?

x/distribution/keeper/keeper.go Show resolved Hide resolved
x/mint/abci_app.go Outdated Show resolved Hide resolved
x/mint/abci_app.go Outdated Show resolved Hide resolved
x/mint/expected_keepers.go Outdated Show resolved Hide resolved
x/staking/keeper/alias_functions.go Outdated Show resolved Hide resolved
x/supply/keeper/invariants.go Outdated Show resolved Hide resolved
x/supply/keeper/invariants.go Outdated Show resolved Hide resolved
x/supply/keeper/invariants.go Outdated Show resolved Hide resolved
x/supply/keeper/invariants.go Outdated Show resolved Hide resolved
x/supply/keeper/keeper.go Outdated Show resolved Hide resolved
@alexanderbez
Copy link
Contributor

Looks like this PR isn't complete. Should we close @fedekunze ?

x/supply/keeper/keeper.go Outdated Show resolved Hide resolved
x/supply/keeper/keeper.go Outdated Show resolved Hide resolved
x/supply/keeper/keeper.go Outdated Show resolved Hide resolved
x/supply/keeper/keeper.go Outdated Show resolved Hide resolved
@fedekunze fedekunze mentioned this pull request May 5, 2019
5 tasks
Copy link
Contributor

@sabau sabau left a comment

Choose a reason for hiding this comment

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

Tested, all good! What a massive work, thanks Fede!

@fedekunze
Copy link
Collaborator Author

I forgot to add the clog entry, will do it now

@fedekunze fedekunze mentioned this pull request Jun 28, 2019
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.

ACK -- just a few more points of nitpicking.

simapp/sim_test.go Outdated Show resolved Hide resolved
x/mint/internal/keeper/keeper.go Show resolved Hide resolved
x/mint/internal/keeper/keeper.go Show resolved Hide resolved
x/slashing/keeper.go Outdated Show resolved Hide resolved
x/slashing/keeper.go Outdated Show resolved Hide resolved
x/slashing/keeper.go Outdated Show resolved Hide resolved
x/slashing/keeper.go Outdated Show resolved Hide resolved
x/slashing/keeper.go Outdated Show resolved Hide resolved
x/staking/keeper/val_state_change.go Outdated Show resolved Hide resolved
x/staking/keeper/val_state_change.go Outdated Show resolved Hide resolved
fedekunze and others added 9 commits June 28, 2019 18:37
Co-Authored-By: Alexander Bezobchuk <[email protected]>
Co-Authored-By: Alexander Bezobchuk <[email protected]>
Co-Authored-By: Alexander Bezobchuk <[email protected]>
Co-Authored-By: Alexander Bezobchuk <[email protected]>
Co-Authored-By: Alexander Bezobchuk <[email protected]>
Co-Authored-By: Alexander Bezobchuk <[email protected]>
Co-Authored-By: Alexander Bezobchuk <[email protected]>
Co-Authored-By: Alexander Bezobchuk <[email protected]>
Co-Authored-By: Alexander Bezobchuk <[email protected]>
@jackzampolin
Copy link
Member

yes

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.

ACK ⚡️ 🐸 🎉 💯

@alexanderbez alexanderbez merged commit 3526784 into master Jun 28, 2019
@alexanderbez alexanderbez deleted the fedekunze/3972-supply branch June 28, 2019 20:12
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). T: State Machine Breaking State machine breaking changes (impacts consensus).
Projects
None yet