R4R: Write bank module specification, check spec/code consistency #2853
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## develop #2853 +/- ##
===========================================
+ Coverage 56.29% 56.86% +0.57%
===========================================
Files 120 120
Lines 8406 8298 -108
===========================================
- Hits 4732 4719 -13
+ Misses 3352 3261 -91
+ Partials 322 318 -4 |
|
Discovered two issues so far:
|
cwgoes
changed the title
ValarDragon
reviewed
Nov 23, 2018
ValarDragon
reviewed
Nov 23, 2018
ValarDragon
reviewed
Nov 23, 2018
sunnya97
reviewed
Nov 26, 2018
|
|
||
| # Keeper | ||
|
|
||
| ## Denom Metadata |
There was a problem hiding this comment.
Don't we still want this metadata store?
There was a problem hiding this comment.
AFAIU This is moreso a spec for what we want at launch, not a spec for the ideal #pre-ibc
There was a problem hiding this comment.
This should be done prelaunch.
- It makes the staking codebase much simpler (removing this Pool that's being constantly passed around)
- Any UI/block explorer will want this.
- Having the loose token supply accounted by staking module is absurd and causes all sorts of weird situations. Governance currently gets around this in a super super hacky way (creating a burn account where it sends coins in order to "burn them"). Now, the loose token supply known by staking doesn't reflect reality. Had a discussion with @jaekwon about this, he agreed it needs to be prelaunch.
There was a problem hiding this comment.
OK, agreed, I'll do that in this PR.
Never mind, @sunnya97 wants to do this! 🎉
| @@ -0,0 +1,130 @@ | |||
| ## Keepers | |||
|
|
|||
| The bank module provides three different exported keeper interfaces which can be passed to other modules which need to read or update account balances. Modules should use the least-permissive interface which provides the functionality they require. | |||
There was a problem hiding this comment.
Should the interfaces maybe go in the sdk (types) folder? And the structs go in the bank module?
alexanderbez
approved these changes
Nov 26, 2018
There was a problem hiding this comment.
Spec looks great @cwgoes -- left some minor comments, but overall LGTM.
mircea-c
pushed a commit
that referenced
this pull request
Dec 4, 2018
…istency * Update PENDING.md * New structure * Start transactions section * Remove MsgIssue * Update keepers.md * Add state.md * Update keepers.md, discovered #2887 * Move inputOutputCoins to BaseKeeper * Remove no-loner-applicable tests * More spec updates * Tiny cleanup * Clarify storage rationale * Warn the user * Remove extra newline
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #1277
Closes #2887
Also does a bit of minor cleanup in
x/bank.If you think anything else should be added to the spec, do comment!
docs/)PENDING.mdwith issue #Files changedin the github PR explorerFor Admin Use: