R4R: Added bank module's total supply feature #2939

sunnya97 · 2018-11-29T06:28:54Z

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 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 · 2018-11-29T06:32:15Z

Codecov Report

Merging #2939 into develop will decrease coverage by 3.03%.
The diff coverage is 37.5%.

@@            Coverage Diff             @@
##           develop   #2939      +/-   ##
==========================================
- Coverage    54.83%   51.8%   -3.04%     
==========================================
  Files          133     137       +4     
  Lines         9559    9702     +143     
==========================================
- Hits          5242    5026     -216     
- Misses        3996    4343     +347     
- Partials       321     333      +12

sunnya97 · 2018-11-29T06:34:38Z

We need to readd the total supply check invariant, but this needs some discussion on the preferred way to do this (GetControlledCoins function on Keepers, Keepers have Accounts, better abstraction in bank, etc).

alexanderbez · 2018-11-29T14:18:07Z

x/bank/keeper.go

 	SendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) (sdk.Tags, sdk.Error)
+	InputOutputCoins(ctx sdk.Context, inputs []Input, outputs []Output) (sdk.Tags, sdk.Error)


I don't think we want I/O coins on a SendKeeper. What was the motivation for moving these methods from Keeper to SendKeeper?

I'm not sure why it wouldn't be here. Like SendCoins, InputOutputCoins sends.

cwgoes · 2018-11-29T21:59:22Z

Went ahead and merged the first PR to develop and retargeted this; should make review a bit easier.

cwgoes · 2018-12-05T14:02:57Z

Looks like this needs a rebase, happy to review after (if ready)!

cwgoes · 2018-12-12T12:39:25Z

Still needs a rebase (lots of changes with no apparent relation to this PR), still happy to review after.

sunnya97 · 2018-12-17T14:27:55Z

@cwgoes all the changes in Files Changed are intended for this PR. I'll fix merge conflicts though.

jackzampolin · 2018-12-17T17:44:16Z

Still some conflicts here.

alexanderbez · 2018-12-18T18:26:43Z

bump @sunnya97

alexanderbez · 2018-12-19T14:33:41Z

This has some merge conflicts needing resolution @sunnya97 + this is gonna have merge conflicts with #2996 :-(

rigelrozanski

needs a decent merge of develop.

I don't understand the usage of denom decimals... As far as I understand it decimals of a token should never exist... except for in the situation of decimal coins being created through minting - and those should probably just stay within minting until withdrawn as whole coins? It doesn't really seem reasonable to track those decimal portions within the bank keeper as I would hope that that total supply which includes decimals is really not used in any of the mechanisms of our system besides in minting/distribution. If there is a way we can get around not implementing that, I think it will clean up the code... Also this comment is made more relevant via the Validator Dec->Int PR which should be merged imminently

jackzampolin · 2019-01-09T20:37:54Z

I have a feeling we need this PR. Are there changes that would be required by F1 here? How do we get this in shape to merge?

cwgoes · 2019-01-14T12:37:52Z

Needs a rebase thanks to #3280, will review after.

jaekwon · 2019-01-15T17:10:47Z

x/gov/keeper.go

@@ -382,7 +377,7 @@ func (keeper Keeper) AddDeposit(ctx sdk.Context, proposalID uint64, depositorAdd
 	}

 	// Send coins from depositor's account to DepositedCoinsAccAddr account
-	_, err := keeper.ck.SendCoins(ctx, depositorAddr, DepositedCoinsAccAddr, depositAmount)
+	_, _, err := keeper.ck.SubtractCoins(ctx, depositorAddr, depositAmount)


I prefer that we have the DepositedCoinsAccAddr... it's weird that a deposit would change an invariant of the total number of coins.

It's also useful to be able to query for the total number of deposited coins, etc.

calling subtract/add is dangerous... e.g. a bug in this code could result in hyperinflation. The Send method ensures that only a limit is sent.

If you're concerned about the hash collision, we have bigger problems if there's a hash collision. It's strictly OK to create human-readable addresses, we also bumped up the []byte to 32, so lets make use of that.

I don't think than an account is not the right place to be storing this data. The sum total of all deposits isn't "an account". One suggestion we had instead was that we could have a function that all keepers expose that allow them to declare how many coins they have under their control. That way we could still have an invariant that checks the total supply is still correct.

AddCoins and SubtractCoins can't change the coin's supply, only MintAddCoins and BurnSubtractCoins can.

And yeah, not worried about a hash collision, but just the general abstraction here.

jaekwon · 2019-01-15T17:11:01Z

x/gov/keeper.go

