Merge PR cosmos#5572: ADR 004 Implementation

CHANGELOG.md

@@ -37,14 +37,29 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
 
+### Client Breaking
+
+* (modules) [\#5572](https://github.com/cosmos/cosmos-sdk/pull/5572) The `/bank/balances/{address}` endpoint now returns all account
+balances or a single balance by denom when the `denom` query parameter is present.
+
 ### API Breaking Changes
 
 * (modules) [\#5555](https://github.com/cosmos/cosmos-sdk/pull/5555) Move x/auth/client/utils/ types and functions to x/auth/client/.
+* (modules) [\#5572](https://github.com/cosmos/cosmos-sdk/pull/5572) Move account balance logic and APIs from `x/auth` to `x/bank`.
 
 ### Bug Fixes
 
 * (x/bank) [\#5531](https://github.com/cosmos/cosmos-sdk/issues/5531) Added missing amount event to MsgMultiSend, emitted for each output.
 
+### State Machine Breaking
+
+* (modules) [\#5572](https://github.com/cosmos/cosmos-sdk/pull/5572) Separate balance from accounts per ADR 004.
+  * Account balances are now persisted and retrieved via the `x/bank` module.
+  * Vesting account interface has been modified to account for changes.
+  * Callers to `NewBaseVestingAccount` are responsible for verifying account balance in relation to
+  the original vesting amount.
+  * The `SendKeeper` and `ViewKeeper` interfaces in `x/bank` have been modified to account for changes.
+
 ### Improvements
 
 * (types) [\#5581](https://github.com/cosmos/cosmos-sdk/pull/5581) Add convenience functions {,Must}Bech32ifyAddressBytes.

docs/architecture/adr-004-split-denomination-keys.md

@@ -5,6 +5,7 @@
 - 2020-01-08: Initial version
 - 2020-01-09: Alterations to handle vesting accounts
 - 2020-01-14: Updates from review feedback
+- 2020-01-30: Updates from implementation
 
 ## Context
 
@@ -18,35 +19,74 @@ Balances shall be stored per-account & per-denomination under a denomination- an
 
 ### Account interface (x/auth)
 
-`GetCoins()` and `SetCoins()` will be removed from the account interface, since coin balances will now be stored in & managed by the bank module.
+`GetCoins()` and `SetCoins()` will be removed from the account interface, since coin balances will
+now be stored in & managed by the bank module.
 
-`SpendableCoinsVestingAccount()` and `TrackDelegation()` will be altered to take a bank keeper and a denomination as two additional arguments, which will be used to lookup the balances from the base account as necessary.
+The vesting account interface will replace `SpendableCoins` in favor of `LockedCoins` which does
+not require the account balance anymore. In addition, `TrackDelegation()`  will now accept the
+account balance of all tokens denominated in the vesting balance instead of loading the entire
+account balance.
 
-Vesting accounts will continue to store original vesting, delegated free, and delegated vesting coins (which is safe since these cannot contain arbitrary denominations).
+Vesting accounts will continue to store original vesting, delegated free, and delegated
+vesting coins (which is safe since these cannot contain arbitrary denominations).
 
 ### Bank keeper (x/bank)
 
-`GetBalance(addr AccAddress, denom string) sdk.Coin` and `SetBalance(addr AccAddress, coin sdk.Coin)` methods will be added to the bank keeper to retrieve & set balances, respectively.
+The following APIs will be added to the `x/bank` keeper:
 
-Balances will be stored first by the address, then by the denomination (the reverse is also possible, but retrieval of all balances for a single account is presumed to be more frequent):
+- `GetAllBalances(ctx Context, addr AccAddress) Coins`
+- `GetBalance(ctx Context, addr AccAddress, denom string) Coin`
+- `SetBalance(ctx Context, addr AccAddress, coin Coin)`
+- `LockedCoins(ctx Context, addr AccAddress) Coins`
+- `SpendableCoins(ctx Context, addr AccAddress) Coins`
+
+Additional APIs may be added to facilitate iteration and auxiliary functionality not essential to
+core functionality or persistence.
+
+Balances will be stored first by the address, then by the denomination (the reverse is also possible,
+but retrieval of all balances for a single account is presumed to be more frequent):
 
 ```golang
-func BalanceKey(addr sdk.AccAddress, denom string) []byte {
-  return append(append(BalanceKeyPrefix, addr.Bytes()...), []byte(denom)...)
+var BalancesPrefix = []byte("balances")
+
+func (k Keeper) SetBalance(ctx Context, addr AccAddress, balance Coin) error {
+  if !balance.IsValid() {
+    return err
+  }
+
+  store := ctx.KVStore(k.storeKey)
+  balancesStore := prefix.NewStore(store, BalancesPrefix)
+  accountStore := prefix.NewStore(balancesStore, addr.Bytes())
+
+  bz := Marshal(balance)
+  accountStore.Set([]byte(balance.Denom), bz)
+
+  return nil
 }
 ```
 
-`DelegateCoins()` and `UndelegateCoins()` will be altered to take a single `sdk.Coin` (one denomination & amount) instead of `sdk.Coins`, since they should only operate on one denomination. They will read balances directly instead of calling `GetCoins()` (which no longer exists).
+This will result in the balances being indexed by the byte representation of
+`balances/{address}/{denom}`.
+
+`DelegateCoins()` and `UndelegateCoins()` will be altered to only load each individual
+account balance by denomination found in the (un)delegation amount. As a result,
+any mutations to the account balance by will made by denomination.
 
-`SubtractCoins()` and `AddCoins()` will be altered to read & write the balances directly instead of calling `GetCoins()` / `SetCoins()` (which no longer exist).
+`SubtractCoins()` and `AddCoins()` will be altered to read & write the balances
+directly instead of calling `GetCoins()` / `SetCoins()` (which no longer exist).
 
-`trackDelegation()` and `trackUndelegation()` will be altered to read & write the balances directly instead of calling `GetCoins()` / `SetCoins()` (which no longer exist).
+`trackDelegation()` and `trackUndelegation()` will be altered to no longer update
+account balances.
 
-External APIs will need to scan all balances under an account to retain backwards-compatibility - additional methods should be added to fetch a balance for a single denomination only.
+External APIs will need to scan all balances under an account to retain backwards-compatibility. It
+is advised that these APIs use `GetBalance` and `SetBalance` instead of `GetAllBalances` when
+possible as to not load the entire account balance.
 
 ### Supply module
 
-The supply module, in order to implement the total supply invariant, will now need to scan all accounts & call `GetBalance` using the `x/bank` Keeper for the denomination in question, then sum the balances and check that they match the expected total supply.
+The supply module, in order to implement the total supply invariant, will now need
+to scan all accounts & call `GetAllBalances` using the `x/bank` Keeper, then sum
+the balances and check that they match the expected total supply.
 
 ## Status
 
@@ -56,18 +96,21 @@ Proposed.
 
 ### Positive
 
-- O(1) reads & writes of balances (with respect to the number of denominations for which an account has non-zero balances)
+- O(1) reads & writes of balances (with respect to the number of denominations for
+which an account has non-zero balances). Note, this does not relate to the actual
+I/O cost, rather the total number of direct reads needed.
 
 ### Negative
 
-- Slighly less efficient reads/writes when reading & writing all balances of a single account in a transaction.
+- Slightly less efficient reads/writes when reading & writing all balances of a
+single account in a transaction.
 
 ### Neutral
 
 None in particular.
 
 ## References
 
-Ref https://github.com/cosmos/cosmos-sdk/issues/4982
-Ref https://github.com/cosmos/cosmos-sdk/issues/5467
-Ref https://github.com/cosmos/cosmos-sdk/issues/5492
+- Ref: https://github.com/cosmos/cosmos-sdk/issues/4982
+- Ref: https://github.com/cosmos/cosmos-sdk/issues/5467
+- Ref: https://github.com/cosmos/cosmos-sdk/issues/5492

simapp/app.go

@@ -142,7 +142,7 @@ func NewSimApp(
 	bApp.SetAppVersion(version.Version)
 
 	keys := sdk.NewKVStoreKeys(
-		bam.MainStoreKey, auth.StoreKey, staking.StoreKey,
+		bam.MainStoreKey, auth.StoreKey, bank.StoreKey, staking.StoreKey,
 		supply.StoreKey, mint.StoreKey, distr.StoreKey, slashing.StoreKey,
 		gov.StoreKey, params.StoreKey, upgrade.StoreKey, evidence.StoreKey,
 	)
@@ -174,20 +174,20 @@ func NewSimApp(
 		app.cdc, keys[auth.StoreKey], app.subspaces[auth.ModuleName], auth.ProtoBaseAccount,
 	)
 	app.BankKeeper = bank.NewBaseKeeper(
-		app.AccountKeeper, app.subspaces[bank.ModuleName], app.BlacklistedAccAddrs(),
+		app.cdc, keys[bank.StoreKey], app.AccountKeeper, app.subspaces[bank.ModuleName], app.BlacklistedAccAddrs(),
 	)
 	app.SupplyKeeper = supply.NewKeeper(
 		app.cdc, keys[supply.StoreKey], app.AccountKeeper, app.BankKeeper, maccPerms,
 	)
 	stakingKeeper := staking.NewKeeper(
-		app.cdc, keys[staking.StoreKey], app.SupplyKeeper, app.subspaces[staking.ModuleName],
+		app.cdc, keys[staking.StoreKey], app.BankKeeper, app.SupplyKeeper, app.subspaces[staking.ModuleName],
 	)
 	app.MintKeeper = mint.NewKeeper(
 		app.cdc, keys[mint.StoreKey], app.subspaces[mint.ModuleName], &stakingKeeper,
 		app.SupplyKeeper, auth.FeeCollectorName,
 	)
 	app.DistrKeeper = distr.NewKeeper(
-		app.cdc, keys[distr.StoreKey], app.subspaces[distr.ModuleName], &stakingKeeper,
+		app.cdc, keys[distr.StoreKey], app.subspaces[distr.ModuleName], app.BankKeeper, &stakingKeeper,
 		app.SupplyKeeper, auth.FeeCollectorName, app.ModuleAccountAddrs(),
 	)
 	app.SlashingKeeper = slashing.NewKeeper(
@@ -231,12 +231,12 @@ func NewSimApp(
 		auth.NewAppModule(app.AccountKeeper),
 		bank.NewAppModule(app.BankKeeper, app.AccountKeeper),
 		crisis.NewAppModule(&app.CrisisKeeper),
-		supply.NewAppModule(app.SupplyKeeper, app.AccountKeeper),
-		gov.NewAppModule(app.GovKeeper, app.AccountKeeper, app.SupplyKeeper),
+		supply.NewAppModule(app.SupplyKeeper, app.BankKeeper, app.AccountKeeper),
+		gov.NewAppModule(app.GovKeeper, app.AccountKeeper, app.BankKeeper, app.SupplyKeeper),
 		mint.NewAppModule(app.MintKeeper),
-		slashing.NewAppModule(app.SlashingKeeper, app.AccountKeeper, app.StakingKeeper),
-		distr.NewAppModule(app.DistrKeeper, app.AccountKeeper, app.SupplyKeeper, app.StakingKeeper),
-		staking.NewAppModule(app.StakingKeeper, app.AccountKeeper, app.SupplyKeeper),
+		slashing.NewAppModule(app.SlashingKeeper, app.AccountKeeper, app.BankKeeper, app.StakingKeeper),
+		distr.NewAppModule(app.DistrKeeper, app.AccountKeeper, app.BankKeeper, app.SupplyKeeper, app.StakingKeeper),
+		staking.NewAppModule(app.StakingKeeper, app.AccountKeeper, app.BankKeeper, app.SupplyKeeper),
 		upgrade.NewAppModule(app.UpgradeKeeper),
 		evidence.NewAppModule(app.EvidenceKeeper),
 	)
@@ -265,12 +265,12 @@ func NewSimApp(
 	app.sm = module.NewSimulationManager(
 		auth.NewAppModule(app.AccountKeeper),
 		bank.NewAppModule(app.BankKeeper, app.AccountKeeper),
-		supply.NewAppModule(app.SupplyKeeper, app.AccountKeeper),
-		gov.NewAppModule(app.GovKeeper, app.AccountKeeper, app.SupplyKeeper),
+		supply.NewAppModule(app.SupplyKeeper, app.BankKeeper, app.AccountKeeper),
+		gov.NewAppModule(app.GovKeeper, app.AccountKeeper, app.BankKeeper, app.SupplyKeeper),
 		mint.NewAppModule(app.MintKeeper),
-		staking.NewAppModule(app.StakingKeeper, app.AccountKeeper, app.SupplyKeeper),
-		distr.NewAppModule(app.DistrKeeper, app.AccountKeeper, app.SupplyKeeper, app.StakingKeeper),
-		slashing.NewAppModule(app.SlashingKeeper, app.AccountKeeper, app.StakingKeeper),
+		staking.NewAppModule(app.StakingKeeper, app.AccountKeeper, app.BankKeeper, app.SupplyKeeper),
+		distr.NewAppModule(app.DistrKeeper, app.AccountKeeper, app.BankKeeper, app.SupplyKeeper, app.StakingKeeper),
+		slashing.NewAppModule(app.SlashingKeeper, app.AccountKeeper, app.BankKeeper, app.StakingKeeper),
 		params.NewAppModule(), // NOTE: only used for simulation to generate randomized param change proposals
 	)
 

simapp/genesis_account.go

@@ -31,9 +31,6 @@ type SimGenesisAccount struct {
 // Validate checks for errors on the vesting and module account parameters
 func (sga SimGenesisAccount) Validate() error {
 	if !sga.OriginalVesting.IsZero() {
-		if sga.OriginalVesting.IsAnyGT(sga.Coins) {
-			return errors.New("vesting amount cannot be greater than total amount")
-		}
 		if sga.StartTime >= sga.EndTime {
 			return errors.New("vesting start-time cannot be before end-time")
 		}

simapp/genesis_account_test.go

@@ -20,8 +20,7 @@ func TestSimGenesisAccountValidate(t *testing.T) {
 	vestingStart := time.Now().UTC()
 
 	coins := sdk.NewCoins(sdk.NewInt64Coin("test", 1000))
-	baseAcc := authtypes.NewBaseAccount(addr, nil, pubkey, 0, 0)
-	require.NoError(t, baseAcc.SetCoins(coins))
+	baseAcc := authtypes.NewBaseAccount(addr, pubkey, 0, 0)
 
 	testCases := []struct {
 		name    string
@@ -38,14 +37,14 @@ func TestSimGenesisAccountValidate(t *testing.T) {
 		{
 			"invalid basic account with mismatching address/pubkey",
 			simapp.SimGenesisAccount{
-				BaseAccount: authtypes.NewBaseAccount(addr, nil, secp256k1.GenPrivKey().PubKey(), 0, 0),
+				BaseAccount: authtypes.NewBaseAccount(addr, secp256k1.GenPrivKey().PubKey(), 0, 0),
 			},
 			true,
 		},
 		{
 			"valid basic account with module name",
 			simapp.SimGenesisAccount{
-				BaseAccount: authtypes.NewBaseAccount(sdk.AccAddress(crypto.AddressHash([]byte("testmod"))), nil, nil, 0, 0),
+				BaseAccount: authtypes.NewBaseAccount(sdk.AccAddress(crypto.AddressHash([]byte("testmod"))), nil, 0, 0),
 				ModuleName:  "testmod",
 			},
 			false,
@@ -78,16 +77,6 @@ func TestSimGenesisAccountValidate(t *testing.T) {
 			},
 			true,
 		},
-		{
-			"valid basic account with invalid original vesting coins",
-			simapp.SimGenesisAccount{
-				BaseAccount:     baseAcc,
-				OriginalVesting: coins.Add(coins...),
-				StartTime:       vestingStart.Unix(),
-				EndTime:         vestingStart.Add(1 * time.Hour).Unix(),
-			},
-			true,
-		},
 	}
 
 	for _, tc := range testCases {

simapp/test_helpers.go

@@ -1,7 +1,6 @@
 package simapp
 
 import (
-	"os"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -48,7 +47,7 @@ func Setup(isCheckTx bool) *SimApp {
 // genesis accounts.
 func SetupWithGenesisAccounts(genAccs []authexported.GenesisAccount) *SimApp {
 	db := dbm.NewMemDB()
-	app := NewSimApp(log.NewTMLogger(log.NewSyncWriter(os.Stdout)), db, nil, true, map[int64]bool{}, 0)
+	app := NewSimApp(log.NewNopLogger(), db, nil, true, map[int64]bool{}, 0)
 
 	// initialize the chain with the passed in genesis accounts
 	genesisState := NewDefaultGenesisState()
@@ -101,11 +100,9 @@ func AddTestAddrs(app *SimApp, ctx sdk.Context, accNum int, accAmt sdk.Int) []sd
 }
 
 // CheckBalance checks the balance of an account.
-func CheckBalance(t *testing.T, app *SimApp, addr sdk.AccAddress, exp sdk.Coins) {
+func CheckBalance(t *testing.T, app *SimApp, addr sdk.AccAddress, balances sdk.Coins) {
 	ctxCheck := app.BaseApp.NewContext(true, abci.Header{})
-	res := app.AccountKeeper.GetAccount(ctxCheck, addr)
-
-	require.True(t, exp.IsEqual(res.GetCoins()))
+	require.True(t, balances.IsEqual(app.BankKeeper.GetAllBalances(ctxCheck, addr)))
 }
 
 // SignCheckDeliver checks a generated signed transaction and simulates a

store/prefix/store_test.go

@@ -18,9 +18,7 @@ import (
 
 // copied from iavl/store_test.go
 var (
-	cacheSize        = 100
-	numRecent  int64 = 5
-	storeEvery int64 = 3
+	cacheSize = 100
 )
 
 func bz(s string) []byte { return []byte(s) }