diff --git a/.pending/breaking/sdk/refactor-bank-keeper b/.pending/breaking/sdk/refactor-bank-keeper new file mode 100644 index 000000000000..09e3dbb524c5 --- /dev/null +++ b/.pending/breaking/sdk/refactor-bank-keeper @@ -0,0 +1,2 @@ +#4663 Refactor bank keeper by removing private functions + - `InputOutputCoins`, `SetCoins`, `SubtractCoins` and `AddCoins` are now part of the `SendKeeper` instead of the `Keeper` interface diff --git a/x/bank/internal/keeper/keeper.go b/x/bank/internal/keeper/keeper.go index 45946b605b96..2bb763e1b28e 100644 --- a/x/bank/internal/keeper/keeper.go +++ b/x/bank/internal/keeper/keeper.go @@ -19,11 +19,6 @@ var _ Keeper = (*BaseKeeper)(nil) type Keeper interface { SendKeeper - SetCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) sdk.Error - SubtractCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) (sdk.Coins, sdk.Error) - AddCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) (sdk.Coins, sdk.Error) - InputOutputCoins(ctx sdk.Context, inputs []types.Input, outputs []types.Output) sdk.Error - DelegateCoins(ctx sdk.Context, delegatorAddr, moduleAccAddr sdk.AccAddress, amt sdk.Coins) sdk.Error UndelegateCoins(ctx sdk.Context, moduleAccAddr, delegatorAddr sdk.AccAddress, amt sdk.Coins) sdk.Error } @@ -49,47 +44,19 @@ func NewBaseKeeper(ak types.AccountKeeper, } } -// SetCoins sets the coins at the addr. -func (keeper BaseKeeper) SetCoins( - ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins, -) sdk.Error { - return setCoins(ctx, keeper.ak, addr, amt) -} - -// SubtractCoins subtracts amt from the coins at the addr. -func (keeper BaseKeeper) SubtractCoins( - ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins, -) (sdk.Coins, sdk.Error) { - return subtractCoins(ctx, keeper.ak, addr, amt) -} - -// AddCoins adds amt to the coins at the addr. -func (keeper BaseKeeper) AddCoins( - ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins, -) (sdk.Coins, sdk.Error) { - return addCoins(ctx, keeper.ak, addr, amt) -} - -// InputOutputCoins handles a list of inputs and outputs -func (keeper BaseKeeper) InputOutputCoins(ctx sdk.Context, inputs []types.Input, outputs []types.Output) sdk.Error { - return inputOutputCoins(ctx, keeper.ak, inputs, outputs) -} - // DelegateCoins performs delegation by deducting amt coins from an account with // address addr. For vesting accounts, delegations amounts are tracked for both // vesting and vested coins. // The coins are then transferred from the delegator address to a ModuleAccount address. // If any of the delegation amounts are negative, an error is returned. -func (keeper BaseKeeper) DelegateCoins( - ctx sdk.Context, delegatorAddr, moduleAccAddr sdk.AccAddress, amt sdk.Coins, -) sdk.Error { +func (keeper BaseKeeper) DelegateCoins(ctx sdk.Context, delegatorAddr, moduleAccAddr sdk.AccAddress, amt sdk.Coins) sdk.Error { - delegatorAcc := getAccount(ctx, keeper.ak, delegatorAddr) + delegatorAcc := keeper.ak.GetAccount(ctx, delegatorAddr) if delegatorAcc == nil { return sdk.ErrUnknownAddress(fmt.Sprintf("account %s does not exist", delegatorAddr)) } - moduleAcc := getAccount(ctx, keeper.ak, moduleAccAddr) + moduleAcc := keeper.ak.GetAccount(ctx, moduleAccAddr) if moduleAcc == nil { return sdk.ErrUnknownAddress(fmt.Sprintf("module account %s does not exist", moduleAccAddr)) } @@ -107,14 +74,13 @@ func (keeper BaseKeeper) DelegateCoins( ) } - // TODO: return error on account.TrackDelegation if err := trackDelegation(delegatorAcc, ctx.BlockHeader().Time, amt); err != nil { return sdk.ErrInternal(fmt.Sprintf("failed to track delegation: %v", err)) } - setAccount(ctx, keeper.ak, delegatorAcc) + keeper.ak.SetAccount(ctx, delegatorAcc) - _, err := addCoins(ctx, keeper.ak, moduleAccAddr, amt) + _, err := keeper.AddCoins(ctx, moduleAccAddr, amt) if err != nil { return err } @@ -127,16 +93,14 @@ func (keeper BaseKeeper) DelegateCoins( // vesting and vested coins. // The coins are then transferred from a ModuleAccount address to the delegator address. // If any of the undelegation amounts are negative, an error is returned. -func (keeper BaseKeeper) UndelegateCoins( - ctx sdk.Context, moduleAccAddr, delegatorAddr sdk.AccAddress, amt sdk.Coins, -) sdk.Error { +func (keeper BaseKeeper) UndelegateCoins(ctx sdk.Context, moduleAccAddr, delegatorAddr sdk.AccAddress, amt sdk.Coins) sdk.Error { - delegatorAcc := getAccount(ctx, keeper.ak, delegatorAddr) + delegatorAcc := keeper.ak.GetAccount(ctx, delegatorAddr) if delegatorAcc == nil { return sdk.ErrUnknownAddress(fmt.Sprintf("account %s does not exist", delegatorAddr)) } - moduleAcc := getAccount(ctx, keeper.ak, moduleAccAddr) + moduleAcc := keeper.ak.GetAccount(ctx, moduleAccAddr) if moduleAcc == nil { return sdk.ErrUnknownAddress(fmt.Sprintf("module account %s does not exist", moduleAccAddr)) } @@ -154,18 +118,16 @@ func (keeper BaseKeeper) UndelegateCoins( ) } - err := setCoins(ctx, keeper.ak, moduleAccAddr, newCoins) + err := keeper.SetCoins(ctx, moduleAccAddr, newCoins) if err != nil { return err } - // TODO: return error on account.TrackUndelegation if err := trackUndelegation(delegatorAcc, amt); err != nil { return sdk.ErrInternal(fmt.Sprintf("failed to track undelegation: %v", err)) } - setAccount(ctx, keeper.ak, delegatorAcc) - + keeper.ak.SetAccount(ctx, delegatorAcc) return nil } @@ -174,8 +136,13 @@ func (keeper BaseKeeper) UndelegateCoins( type SendKeeper interface { ViewKeeper + InputOutputCoins(ctx sdk.Context, inputs []types.Input, outputs []types.Output) sdk.Error SendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) sdk.Error + SubtractCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) (sdk.Coins, sdk.Error) + AddCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) (sdk.Coins, sdk.Error) + SetCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) sdk.Error + GetSendEnabled(ctx sdk.Context) bool SetSendEnabled(ctx sdk.Context, enabled bool) } @@ -202,14 +169,56 @@ func NewBaseSendKeeper(ak types.AccountKeeper, } } -// TODO combine with sendCoins +// InputOutputCoins handles a list of inputs and outputs +func (keeper BaseSendKeeper) InputOutputCoins(ctx sdk.Context, inputs []types.Input, outputs []types.Output) sdk.Error { + // Safety check ensuring that when sending coins the keeper must maintain the + // Check supply invariant and validity of Coins. + if err := types.ValidateInputsOutputs(inputs, outputs); err != nil { + return err + } + + for _, in := range inputs { + _, err := keeper.SubtractCoins(ctx, in.Address, in.Coins) + if err != nil { + return err + } + + ctx.EventManager().EmitEvent( + sdk.NewEvent( + sdk.EventTypeMessage, + sdk.NewAttribute(types.AttributeKeySender, in.Address.String()), + ), + ) + } + + for _, out := range outputs { + _, err := keeper.AddCoins(ctx, out.Address, out.Coins) + if err != nil { + return err + } + + ctx.EventManager().EmitEvent( + sdk.NewEvent( + types.EventTypeTransfer, + sdk.NewAttribute(types.AttributeKeyRecipient, out.Address.String()), + ), + ) + } + + return nil +} + // SendCoins moves coins from one account to another -func (keeper BaseSendKeeper) SendCoins( - ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins, -) sdk.Error { +func (keeper BaseSendKeeper) SendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) sdk.Error { - if !amt.IsValid() { - return sdk.ErrInvalidCoins(amt.String()) + _, err := keeper.SubtractCoins(ctx, fromAddr, amt) + if err != nil { + return err + } + + _, err = keeper.AddCoins(ctx, toAddr, amt) + if err != nil { + return err } ctx.EventManager().EmitEvents(sdk.Events{ @@ -223,119 +232,21 @@ func (keeper BaseSendKeeper) SendCoins( ), }) - return sendCoins(ctx, keeper.ak, fromAddr, toAddr, amt) -} - -// GetSendEnabled returns the current SendEnabled -// nolint: errcheck -func (keeper BaseSendKeeper) GetSendEnabled(ctx sdk.Context) bool { - var enabled bool - keeper.paramSpace.Get(ctx, types.ParamStoreKeySendEnabled, &enabled) - return enabled -} - -// SetSendEnabled sets the send enabled -func (keeper BaseSendKeeper) SetSendEnabled(ctx sdk.Context, enabled bool) { - keeper.paramSpace.Set(ctx, types.ParamStoreKeySendEnabled, &enabled) -} - -var _ ViewKeeper = (*BaseViewKeeper)(nil) - -// ViewKeeper defines a module interface that facilitates read only access to -// account balances. -type ViewKeeper interface { - GetCoins(ctx sdk.Context, addr sdk.AccAddress) sdk.Coins - HasCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) bool - - Codespace() sdk.CodespaceType -} - -// BaseViewKeeper implements a read only keeper implementation of ViewKeeper. -type BaseViewKeeper struct { - ak types.AccountKeeper - codespace sdk.CodespaceType -} - -// NewBaseViewKeeper returns a new BaseViewKeeper. -func NewBaseViewKeeper(ak types.AccountKeeper, codespace sdk.CodespaceType) BaseViewKeeper { - return BaseViewKeeper{ak: ak, codespace: codespace} -} - -// Logger returns a module-specific logger. -func (keeper BaseViewKeeper) Logger(ctx sdk.Context) log.Logger { - return ctx.Logger().With("module", fmt.Sprintf("x/%s", types.ModuleName)) -} - -// GetCoins returns the coins at the addr. -func (keeper BaseViewKeeper) GetCoins(ctx sdk.Context, addr sdk.AccAddress) sdk.Coins { - return getCoins(ctx, keeper.ak, addr) -} - -// HasCoins returns whether or not an account has at least amt coins. -func (keeper BaseViewKeeper) HasCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) bool { - return hasCoins(ctx, keeper.ak, addr, amt) -} - -// Codespace returns the keeper's codespace. -func (keeper BaseViewKeeper) Codespace() sdk.CodespaceType { - return keeper.codespace -} - -// getCoins returns the account coins -func getCoins(ctx sdk.Context, am types.AccountKeeper, addr sdk.AccAddress) sdk.Coins { - acc := am.GetAccount(ctx, addr) - if acc == nil { - return sdk.NewCoins() - } - return acc.GetCoins() -} - -func setCoins(ctx sdk.Context, ak types.AccountKeeper, addr sdk.AccAddress, amt sdk.Coins) sdk.Error { - if !amt.IsValid() { - return sdk.ErrInvalidCoins(amt.String()) - } - - acc := ak.GetAccount(ctx, addr) - if acc == nil { - acc = ak.NewAccountWithAddress(ctx, addr) - } - - err := acc.SetCoins(amt) - if err != nil { - // Handle w/ #870 - panic(err) - } - - ak.SetAccount(ctx, acc) return nil } -// HasCoins returns whether or not an account has at least amt coins. -func hasCoins(ctx sdk.Context, ak types.AccountKeeper, addr sdk.AccAddress, amt sdk.Coins) bool { - return getCoins(ctx, ak, addr).IsAllGTE(amt) -} - -// getAccount implements AccountKeeper -func getAccount(ctx sdk.Context, ak types.AccountKeeper, addr sdk.AccAddress) exported.Account { - return ak.GetAccount(ctx, addr) -} - -// setAccount implements AccountKeeper -func setAccount(ctx sdk.Context, ak types.AccountKeeper, acc exported.Account) { - ak.SetAccount(ctx, acc) -} - -// subtractCoins subtracts amt coins from an account with the given address addr. +// SubtractCoins subtracts amt from the coins at the addr. // // CONTRACT: If the account is a vesting account, the amount has to be spendable. -func subtractCoins(ctx sdk.Context, ak types.AccountKeeper, addr sdk.AccAddress, amt sdk.Coins) (sdk.Coins, sdk.Error) { +func (keeper BaseSendKeeper) SubtractCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) (sdk.Coins, sdk.Error) { + if !amt.IsValid() { return nil, sdk.ErrInvalidCoins(amt.String()) } oldCoins, spendableCoins := sdk.NewCoins(), sdk.NewCoins() - acc := getAccount(ctx, ak, addr) + acc := keeper.ak.GetAccount(ctx, addr) if acc != nil { oldCoins = acc.GetCoins() spendableCoins = acc.SpendableCoins(ctx.BlockHeader().Time) @@ -351,18 +262,19 @@ func subtractCoins(ctx sdk.Context, ak types.AccountKeeper, addr sdk.AccAddress, } newCoins := oldCoins.Sub(amt) // should not panic as spendable coins was already checked - err := setCoins(ctx, ak, addr, newCoins) + err := keeper.SetCoins(ctx, addr, newCoins) return newCoins, err } // AddCoins adds amt to the coins at the addr. -func addCoins(ctx sdk.Context, ak types.AccountKeeper, addr sdk.AccAddress, amt sdk.Coins) (sdk.Coins, sdk.Error) { +func (keeper BaseSendKeeper) AddCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) (sdk.Coins, sdk.Error) { + if !amt.IsValid() { return nil, sdk.ErrInvalidCoins(amt.String()) } - oldCoins := getCoins(ctx, ak, addr) + oldCoins := keeper.GetCoins(ctx, addr) newCoins := oldCoins.Add(amt) if newCoins.IsAnyNegative() { @@ -371,72 +283,95 @@ func addCoins(ctx sdk.Context, ak types.AccountKeeper, addr sdk.AccAddress, amt ) } - err := setCoins(ctx, ak, addr, newCoins) - + err := keeper.SetCoins(ctx, addr, newCoins) return newCoins, err } -// SendCoins moves coins from one account to another -// Returns ErrInvalidCoins if amt is invalid. -func sendCoins(ctx sdk.Context, ak types.AccountKeeper, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins) sdk.Error { +// SetCoins sets the coins at the addr. +func (keeper BaseSendKeeper) SetCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) sdk.Error { - _, err := subtractCoins(ctx, ak, fromAddr, amt) - if err != nil { - return err + if !amt.IsValid() { + return sdk.ErrInvalidCoins(amt.String()) } - _, err = addCoins(ctx, ak, toAddr, amt) + acc := keeper.ak.GetAccount(ctx, addr) + if acc == nil { + acc = keeper.ak.NewAccountWithAddress(ctx, addr) + } + + err := acc.SetCoins(amt) if err != nil { - return err + panic(err) } + keeper.ak.SetAccount(ctx, acc) return nil } -// InputOutputCoins handles a list of inputs and outputs -// NOTE: Make sure to revert state changes from tx on error -func inputOutputCoins(ctx sdk.Context, ak types.AccountKeeper, inputs []types.Input, outputs []types.Output) sdk.Error { - // Safety check ensuring that when sending coins the keeper must maintain the - // Check supply invariant and validity of Coins. - if err := types.ValidateInputsOutputs(inputs, outputs); err != nil { - return err - } +// GetSendEnabled returns the current SendEnabled +// nolint: errcheck +func (keeper BaseSendKeeper) GetSendEnabled(ctx sdk.Context) bool { + var enabled bool + keeper.paramSpace.Get(ctx, types.ParamStoreKeySendEnabled, &enabled) + return enabled +} - for _, in := range inputs { - _, err := subtractCoins(ctx, ak, in.Address, in.Coins) - if err != nil { - return err - } +// SetSendEnabled sets the send enabled +func (keeper BaseSendKeeper) SetSendEnabled(ctx sdk.Context, enabled bool) { + keeper.paramSpace.Set(ctx, types.ParamStoreKeySendEnabled, &enabled) +} - ctx.EventManager().EmitEvent( - sdk.NewEvent( - sdk.EventTypeMessage, - sdk.NewAttribute(types.AttributeKeySender, in.Address.String()), - ), - ) - } +var _ ViewKeeper = (*BaseViewKeeper)(nil) - for _, out := range outputs { - _, err := addCoins(ctx, ak, out.Address, out.Coins) - if err != nil { - return err - } +// ViewKeeper defines a module interface that facilitates read only access to +// account balances. +type ViewKeeper interface { + GetCoins(ctx sdk.Context, addr sdk.AccAddress) sdk.Coins + HasCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) bool - ctx.EventManager().EmitEvent( - sdk.NewEvent( - types.EventTypeTransfer, - sdk.NewAttribute(types.AttributeKeyRecipient, out.Address.String()), - ), - ) + Codespace() sdk.CodespaceType +} + +// BaseViewKeeper implements a read only keeper implementation of ViewKeeper. +type BaseViewKeeper struct { + ak types.AccountKeeper + codespace sdk.CodespaceType +} + +// NewBaseViewKeeper returns a new BaseViewKeeper. +func NewBaseViewKeeper(ak types.AccountKeeper, codespace sdk.CodespaceType) BaseViewKeeper { + return BaseViewKeeper{ak: ak, codespace: codespace} +} + +// Logger returns a module-specific logger. +func (keeper BaseViewKeeper) Logger(ctx sdk.Context) log.Logger { + return ctx.Logger().With("module", fmt.Sprintf("x/%s", types.ModuleName)) +} + +// GetCoins returns the coins at the addr. +func (keeper BaseViewKeeper) GetCoins(ctx sdk.Context, addr sdk.AccAddress) sdk.Coins { + acc := keeper.ak.GetAccount(ctx, addr) + if acc == nil { + return sdk.NewCoins() } + return acc.GetCoins() +} - return nil +// HasCoins returns whether or not an account has at least amt coins. +func (keeper BaseViewKeeper) HasCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) bool { + return keeper.GetCoins(ctx, addr).IsAllGTE(amt) +} + +// Codespace returns the keeper's codespace. +func (keeper BaseViewKeeper) Codespace() sdk.CodespaceType { + return keeper.codespace } // CONTRACT: assumes that amt is valid. func trackDelegation(acc exported.Account, blockTime time.Time, amt sdk.Coins) error { vacc, ok := acc.(exported.VestingAccount) if ok { + // TODO: return error on account.TrackDelegation vacc.TrackDelegation(blockTime, amt) return nil } @@ -448,6 +383,7 @@ func trackDelegation(acc exported.Account, blockTime time.Time, amt sdk.Coins) e func trackUndelegation(acc exported.Account, amt sdk.Coins) error { vacc, ok := acc.(exported.VestingAccount) if ok { + // TODO: return error on account.TrackUndelegation vacc.TrackUndelegation(amt) return nil }