@@ -427,7 +422,7 @@ func (keeper Keeper) RefundDeposits(ctx sdk.Context, proposalID uint64) {
 		deposit := &Deposit{}
 		keeper.cdc.MustUnmarshalBinaryLengthPrefixed(depositsIterator.Value(), deposit)

-		_, err := keeper.ck.SendCoins(ctx, DepositedCoinsAccAddr, deposit.Depositor, deposit.Amount)
+		_, _, err := keeper.ck.AddCoins(ctx, deposit.Depositor, deposit.Amount)


jaekwon · 2019-01-15T17:11:08Z

x/gov/keeper.go

@@ -447,7 +442,7 @@ func (keeper Keeper) DeleteDeposits(ctx sdk.Context, proposalID uint64) {
 		deposit := &Deposit{}
 		keeper.cdc.MustUnmarshalBinaryLengthPrefixed(depositsIterator.Value(), deposit)

-		_, err := keeper.ck.SendCoins(ctx, DepositedCoinsAccAddr, BurnedDepositCoinsAccAddr, deposit.Amount)
+		err := keeper.ck.BurnCoins(ctx, deposit.Amount)


sunnya97 · 2019-01-16T02:01:15Z

After discussion with @jaekwon we decided to push this off to post-launch.

cwgoes and others added 19 commits November 19, 2018 15:32

Update PENDING.md

8c6f2de

New structure

03e8121

Start transactions section

18ca55c

Merge branch 'develop' into cwgoes/bank-spec-code-consistency

c63b6f1

Remove MsgIssue

f25e562

Update keepers.md

ca0805b

Merge branch 'develop' into cwgoes/bank-spec-code-consistency

87954ab

Add state.md

ad7c0e1

Update keepers.md, discovered #2887

449a0af

Move inputOutputCoins to BaseKeeper

1a6bb0f

Remove no-loner-applicable tests

58997a9

More spec updates

abb0b73

Tiny cleanup

5e8aa3e

Clarify storage rationale

afe5076

Warn the user

81c4f51

Remove extra newline

d039e52

passes tests

3d4c877

fixed simulation

9f618eb

fixed name of bonded amount invariant

176cf0b

sunnya97 requested review from cwgoes, ebuchman, rigelrozanski and zramsay as code owners November 29, 2018 06:28

sunnya97 assigned cwgoes Nov 29, 2018

make governance normal and not ugly anymore

7558686

alexanderbez reviewed Nov 29, 2018

View reviewed changes

jackzampolin mentioned this pull request Nov 29, 2018

R4R: Write bank module specification, check spec/code consistency #2853

Merged

4 tasks

cwgoes changed the base branch from cwgoes/bank-spec-code-consistency to develop November 29, 2018 21:56

fixed merged conflicts

e405b43

fixed merge conflicts

5451a08

jackzampolin mentioned this pull request Dec 17, 2018

Release 0.29 Tracking #3139

Closed

alexanderbez added the wip label Dec 18, 2018

sunnya97 changed the title ~~WIP: Added bank module's total supply feature~~ R4R: Added bank module's total supply feature Dec 18, 2018

sunnya97 added ready-for-review and removed wip labels Dec 18, 2018

cwgoes mentioned this pull request Dec 18, 2018

"Red Wedding" (first GoS hard fork) master to-do list #3079

Closed

7 tasks

rigelrozanski suggested changes Dec 27, 2018

View reviewed changes

sunnya97 added 2 commits January 1, 2019 21:09

merged develop

fa2c365

in progress

b8af95e

sunnya97 mentioned this pull request Jan 9, 2019

Pre-ADR/Discussion - use sdk.Decimal for sdk.Coins #3251

Closed

jaekwon reviewed Jan 15, 2019

View reviewed changes

sunnya97 closed this Jan 16, 2019

sunnya97 mentioned this pull request Mar 4, 2019

R4R: Multimsg ante handler Jae's proposal #3787

Closed

5 tasks

sunnya97 mentioned this pull request Mar 25, 2019

Remove accounts to store deposits within Governance #3628

Closed

rigelrozanski mentioned this pull request Mar 25, 2019

Total Supply Feature #3972

Closed

alessio deleted the sunny/bank-refactor-3 branch June 15, 2019 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Added bank module's total supply feature #2939

R4R: Added bank module's total supply feature #2939

sunnya97 commented Nov 29, 2018

codecov bot commented Nov 29, 2018 •

edited

Loading

sunnya97 commented Nov 29, 2018

alexanderbez Nov 29, 2018 •

edited

Loading

jaekwon Jan 15, 2019

cwgoes commented Nov 29, 2018

cwgoes commented Dec 5, 2018

cwgoes commented Dec 12, 2018 •

edited

Loading

sunnya97 commented Dec 17, 2018

jackzampolin commented Dec 17, 2018

alexanderbez commented Dec 18, 2018

alexanderbez commented Dec 19, 2018

rigelrozanski left a comment

jackzampolin commented Jan 9, 2019

cwgoes commented Jan 14, 2019

jaekwon Jan 15, 2019

jaekwon Jan 15, 2019

jaekwon Jan 15, 2019

sunnya97 Jan 15, 2019

jaekwon Jan 15, 2019

jaekwon Jan 15, 2019

sunnya97 commented Jan 16, 2019

		SendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) (sdk.Tags, sdk.Error)
		InputOutputCoins(ctx sdk.Context, inputs []Input, outputs []Output) (sdk.Tags, sdk.Error)

R4R: Added bank module's total supply feature #2939

R4R: Added bank module's total supply feature #2939

Conversation

sunnya97 commented Nov 29, 2018

codecov bot commented Nov 29, 2018 • edited Loading

Codecov Report

sunnya97 commented Nov 29, 2018

alexanderbez Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

jaekwon Jan 15, 2019

Choose a reason for hiding this comment

cwgoes commented Nov 29, 2018

cwgoes commented Dec 5, 2018

cwgoes commented Dec 12, 2018 • edited Loading

sunnya97 commented Dec 17, 2018

jackzampolin commented Dec 17, 2018

alexanderbez commented Dec 18, 2018

alexanderbez commented Dec 19, 2018

rigelrozanski left a comment

Choose a reason for hiding this comment

jackzampolin commented Jan 9, 2019

cwgoes commented Jan 14, 2019

jaekwon Jan 15, 2019

Choose a reason for hiding this comment

jaekwon Jan 15, 2019

Choose a reason for hiding this comment

jaekwon Jan 15, 2019

Choose a reason for hiding this comment

sunnya97 Jan 15, 2019

Choose a reason for hiding this comment

jaekwon Jan 15, 2019

Choose a reason for hiding this comment

jaekwon Jan 15, 2019

Choose a reason for hiding this comment

sunnya97 commented Jan 16, 2019

codecov bot commented Nov 29, 2018 •

edited

Loading

alexanderbez Nov 29, 2018 •

edited

Loading

cwgoes commented Dec 12, 2018 •

edited

Loading