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

Improve set supply #8950

Merged
merged 27 commits into from
Mar 25, 2021
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
006852a
temp commit
jgimeno Mar 22, 2021
00416d3
Merge branch 'master' into jonathan/supply-improvement
jgimeno Mar 22, 2021
34c1bf8
remove set supply
jgimeno Mar 22, 2021
bb0511f
Merge branch 'jonathan/supply-improvement' of github.com:cosmos/cosmo…
jgimeno Mar 22, 2021
4daba57
Merge branch 'master' into jonathan/supply-improvement
jgimeno Mar 22, 2021
9daa707
fix supply
jgimeno Mar 22, 2021
a00dd17
remove keys
jgimeno Mar 22, 2021
af5c523
Merge branch 'master' into jonathan/supply-improvement
jgimeno Mar 22, 2021
463a7ec
Merge branch 'master' into jonathan/supply-improvement
Mar 23, 2021
141626a
Merge branch 'master' into jonathan/supply-improvement
fdymylja Mar 23, 2021
5d5be8c
improve supply set
jgimeno Mar 23, 2021
06c3d60
Merge branch 'jonathan/supply-improvement' of github.com:cosmos/cosmo…
jgimeno Mar 23, 2021
b51e816
Merge branch 'master' into jonathan/supply-improvement
jgimeno Mar 23, 2021
174d9bf
update changelog
jgimeno Mar 23, 2021
2e22746
Merge branch 'jonathan/supply-improvement' of github.com:cosmos/cosmo…
jgimeno Mar 23, 2021
9c33f45
improve linter
jgimeno Mar 23, 2021
aed0e8c
update setSupply to get only one coin
jgimeno Mar 23, 2021
0267d24
update genesis
jgimeno Mar 23, 2021
f265a5a
remove dirt
jgimeno Mar 23, 2021
0bdc7a3
save only supply
jgimeno Mar 23, 2021
34666bc
go fmt
jgimeno Mar 23, 2021
2ec3fa3
Merge branch 'master' into jonathan/supply-improvement
jgimeno Mar 24, 2021
2f8ca8f
Merge branch 'master' into jonathan/supply-improvement
Mar 24, 2021
2591510
Merge branch 'master' into jonathan/supply-improvement
jgimeno Mar 24, 2021
f223213
update rosetta test bootstrap
jgimeno Mar 24, 2021
7b5645f
Merge remote-tracking branch 'origin/master' into jonathan/supply-imp…
jgimeno Mar 25, 2021
58f2b4e
Merge branch 'master' into jonathan/supply-improvement
mergify[bot] Mar 25, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/auth) [\#8522](https://github.com/cosmos/cosmos-sdk/pull/8522) Allow to query all stored accounts
* (crypto/types) [\#8600](https://github.com/cosmos/cosmos-sdk/pull/8600) `CompactBitArray`: optimize the `NumTrueBitsBefore` method and add an `Equal` method.
* (x/upgrade) [\#8743](https://github.com/cosmos/cosmos-sdk/pull/8743) Add tracking module versions as per ADR-041
* (x/bank) [\#8950](https://github.com/cosmos/cosmos-sdk/pull/8950) Improve efficiency on supply updates.

### Bug Fixes

Expand Down
4 changes: 3 additions & 1 deletion x/bank/keeper/genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ func (k BaseKeeper) InitGenesis(ctx sdk.Context, genState *types.GenesisState) {
panic(fmt.Errorf("genesis supply is incorrect, expected %v, got %v", genState.Supply, totalSupply))
}

k.setSupply(ctx, totalSupply)
for _, supply := range totalSupply {
k.setSupply(ctx, supply)
}

for _, meta := range genState.DenomMetadata {
k.SetDenomMetaData(ctx, meta)
Expand Down
91 changes: 40 additions & 51 deletions x/bank/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,39 +175,14 @@ func (k BaseKeeper) GetSupply(ctx sdk.Context, denom string) sdk.Coin {
}
}

var coin sdk.Coin
err := k.cdc.UnmarshalBinaryBare(bz, &coin)
if err != nil {
panic(err)
amount, ok := sdk.NewIntFromString(string(bz))
if !ok {
panic("unexpected supply")
}

return coin
}

// SetSupply sets the Supply to store
func (k BaseKeeper) setSupply(ctx sdk.Context, supply sdk.Coins) {
store := ctx.KVStore(k.storeKey)
supplyStore := prefix.NewStore(store, types.SupplyKey)

var newSupply []sdk.Coin
storeSupply := k.GetTotalSupply(ctx)

// update supply for coins which have non zero amount
for _, coin := range storeSupply {
if supply.AmountOf(coin.Denom).IsZero() {
zeroCoin := &sdk.Coin{
Denom: coin.Denom,
Amount: sdk.NewInt(0),
}
bz := k.cdc.MustMarshalBinaryBare(zeroCoin)
supplyStore.Set([]byte(coin.Denom), bz)
}
}
newSupply = append(newSupply, supply...)

for i := range newSupply {
bz := k.cdc.MustMarshalBinaryBare(&supply[i])
supplyStore.Set([]byte(supply[i].Denom), bz)
return sdk.Coin{
Denom: denom,
Amount: amount,
}
}

Expand Down Expand Up @@ -354,7 +329,7 @@ func (k BaseKeeper) UndelegateCoinsFromModuleToAccount(

// MintCoins creates new coins from thin air and adds it to the module account.
// It will panic if the module account does not exist or is unauthorized.
func (k BaseKeeper) MintCoins(ctx sdk.Context, moduleName string, amt sdk.Coins) error {
func (k BaseKeeper) MintCoins(ctx sdk.Context, moduleName string, amounts sdk.Coins) error {
acc := k.ak.GetModuleAccount(ctx, moduleName)
if acc == nil {
panic(sdkerrors.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", moduleName))
Expand All @@ -364,31 +339,31 @@ func (k BaseKeeper) MintCoins(ctx sdk.Context, moduleName string, amt sdk.Coins)
panic(sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "module account %s does not have permissions to mint tokens", moduleName))
}

err := k.addCoins(ctx, acc.GetAddress(), amt)
err := k.addCoins(ctx, acc.GetAddress(), amounts)
if err != nil {
return err
}

// update total supply
supply := k.GetTotalSupply(ctx)
supply = supply.Add(amt...)

k.setSupply(ctx, supply)
for _, amount := range amounts {
supply := k.GetSupply(ctx, amount.GetDenom())
supply = supply.Add(amount)
k.setSupply(ctx, supply)
}

logger := k.Logger(ctx)
logger.Info("minted coins from module account", "amount", amt.String(), "from", moduleName)
logger.Info("minted coins from module account", "amount", amounts.String(), "from", moduleName)

// emit mint event
ctx.EventManager().EmitEvent(
types.NewCoinMintEvent(acc.GetAddress(), amt),
types.NewCoinMintEvent(acc.GetAddress(), amounts),
)

return nil
}

// BurnCoins burns coins deletes coins from the balance of the module account.
// It will panic if the module account does not exist or is unauthorized.
func (k BaseKeeper) BurnCoins(ctx sdk.Context, moduleName string, amt sdk.Coins) error {
func (k BaseKeeper) BurnCoins(ctx sdk.Context, moduleName string, amounts sdk.Coins) error {
acc := k.ak.GetModuleAccount(ctx, moduleName)
if acc == nil {
panic(sdkerrors.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", moduleName))
Expand All @@ -398,28 +373,35 @@ func (k BaseKeeper) BurnCoins(ctx sdk.Context, moduleName string, amt sdk.Coins)
panic(sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "module account %s does not have permissions to burn tokens", moduleName))
}

err := k.subUnlockedCoins(ctx, acc.GetAddress(), amt)
err := k.subUnlockedCoins(ctx, acc.GetAddress(), amounts)
if err != nil {
return err
}

// update total supply
supply := k.GetTotalSupply(ctx)
supply = supply.Sub(amt)

k.setSupply(ctx, supply)
for _, amount := range amounts {
supply := k.GetSupply(ctx, amount.GetDenom())
supply = supply.Sub(amount)
k.setSupply(ctx, supply)
}

logger := k.Logger(ctx)
logger.Info("burned tokens from module account", "amount", amt.String(), "from", moduleName)
logger.Info("burned tokens from module account", "amount", amounts.String(), "from", moduleName)

// emit burn event
ctx.EventManager().EmitEvent(
types.NewCoinBurnEvent(acc.GetAddress(), amt),
types.NewCoinBurnEvent(acc.GetAddress(), amounts),
)

return nil
}

func (k BaseKeeper) setSupply(ctx sdk.Context, coin sdk.Coin) {
store := ctx.KVStore(k.storeKey)
supplyStore := prefix.NewStore(store, types.SupplyKey)

supplyStore.Set([]byte(coin.GetDenom()), []byte(coin.Amount.String()))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not keeping a loop here?

  1. in all places where we use this function we have the following pattern: https://github.com/cosmos/cosmos-sdk/pull/8950/files#diff-9994bff9a7f71878caa036e9c924ab8b06e31490a17874fab72648ed3404d220R346
  2. if we move the loop here, we don't need to recreate and store and do additional allocations. @odeke-em was shaving recently such places.

Copy link
Member

Choose a reason for hiding this comment

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

An argument could be made for doing all of this logic inline inside MintCoins and BurnCoins

Copy link
Member

Choose a reason for hiding this comment

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

@jgimeno I wonder if there's value in making this setSupply(sdk.KVStore, sdk.Coin) to avoid the allocations from ctx.KVStore as @robert-zaremba is suggesting. Wdyt?


func (k BaseKeeper) trackDelegation(ctx sdk.Context, addr sdk.AccAddress, balance, amt sdk.Coins) error {
acc := k.ak.GetAccount(ctx, addr)
if acc == nil {
Expand Down Expand Up @@ -458,8 +440,15 @@ func (k BaseViewKeeper) IterateTotalSupply(ctx sdk.Context, cb func(sdk.Coin) bo
defer iterator.Close()

for ; iterator.Valid(); iterator.Next() {
var balance sdk.Coin
k.cdc.MustUnmarshalBinaryBare(iterator.Value(), &balance)
amount, ok := sdk.NewIntFromString(string(iterator.Value()))
if !ok {
panic("unexpected supply")
}

balance := sdk.Coin{
Denom: string(iterator.Key()),
Amount: amount,
}

if cb(balance) {
break
Expand Down