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

R4R: Write bank module specification, check spec/code consistency #2853

Merged
merged 17 commits into from
Nov 29, 2018
Merged
1 change: 1 addition & 0 deletions PENDING.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ IMPROVEMENTS
* SDK
- [x/mock/simulation] [\#2720] major cleanup, introduction of helper objects, reorganization
- \#2821 Codespaces are now strings
- \#1277 Complete bank module specification
- [types] #2776 Improve safety of `Coin` and `Coins` types. Various functions
and methods will panic when a negative amount is discovered.
- #2815 Gas unit fields changed from `int64` to `uint64`.
Expand Down
26 changes: 26 additions & 0 deletions docs/spec/bank/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Bank module specification

## Abstract

This document specifies the bank module of the Cosmos SDK.

The bank module is responsible for handling multi-asset coin transfers between
accounts and tracking special-case pseudo-transfers which must work differently
with particular kinds of accounts (notably delegating/undelegating for vesting
accounts). It exposes several interfaces with varying capabilities for secure
interaction with other modules which must alter user balances.

This module will be used in the Cosmos Hub.

## Contents

1. **[State](state.md)**
1. **[Keepers](keepers.md)**
1. [Common Types](keepers.md#common-types)
1. [Input](keepers.md#input)
1. [Output](keepers.md#output)
1. [BaseKeeper](keepers.md#basekeeper)
1. [SendKeeper](keepers.md#sendkeeper)
1. [ViewKeeper](keepers.md#viewkeeper)
1. **[Transactions](transactions.md)**
1. [MsgSend](transactions.md#msgsend)
25 changes: 0 additions & 25 deletions docs/spec/bank/WIP_keeper.md

This file was deleted.

131 changes: 131 additions & 0 deletions docs/spec/bank/keepers.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
## Keepers

The bank module provides three different exported keeper interfaces which can be passed to other modules which need to read or update account balances. Modules should use the least-permissive interface which provides the functionality they require.
cwgoes marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Should the interfaces maybe go in the sdk (types) folder? And the structs go in the bank module?


Note that you should always review the `bank` module code to ensure that permissions are limited in the way that you expect.

### Common Types

#### Input

An input of a multiparty transfer

```golang
type Input struct {
Address AccAddress
Coins Coins
}
```

#### Output

An output of a multiparty transfer.

```golang
type Output struct {
Address AccAddress
Coins Coins
}
```

### BaseKeeper

The base keeper provides full-permission access: the ability to arbitrary modify any account's balance and mint or burn coins.

```golang
type BaseKeeper interface {
SetCoins(addr AccAddress, amt Coins)
SubtractCoins(addr AccAddress, amt Coins)
AddCoins(addr AccAddress, amt Coins)
InputOutputCoins(inputs []Input, outputs []Output)
}
```

`setCoins` fetches an account by address, sets the coins on the account, and saves the account.

```
setCoins(addr AccAddress, amt Coins)
account = accountKeeper.getAccount(addr)
if account == nil
fail with "no account found"
account.Coins = amt
accountKeeper.setAccount(account)
```

`subtractCoins` fetches the coins of an account, subtracts the provided amount, and saves the account. This decreases the total supply.

```
subtractCoins(addr AccAddress, amt Coins)
oldCoins = getCoins(addr)
newCoins = oldCoins - amt
if newCoins < 0
fail with "cannot end up with negative coins"
setCoins(addr, newCoins)
```

`addCoins` fetches the coins of an account, adds the provided amount, and saves the account. This increases the total supply.

```
addCoins(addr AccAddress, amt Coins)
oldCoins = getCoins(addr)
newCoins = oldCoins + amt
setCoins(addr, newCoins)
```

`inputOutputCoins` transfers coins from any number of input accounts to any number of output accounts.

cwgoes marked this conversation as resolved.
Show resolved Hide resolved
```
inputOutputCoins(inputs []Input, outputs []Output)
for input in inputs
subtractCoins(input.Address, input.Coins)
for output in outputs
addCoins(output.Address, output.Coins)
```

### SendKeeper

The send keeper provides access to account balances and the ability to transfer coins between accounts, but not to alter the total supply (mint or burn coins).

```golang
type SendKeeper interface {
SendCoins(from AccAddress, to AccAddress, amt Coins)
}
```

`sendCoins` transfers coins from one account to another.

```
sendCoins(from AccAddress, to AccAddress, amt Coins)
subtractCoins(from, amt)
addCoins(to, amt)
```

### ViewKeeper

The view keeper provides read-only access to account balances but no balance alteration functionality. All balance lookups are `O(1)`.

```golang
type ViewKeeper interface {
GetCoins(addr AccAddress) Coins
HasCoins(addr AccAddress, amt Coins) bool
}
```

`getCoins` returns the coins associated with an account.

```
getCoins(addr AccAddress)
account = accountKeeper.getAccount(addr)
if account == nil
return Coins{}
return account.Coins
```

`hasCoins` returns whether or not an account has at least the provided amount of coins.

```
hasCoins(addr AccAddress, amt Coins)
account = accountKeeper.getAccount(addr)
coins = getCoins(addr)
return coins >= amt
```
5 changes: 5 additions & 0 deletions docs/spec/bank/state.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
## State

Presently, the bank module has no inherent state — it simply reads and writes accounts using the `AccountKeeper` from the `auth` module.
cwgoes marked this conversation as resolved.
Show resolved Hide resolved

This implementation choice is intended to minimize necessary state reads/writes, since we expect most transactions to involve coin amounts (for fees), so storing coin data in the account saves reading it separately.
26 changes: 26 additions & 0 deletions docs/spec/bank/transactions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
## Transactions

### MsgSend

```golang
type MsgSend struct {
Inputs []Input
Outputs []Output
}
```

`handleMsgSend` just runs `inputOutputCoins`.

```
handleMsgSend(msg MsgSend)
inputSum = 0
for input in inputs
inputSum += input.Amount
outputSum = 0
for output in outputs
outputSum += output.Amount
if inputSum != outputSum:
fail with "input/output amount mismatch"

return inputOutputCoins(msg.Inputs, msg.Outputs)
```
18 changes: 9 additions & 9 deletions x/bank/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ type Keeper interface {
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.Tags, sdk.Error)
AddCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) (sdk.Coins, sdk.Tags, sdk.Error)
InputOutputCoins(ctx sdk.Context, inputs []Input, outputs []Output) (sdk.Tags, sdk.Error)
}

// BaseKeeper manages transfers between accounts. It implements the Keeper
Expand Down Expand Up @@ -67,6 +68,14 @@ func (keeper BaseKeeper) AddCoins(
return addCoins(ctx, keeper.ak, addr, amt)
}

// InputOutputCoins handles a list of inputs and outputs
func (keeper BaseKeeper) InputOutputCoins(
ctx sdk.Context, inputs []Input, outputs []Output,
) (sdk.Tags, sdk.Error) {

return inputOutputCoins(ctx, keeper.ak, inputs, outputs)
}

//-----------------------------------------------------------------------------
// Send Keeper

Expand All @@ -76,7 +85,6 @@ type SendKeeper interface {
ViewKeeper

SendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) (sdk.Tags, sdk.Error)
InputOutputCoins(ctx sdk.Context, inputs []Input, outputs []Output) (sdk.Tags, sdk.Error)
}

var _ SendKeeper = (*BaseSendKeeper)(nil)
Expand Down Expand Up @@ -105,14 +113,6 @@ func (keeper BaseSendKeeper) SendCoins(
return sendCoins(ctx, keeper.ak, fromAddr, toAddr, amt)
}

// InputOutputCoins handles a list of inputs and outputs
func (keeper BaseSendKeeper) InputOutputCoins(
ctx sdk.Context, inputs []Input, outputs []Output,
) (sdk.Tags, sdk.Error) {

return inputOutputCoins(ctx, keeper.ak, inputs, outputs)
}

//-----------------------------------------------------------------------------
// View Keeper

Expand Down
22 changes: 0 additions & 22 deletions x/bank/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,6 @@ func TestSendKeeper(t *testing.T) {

addr := sdk.AccAddress([]byte("addr1"))
addr2 := sdk.AccAddress([]byte("addr2"))
addr3 := sdk.AccAddress([]byte("addr3"))
acc := accountKeeper.NewAccountWithAddress(ctx, addr)

// Test GetCoins/SetCoins
Expand Down Expand Up @@ -157,27 +156,6 @@ func TestSendKeeper(t *testing.T) {
require.True(t, sendKeeper.GetCoins(ctx, addr).IsEqual(sdk.Coins{sdk.NewInt64Coin("barcoin", 20), sdk.NewInt64Coin("foocoin", 5)}))
require.True(t, sendKeeper.GetCoins(ctx, addr2).IsEqual(sdk.Coins{sdk.NewInt64Coin("barcoin", 10), sdk.NewInt64Coin("foocoin", 10)}))

// Test InputOutputCoins
cwgoes marked this conversation as resolved.
Show resolved Hide resolved
input1 := NewInput(addr2, sdk.Coins{sdk.NewInt64Coin("foocoin", 2)})
output1 := NewOutput(addr, sdk.Coins{sdk.NewInt64Coin("foocoin", 2)})
sendKeeper.InputOutputCoins(ctx, []Input{input1}, []Output{output1})
require.True(t, sendKeeper.GetCoins(ctx, addr).IsEqual(sdk.Coins{sdk.NewInt64Coin("barcoin", 20), sdk.NewInt64Coin("foocoin", 7)}))
require.True(t, sendKeeper.GetCoins(ctx, addr2).IsEqual(sdk.Coins{sdk.NewInt64Coin("barcoin", 10), sdk.NewInt64Coin("foocoin", 8)}))

inputs := []Input{
NewInput(addr, sdk.Coins{sdk.NewInt64Coin("foocoin", 3)}),
NewInput(addr2, sdk.Coins{sdk.NewInt64Coin("barcoin", 3), sdk.NewInt64Coin("foocoin", 2)}),
}

outputs := []Output{
NewOutput(addr, sdk.Coins{sdk.NewInt64Coin("barcoin", 1)}),
NewOutput(addr3, sdk.Coins{sdk.NewInt64Coin("barcoin", 2), sdk.NewInt64Coin("foocoin", 5)}),
}
sendKeeper.InputOutputCoins(ctx, inputs, outputs)
require.True(t, sendKeeper.GetCoins(ctx, addr).IsEqual(sdk.Coins{sdk.NewInt64Coin("barcoin", 21), sdk.NewInt64Coin("foocoin", 4)}))
require.True(t, sendKeeper.GetCoins(ctx, addr2).IsEqual(sdk.Coins{sdk.NewInt64Coin("barcoin", 7), sdk.NewInt64Coin("foocoin", 6)}))
require.True(t, sendKeeper.GetCoins(ctx, addr3).IsEqual(sdk.Coins{sdk.NewInt64Coin("barcoin", 2), sdk.NewInt64Coin("foocoin", 5)}))

}

func TestViewKeeper(t *testing.T) {
Expand Down
5 changes: 4 additions & 1 deletion x/bank/msgs.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
)

// name to identify transaction routes
const MsgRoute = "bank"

// MsgSend - high level transaction of the coin module
type MsgSend struct {
Inputs []Input `json:"inputs"`
Expand All @@ -21,7 +24,7 @@ func NewMsgSend(in []Input, out []Output) MsgSend {

// Implements Msg.
// nolint
func (msg MsgSend) Route() string { return "bank" } // TODO: "bank/send"
func (msg MsgSend) Route() string { return MsgRoute }
func (msg MsgSend) Type() string { return "send" }
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved

// Implements Msg.
Expand Down