From 55e6b25035541fa578c467e44702f18f37daffed Mon Sep 17 00:00:00 2001 From: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Date: Tue, 2 Jul 2019 17:19:21 +0200 Subject: [PATCH] Merge PR #4663: Refactor bank keeper by removing private funcs --- .pending/breaking/sdk/refactor-bank-keeper | 2 + x/bank/internal/keeper/keeper.go | 356 +++++++++------------ 2 files changed, 148 insertions(+), 210 deletions(-) create mode 100644 .pending/breaking/sdk/refactor-bank-keeper diff --git a/.pending/breaking/sdk/refactor-bank-keeper b/.pending/breaking/sdk/refactor-bank-keeper new file mode 100644 index 0000000000..09e3dbb524 --- /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 45946b605b..2bb763e1b2 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 -// 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 { +// 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 + } - if !amt.IsValid() { - return sdk.ErrInvalidCoins(amt.String()) + 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 { + + _, 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,7 +232,80 @@ func (keeper BaseSendKeeper) SendCoins( ), }) - return sendCoins(ctx, keeper.ak, fromAddr, toAddr, amt) + return nil +} + +// SubtractCoins subtracts amt from the coins at the addr. +// +// CONTRACT: If the account is a vesting account, the amount has to be spendable. +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 := keeper.ak.GetAccount(ctx, addr) + if acc != nil { + oldCoins = acc.GetCoins() + spendableCoins = acc.SpendableCoins(ctx.BlockHeader().Time) + } + + // For non-vesting accounts, spendable coins will simply be the original coins. + // So the check here is sufficient instead of subtracting from oldCoins. + _, hasNeg := spendableCoins.SafeSub(amt) + if hasNeg { + return amt, sdk.ErrInsufficientCoins( + fmt.Sprintf("insufficient account funds; %s < %s", spendableCoins, amt), + ) + } + + newCoins := oldCoins.Sub(amt) // should not panic as spendable coins was already checked + err := keeper.SetCoins(ctx, addr, newCoins) + + return newCoins, err +} + +// AddCoins adds amt to the coins at the addr. +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 := keeper.GetCoins(ctx, addr) + newCoins := oldCoins.Add(amt) + + if newCoins.IsAnyNegative() { + return amt, sdk.ErrInsufficientCoins( + fmt.Sprintf("insufficient account funds; %s < %s", oldCoins, amt), + ) + } + + err := keeper.SetCoins(ctx, addr, newCoins) + return newCoins, err +} + +// SetCoins sets the coins at the addr. +func (keeper BaseSendKeeper) SetCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) sdk.Error { + + if !amt.IsValid() { + return sdk.ErrInvalidCoins(amt.String()) + } + + acc := keeper.ak.GetAccount(ctx, addr) + if acc == nil { + acc = keeper.ak.NewAccountWithAddress(ctx, addr) + } + + err := acc.SetCoins(amt) + if err != nil { + panic(err) + } + + keeper.ak.SetAccount(ctx, acc) + return nil } // GetSendEnabled returns the current SendEnabled @@ -268,12 +350,16 @@ func (keeper BaseViewKeeper) Logger(ctx sdk.Context) log.Logger { // 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) + acc := keeper.ak.GetAccount(ctx, addr) + if acc == nil { + return sdk.NewCoins() + } + return acc.GetCoins() } // 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) + return keeper.GetCoins(ctx, addr).IsAllGTE(amt) } // Codespace returns the keeper's codespace. @@ -281,162 +367,11 @@ 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. -// -// 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) { - if !amt.IsValid() { - return nil, sdk.ErrInvalidCoins(amt.String()) - } - - oldCoins, spendableCoins := sdk.NewCoins(), sdk.NewCoins() - - acc := getAccount(ctx, ak, addr) - if acc != nil { - oldCoins = acc.GetCoins() - spendableCoins = acc.SpendableCoins(ctx.BlockHeader().Time) - } - - // For non-vesting accounts, spendable coins will simply be the original coins. - // So the check here is sufficient instead of subtracting from oldCoins. - _, hasNeg := spendableCoins.SafeSub(amt) - if hasNeg { - return amt, sdk.ErrInsufficientCoins( - fmt.Sprintf("insufficient account funds; %s < %s", spendableCoins, amt), - ) - } - - newCoins := oldCoins.Sub(amt) // should not panic as spendable coins was already checked - err := setCoins(ctx, ak, 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) { - if !amt.IsValid() { - return nil, sdk.ErrInvalidCoins(amt.String()) - } - - oldCoins := getCoins(ctx, ak, addr) - newCoins := oldCoins.Add(amt) - - if newCoins.IsAnyNegative() { - return amt, sdk.ErrInsufficientCoins( - fmt.Sprintf("insufficient account funds; %s < %s", oldCoins, amt), - ) - } - - err := setCoins(ctx, ak, 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 { - - _, err := subtractCoins(ctx, ak, fromAddr, amt) - if err != nil { - return err - } - - _, err = addCoins(ctx, ak, toAddr, amt) - if err != nil { - return err - } - - 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 - } - - for _, in := range inputs { - _, err := subtractCoins(ctx, ak, 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 := addCoins(ctx, ak, 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 -} - // 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 }