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

feat(bank): Allow injectable restrictions on bank transfers #14224

Merged
merged 58 commits into from
Aug 18, 2023
Merged
Show file tree
Hide file tree
Changes from 57 commits
Commits
Show all changes
58 commits
Select commit Hold shift + click to select a range
83de950
[14124]: Define a SendRestriction function type.
SpicyLemon Dec 7, 2022
1648159
[14124]: Move the MintingRestrictionFn in with the SendRestrictionFn …
SpicyLemon Dec 7, 2022
66b2b93
[14124]: Change InputOutputCoins to accept only a single input since …
SpicyLemon Dec 8, 2022
845480d
[14124]: Add a SendRestrictionFn to the send keeper. Rename the exist…
SpicyLemon Dec 8, 2022
df54462
[14124]: Remove the SendCoinsWithoutRestriction and always run restri…
SpicyLemon Dec 8, 2022
ba605c3
[14124]: Create a struct to hold the send restriction function that c…
SpicyLemon Dec 8, 2022
66e8474
[14124]: Switch the keeper to use this SendRestriction holder and giv…
SpicyLemon Dec 8, 2022
bc988d9
[14124]: Add some more unit test cases on the SendCoins w/restrictions.
SpicyLemon Dec 8, 2022
3f6ea3a
[14124]: Add unit tests on InputOutputCoins with restrictions.
SpicyLemon Dec 8, 2022
a5b1afe
[14124]: Add unit tests on append and prepend send restrictions.
SpicyLemon Dec 8, 2022
64bb3c1
[14124]: Update the gov mocks to include the new Append and Prepend r…
SpicyLemon Dec 8, 2022
9bd62bf
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Dec 8, 2022
987e9c5
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Dec 8, 2022
e388801
[14124]: Update changelog.
SpicyLemon Dec 8, 2022
1ef3412
[14124]: Fix a couple test failure messages.
SpicyLemon Dec 8, 2022
60fbf14
[14124]: Fix some import ordering.
SpicyLemon Dec 8, 2022
915216e
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Dec 8, 2022
66ec36f
[14124]: Update spec doc with SendKeeper Append and Prepend SendRestr…
SpicyLemon Dec 9, 2022
1f0efb8
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Dec 9, 2022
c9cc904
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Jan 12, 2023
28f37ed
[14124]: Fix test compilation (due to AccountI change).
SpicyLemon Jan 12, 2023
a0befb3
[14124]: Lint fixes.
SpicyLemon Jan 12, 2023
327f218
[14124]: Add some extra info to the restriction composition function …
SpicyLemon Jan 12, 2023
f4d0a12
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Jan 12, 2023
6202a5d
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Feb 3, 2023
8aba61a
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Feb 16, 2023
a994e79
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Mar 8, 2023
916608b
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Mar 21, 2023
ed42c18
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Mar 22, 2023
4260317
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Mar 31, 2023
2a3f9fa
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Apr 14, 2023
c9b5bab
[14124]: Remove the API Breaking change listing since another PR did …
SpicyLemon Apr 17, 2023
c7583b4
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Apr 17, 2023
f54f8fa
[14124]: Move the definition of the restriction functions into the ty…
SpicyLemon Apr 18, 2023
c9b7b71
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Apr 18, 2023
c6e1dca
[14124]: Move the wrapper for the SendRestrictionFn back into the kee…
SpicyLemon Apr 19, 2023
053a518
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Apr 19, 2023
0000a6e
[14124]: Regen mocks to get the ClearSendRestriction.
SpicyLemon Apr 19, 2023
2f61c50
Fix broken reference to moved MintingRestrictionFn definition.
SpicyLemon Apr 19, 2023
231c925
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon May 5, 2023
dfb0f30
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon May 5, 2023
a452e6a
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Jun 5, 2023
1c009f5
[14124]: Fix compilation issue after merge.
SpicyLemon Jun 5, 2023
a0f7191
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Jun 5, 2023
c43f119
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Jun 9, 2023
114838d
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Jun 27, 2023
89680e8
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Jun 28, 2023
a116b9a
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Jun 29, 2023
0f36433
Move the send restriction application in SendCoins to after subUnlock…
SpicyLemon Jun 29, 2023
acbc96c
Add some spec documentation about the send restrictions.
SpicyLemon Jun 29, 2023
fdd3354
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Jun 30, 2023
7140caa
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Jun 30, 2023
d4eca5d
[14124]: Fix unit tests that broke when I changed the location of the…
SpicyLemon Jun 30, 2023
8fb5fd4
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Jul 31, 2023
7ab7b5a
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Aug 8, 2023
555523c
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Aug 10, 2023
adb1e73
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Aug 18, 2023
58b6f99
Merge branch 'main' into dwedul/14124-bank-restrictions
SpicyLemon Aug 18, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (client/rpc) [#17274](https://github.com/cosmos/cosmos-sdk/pull/17274) Add `QueryEventForTxCmd` cmd to subscribe and wait event for transaction by hash.
* (baseapp) [#16239](https://github.com/cosmos/cosmos-sdk/pull/16239) Add Gas Limits to allow node operators to resource bound queries.
* (baseapp) [#17393](https://github.com/cosmos/cosmos-sdk/pull/17394) Check BlockID Flag on Votes in `ValidateVoteExtensions`
* (x/bank) [#14224](https://github.com/cosmos/cosmos-sdk/pull/14224) Allow injection of restrictions on transfers using `AppendSendRestriction` or `PrependSendRestriction`.

### Improvements

Expand Down
88 changes: 86 additions & 2 deletions x/bank/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -236,8 +236,12 @@ accounts. The send keeper does not alter the total supply (mint or burn coins).
type SendKeeper interface {
ViewKeeper

InputOutputCoins(ctx context.Context, inputs types.Input, outputs []types.Output) error
SendCoins(ctx context.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) error
AppendSendRestriction(restriction SendRestrictionFn)
PrependSendRestriction(restriction SendRestrictionFn)
Comment on lines +239 to +240
Copy link
Contributor

Choose a reason for hiding this comment

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

This is late, and may have been discussed already, but I wonder what the need is for having 2 new methods, compared to just, say, AppendSendRestriction? More importantly, how can a module decide whether its SendRestrictionFn should be prepended vs appended? It seems like a global decision that can't be made locally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ordering of send restrictions might matter. I expect most things will use Append, but there could easily be a situation where a module needs theirs to go before the ones already injected.

I hate when I get into a situation where I just can't do what I know I need to do because the only layer I have access too didn't provide enough control. So, while Append will usually be good enough, Prepend and Clear can easily save the day.

Copy link
Contributor

Choose a reason for hiding this comment

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

could easily be a situation where a module needs theirs to go before the ones already injected.

But how does the module know whether it's safe to insert its restriction function before all others? That is, I see a problem where modules can be fighting each other for the correct position of their restrictions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The module can't know. The blockchain author can though, and needs to have the tools available to fix ordering problems if the arise.

ClearSendRestriction()
Copy link
Contributor

Choose a reason for hiding this comment

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

When is this needed? If you've added restrictions, do you ever want to remove them again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Clear() is needed for situations where a blockchain author needs to change the ordering of restrictions, and can't do so by other means.


InputOutputCoins(ctx context.Context, input types.Input, outputs []types.Output) error
SendCoins(ctx context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins) error

GetParams(ctx context.Context) types.Params
SetParams(ctx context.Context, params types.Params) error
Expand All @@ -256,6 +260,86 @@ type SendKeeper interface {
}
```

#### Send Restrictions

The `SendKeeper` applies a `SendRestrictionFn` before each transfer of funds.

```golang
// A SendRestrictionFn can restrict sends and/or provide a new receiver address.
type SendRestrictionFn func(ctx context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins) (newToAddr sdk.AccAddress, err error)
```

After the `SendKeeper` (or `BaseKeeper`) has been created, send restrictions can be added to it using the `AppendSendRestriction` or `PrependSendRestriction` functions.
Both functions compose the provided restriction with any previously provided restrictions.
`AppendSendRestriction` adds the provided restriction to be run after any previously provided send restrictions.
`PrependSendRestriction` adds the restriction to be run before any previously provided send restrictions.
The composition will short-circuit when an error is encountered. I.e. if the first one returns an error, the second is not run.

During `SendCoins`, the send restriction is applied after coins are removed from the from address, but before adding them to the to address.
During `InputOutputCoins`, the send restriction is applied after the input coins are removed and once for each output before the funds are added.

A send restriction function should make use of a custom value in the context to allow bypassing that specific restriction.

For example, in your module's keeper package, you'd define the send restriction function:

```golang
var _ banktypes.SendRestrictionFn = Keeper{}.SendRestrictionFn

func (k Keeper) SendRestrictionFn(ctx context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins) (sdk.AccAddress, error) {
// Bypass if the context says to.
if mymodule.HasBypass(ctx) {
return toAddr, nil
}

// Your custom send restriction logic goes here.
return nil, errors.New("not implemented")
}
```

The bank keeper should be provided to your keeper's constructor so the send restriction can be added to it:

```golang
func NewKeeper(cdc codec.BinaryCodec, storeKey storetypes.StoreKey, bankKeeper mymodule.BankKeeper) Keeper {
rv := Keeper{/*...*/}
bankKeeper.AppendSendRestriction(rv.SendRestrictionFn)
return rv
}
```

Then, in the `mymodule` package, define the context helpers:

```golang
const bypassKey = "bypass-mymodule-restriction"

// WithBypass returns a new context that will cause the mymodule bank send restriction to be skipped.
func WithBypass(ctx context.Context) context.Context {
return sdk.UnwrapSDKContext(ctx).WithValue(bypassKey, true)
}

// WithoutBypass returns a new context that will cause the mymodule bank send restriction to not be skipped.
func WithoutBypass(ctx context.Context) context.Context {
return sdk.UnwrapSDKContext(ctx).WithValue(bypassKey, false)
}

// HasBypass checks the context to see if the mymodule bank send restriction should be skipped.
func HasBypass(ctx context.Context) bool {
bypassValue := ctx.Value(bypassKey)
if bypassValue == nil {
return false
}
bypass, isBool := bypassValue.(bool)
return isBool && bypass
}
```

Now, anywhere where you want to use `SendCoins` or `InputOutputCoins`, but you don't want your send restriction applied:

```golang
func (k Keeper) DoThing(ctx context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins) error {
return k.bankKeeper.SendCoins(mymodule.WithBypass(ctx), fromAddr, toAddr, amt)
}
```

### ViewKeeper

The view keeper provides read-only access to account balances. The view keeper does not have balance alteration functionality. All balance lookups are `O(1)`.
Expand Down
14 changes: 14 additions & 0 deletions x/bank/keeper/export_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package keeper

import "github.com/cosmos/cosmos-sdk/x/bank/types"

// This file exists in the keeper package to expose some private things
// for the purpose of testing in the keeper_test package.

func (k BaseSendKeeper) SetSendRestriction(restriction types.SendRestrictionFn) {
k.sendRestriction.fn = restriction
}

func (k BaseSendKeeper) GetSendRestrictionFn() types.SendRestrictionFn {
return k.sendRestriction.fn
}
23 changes: 5 additions & 18 deletions x/bank/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ var _ Keeper = (*BaseKeeper)(nil)
// between accounts.
type Keeper interface {
SendKeeper
WithMintCoinsRestriction(MintingRestrictionFn) BaseKeeper
WithMintCoinsRestriction(types.MintingRestrictionFn) BaseKeeper

InitGenesis(context.Context, *types.GenesisState)
ExportGenesis(context.Context) *types.GenesisState
Expand Down Expand Up @@ -59,12 +59,10 @@ type BaseKeeper struct {
ak types.AccountKeeper
cdc codec.BinaryCodec
storeService store.KVStoreService
mintCoinsRestrictionFn MintingRestrictionFn
mintCoinsRestrictionFn types.MintingRestrictionFn
logger log.Logger
}

type MintingRestrictionFn func(ctx context.Context, coins sdk.Coins) error

// GetPaginatedTotalSupply queries for the supply, ignoring 0 coins, with a given pagination
func (k BaseKeeper) GetPaginatedTotalSupply(ctx context.Context, pagination *query.PageRequest) (sdk.Coins, *query.PageResponse, error) {
coins, pageResp, err := query.CollectionPaginate(ctx, k.Supply, pagination, func(key string, value math.Int) (sdk.Coin, error) {
Expand Down Expand Up @@ -103,7 +101,7 @@ func NewBaseKeeper(
ak: ak,
cdc: cdc,
storeService: storeService,
mintCoinsRestrictionFn: func(ctx context.Context, coins sdk.Coins) error { return nil },
mintCoinsRestrictionFn: types.NoOpMintingRestrictionFn,
logger: logger,
}
}
Expand All @@ -113,19 +111,8 @@ func NewBaseKeeper(
// Previous restriction functions can be nested as such:
//
// bankKeeper.WithMintCoinsRestriction(restriction1).WithMintCoinsRestriction(restriction2)
func (k BaseKeeper) WithMintCoinsRestriction(check MintingRestrictionFn) BaseKeeper {
oldRestrictionFn := k.mintCoinsRestrictionFn
k.mintCoinsRestrictionFn = func(ctx context.Context, coins sdk.Coins) error {
err := check(ctx, coins)
if err != nil {
return err
}
err = oldRestrictionFn(ctx, coins)
if err != nil {
return err
}
return nil
}
func (k BaseKeeper) WithMintCoinsRestriction(check types.MintingRestrictionFn) BaseKeeper {
k.mintCoinsRestrictionFn = check.Then(k.mintCoinsRestrictionFn)
return k
}

Expand Down
Loading
Loading