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.

Sign up for GitHub

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

Jump to bottom

fix zero coins #9229

Merged
merged 10 commits into from
Apr 30, 2021
Merged

fix zero coins #9229

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7ce4c1b
fix zero coins issue
atheeshp Apr 29, 2021
2abc40d
fix tests
atheeshp Apr 29, 2021
6665f8b
udpate tests
atheeshp Apr 29, 2021
bc55cbc
Merge branch 'master' of github.com:cosmos/cosmos-sdk into atheesh/fi…
atheeshp Apr 29, 2021
9c8ba35
Review change
atheeshp Apr 30, 2021
37898c5
add change log
atheeshp Apr 30, 2021
daecb06
Merge branch 'master' of github.com:cosmos/cosmos-sdk into atheesh/fi…
atheeshp Apr 30, 2021
5a98d94
Merge branch 'atheesh/fix-zero-coins' of github.com:cosmos/cosmos-sdk…
atheeshp Apr 30, 2021
93f08d9
review changes
atheeshp Apr 30, 2021
669ac72
review changes
atheeshp Apr 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/bank) [\#8434](https://github.com/cosmos/cosmos-sdk/pull/8434) Fix legacy REST API `GET /bank/total` and `GET /bank/total/{denom}` in swagger
* (x/slashing) [\#8427](https://github.com/cosmos/cosmos-sdk/pull/8427) Fix query signing infos command
* (server) [\#8399](https://github.com/cosmos/cosmos-sdk/pull/8399) fix gRPC-web flag default value
* (x/bank) [\#9229](https://github.com/cosmos/cosmos-sdk/pull/9229) Now zero coin balances cannot be added to balances & supply stores. If any denom becomes zero corresponding key gets deleted from store.

### Deprecated

Expand Down
2 changes: 0 additions & 2 deletions x/bank/keeper/genesis_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ func (suite *IntegrationTestSuite) TestExportGenesis() {
suite.Require().Len(exportGenesis.Params.SendEnabled, 0)
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
suite.Require().Equal(types.DefaultParams().DefaultSendEnabled, exportGenesis.Params.DefaultSendEnabled)
suite.Require().Equal(totalSupply, exportGenesis.Supply)
// add mint module balance as nil
expectedBalances = append(expectedBalances, types.Balance{Address: "cosmos1m3h30wlvsf8llruxtpukdvsy0km2kum8g38c8q", Coins: nil})
suite.Require().Equal(expectedBalances, exportGenesis.Balances)
suite.Require().Equal(expectedMetadata, exportGenesis.DenomMetadata)
}
Expand Down
20 changes: 14 additions & 6 deletions x/bank/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ func (k BaseKeeper) GetPaginatedTotalSupply(ctx sdk.Context, pagination *query.P
if err != nil {
return fmt.Errorf("unable to convert amount string to Int %v", err)
}
supply = append(supply, sdk.NewCoin(string(key), amount))

// `Add` omits the 0 coins addition to the `supply`.
supply = supply.Add(sdk.NewCoin(string(key), amount))
atheeshp marked this conversation as resolved.
Show resolved Hide resolved
return nil
})

Expand Down Expand Up @@ -124,7 +126,7 @@ func (k BaseKeeper) DelegateCoins(ctx sdk.Context, delegatorAddr, moduleAccAddr
balances := sdk.NewCoins()

for _, coin := range amt {
balance := k.GetBalance(ctx, delegatorAddr, coin.Denom)
balance := k.GetBalance(ctx, delegatorAddr, coin.GetDenom())
if balance.IsLT(coin) {
return sdkerrors.Wrapf(
sdkerrors.ErrInsufficientFunds, "failed to delegate; %s is smaller than %s", balance, amt,
Expand Down Expand Up @@ -426,14 +428,20 @@ func (k BaseKeeper) BurnCoins(ctx sdk.Context, moduleName string, amounts sdk.Co
}

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

intBytes, err := coin.Amount.Marshal()
if err != nil {
panic(fmt.Errorf("unable to marshal amount value %v", err))
}
supplyStore.Set([]byte(coin.GetDenom()), intBytes)

store := ctx.KVStore(k.storeKey)
supplyStore := prefix.NewStore(store, types.SupplyKey)

// Bank invariants and IBC requires to remove zero coins.
if coin.IsZero() {
supplyStore.Delete([]byte(coin.GetDenom()))
} else {
supplyStore.Set([]byte(coin.GetDenom()), intBytes)
}
}

func (k BaseKeeper) trackDelegation(ctx sdk.Context, addr sdk.AccAddress, balance, amt sdk.Coins) error {
Expand Down
89 changes: 53 additions & 36 deletions x/bank/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,16 @@ type IntegrationTestSuite struct {
queryClient types.QueryClient
}

func getModuleAccPerms() map[string][]string {
maccPerms := simapp.GetMaccPerms()
maccPerms[holder] = nil
maccPerms[authtypes.Burner] = []string{authtypes.Burner}
maccPerms[authtypes.Minter] = []string{authtypes.Minter}
maccPerms[multiPerm] = []string{authtypes.Burner, authtypes.Minter, authtypes.Staking}
maccPerms[randomPerm] = []string{"random"}
return maccPerms
}

func (suite *IntegrationTestSuite) SetupTest() {
app := simapp.Setup(false)
ctx := app.BaseApp.NewContext(false, tmproto.Header{})
Expand All @@ -89,17 +99,43 @@ func (suite *IntegrationTestSuite) SetupTest() {
}

func (suite *IntegrationTestSuite) TestSupply() {
app, ctx := suite.app, suite.ctx
app := simapp.Setup(false)
ctx := app.BaseApp.NewContext(false, tmproto.Header{Height: 1})
appCodec := simapp.MakeTestEncodingConfig().Marshaler

require := suite.Require()

// add module accounts to supply keeper
maccPerms := getModuleAccPerms()
authKeeper := authkeeper.NewAccountKeeper(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can add authKeepr and bank keeper constructor to the same function - it's always used and repeated next to the maccPerms.

appCodec, app.GetKey(types.StoreKey), app.GetSubspace(types.ModuleName),
authtypes.ProtoBaseAccount, maccPerms,
)
keeper := keeper.NewBaseKeeper(
appCodec, app.GetKey(types.StoreKey), authKeeper,
app.GetSubspace(types.ModuleName), make(map[string]bool),
)

initialPower := int64(100)
initTokens := suite.app.StakingKeeper.TokensFromConsensusPower(suite.ctx, initialPower)

totalSupply := sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, initTokens))
suite.NoError(app.BankKeeper.MintCoins(ctx, minttypes.ModuleName, totalSupply))

total, _, err := app.BankKeeper.GetPaginatedTotalSupply(ctx, &query.PageRequest{})
suite.Require().NoError(err)
suite.Require().Equal(totalSupply, total)
// set burnerAcc balance
authKeeper.SetModuleAccount(ctx, burnerAcc)
require.NoError(keeper.MintCoins(ctx, authtypes.Minter, totalSupply))
require.NoError(keeper.SendCoinsFromModuleToAccount(ctx, authtypes.Minter, burnerAcc.GetAddress(), totalSupply))

total, _, err := keeper.GetPaginatedTotalSupply(ctx, &query.PageRequest{})
require.NoError(err)
require.Equal(totalSupply, total)

// burning all supplied tokens
err = keeper.BurnCoins(ctx, authtypes.Burner, totalSupply)
require.NoError(err)

total, _, err = keeper.GetPaginatedTotalSupply(ctx, &query.PageRequest{})
require.NoError(err)
require.Equal(total.String(), "")
}

func (suite *IntegrationTestSuite) TestSendCoinsFromModuleToAccount_Blacklist() {
Expand All @@ -108,19 +144,13 @@ func (suite *IntegrationTestSuite) TestSendCoinsFromModuleToAccount_Blacklist()
appCodec := app.AppCodec()

// add module accounts to supply keeper
maccPerms := simapp.GetMaccPerms()
maccPerms[holder] = nil
maccPerms[authtypes.Burner] = []string{authtypes.Burner}
maccPerms[authtypes.Minter] = []string{authtypes.Minter}
maccPerms[multiPerm] = []string{authtypes.Burner, authtypes.Minter, authtypes.Staking}
maccPerms[randomPerm] = []string{"random"}

addr1 := sdk.AccAddress([]byte("addr1_______________"))

maccPerms := getModuleAccPerms()
authKeeper := authkeeper.NewAccountKeeper(
appCodec, app.GetKey(types.StoreKey), app.GetSubspace(types.ModuleName),
authtypes.ProtoBaseAccount, maccPerms,
)

addr1 := sdk.AccAddress([]byte("addr1_______________"))
keeper := keeper.NewBaseKeeper(
appCodec, app.GetKey(types.StoreKey), authKeeper,
app.GetSubspace(types.ModuleName), map[string]bool{addr1.String(): true},
Expand All @@ -138,12 +168,7 @@ func (suite *IntegrationTestSuite) TestSupply_SendCoins() {
appCodec := app.AppCodec()

// add module accounts to supply keeper
maccPerms := simapp.GetMaccPerms()
maccPerms[holder] = nil
maccPerms[authtypes.Burner] = []string{authtypes.Burner}
maccPerms[authtypes.Minter] = []string{authtypes.Minter}
maccPerms[multiPerm] = []string{authtypes.Burner, authtypes.Minter, authtypes.Staking}
maccPerms[randomPerm] = []string{"random"}
maccPerms := getModuleAccPerms()

authKeeper := authkeeper.NewAccountKeeper(
appCodec, app.GetKey(types.StoreKey), app.GetSubspace(types.ModuleName),
Expand Down Expand Up @@ -208,13 +233,7 @@ func (suite *IntegrationTestSuite) TestSupply_MintCoins() {
appCodec := app.AppCodec()

// add module accounts to supply keeper
maccPerms := simapp.GetMaccPerms()
maccPerms[holder] = nil
maccPerms[authtypes.Burner] = []string{authtypes.Burner}
maccPerms[authtypes.Minter] = []string{authtypes.Minter}
maccPerms[multiPerm] = []string{authtypes.Burner, authtypes.Minter, authtypes.Staking}
maccPerms[randomPerm] = []string{"random"}

maccPerms := getModuleAccPerms()
authKeeper := authkeeper.NewAccountKeeper(
appCodec, app.GetKey(types.StoreKey), app.GetSubspace(types.ModuleName),
authtypes.ProtoBaseAccount, maccPerms,
Expand Down Expand Up @@ -269,13 +288,7 @@ func (suite *IntegrationTestSuite) TestSupply_BurnCoins() {
appCodec := simapp.MakeTestEncodingConfig().Marshaler

// add module accounts to supply keeper
maccPerms := simapp.GetMaccPerms()
maccPerms[holder] = nil
maccPerms[authtypes.Burner] = []string{authtypes.Burner}
maccPerms[authtypes.Minter] = []string{authtypes.Minter}
maccPerms[multiPerm] = []string{authtypes.Burner, authtypes.Minter, authtypes.Staking}
maccPerms[randomPerm] = []string{"random"}

maccPerms := getModuleAccPerms()
authKeeper := authkeeper.NewAccountKeeper(
appCodec, app.GetKey(types.StoreKey), app.GetSubspace(types.ModuleName),
authtypes.ProtoBaseAccount, maccPerms,
Expand Down Expand Up @@ -349,11 +362,15 @@ func (suite *IntegrationTestSuite) TestSendCoinsNewAccount() {
app.BankKeeper.GetAllBalances(ctx, addr2)
suite.Require().Empty(app.BankKeeper.GetAllBalances(ctx, addr2))

sendAmt := sdk.NewCoins(newFooCoin(50), newBarCoin(25))
sendAmt := sdk.NewCoins(newFooCoin(50), newBarCoin(50))
suite.Require().NoError(app.BankKeeper.SendCoins(ctx, addr1, addr2, sendAmt))

acc2Balances := app.BankKeeper.GetAllBalances(ctx, addr2)
acc1Balances = app.BankKeeper.GetAllBalances(ctx, addr1)
suite.Require().Equal(sendAmt, acc2Balances)
updatedAcc1Bal := balances.Sub(sendAmt)
suite.Require().Len(acc1Balances, len(updatedAcc1Bal))
suite.Require().Equal(acc1Balances, updatedAcc1Bal)
suite.Require().NotNil(app.AccountKeeper.GetAccount(ctx, addr2))
}

Expand Down
9 changes: 7 additions & 2 deletions x/bank/keeper/send.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,8 +266,13 @@ func (k BaseSendKeeper) setBalance(ctx sdk.Context, addr sdk.AccAddress, balance

accountStore := k.getAccountStore(ctx, addr)

bz := k.cdc.MustMarshal(&balance)
accountStore.Set([]byte(balance.Denom), bz)
// Bank invariants require to not store zero balances.
if balance.IsZero() {
accountStore.Delete([]byte(balance.Denom))
} else {
bz := k.cdc.MustMarshal(&balance)
accountStore.Set([]byte(balance.Denom), bz)
}

return nil
}
Expand Down