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 27 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/groups) [#14879](https://github.com/cosmos/cosmos-sdk/pull/14879) Add `Query/Groups` query to get all the groups.
* (x/genutil,cli) [#15147](https://github.com/cosmos/cosmos-sdk/pull/15147) Add `--initial-height` flag to cli init cmd to provide `genesis.json` with user defined initial block height
* (x/gov) [#15151](https://github.com/cosmos/cosmos-sdk/pull/15151) Add `burn_vote_quorum`, `burn_proposal_deposit_prevote` and `burn_vote_veto` params to allow applications to decide if they would like to burn deposits
* (x/bank) [#14224](https://github.com/cosmos/cosmos-sdk/pull/14224) Allow injection of restrictions on transfers using `AppendSendRestriction` or `PrependSendRestriction`.

### Improvements

Expand Down Expand Up @@ -135,6 +136,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/bank) [#14894](https://github.com/cosmos/cosmos-sdk/pull/14894) Allow a human readable denomination for coins when querying bank balances. Added a `ResolveDenom` parameter to `types.QueryAllBalancesRequest`.
* (crypto) [#15070](https://github.com/cosmos/cosmos-sdk/pull/15070) `GenerateFromPassword` and `Cost` from `bcrypt.go` now take a `uint32` instead of a `int` type.
* (client) [#15123](https://github.com/cosmos/cosmos-sdk/pull/15123) `NewFactoryCLI` now returns an error, in addition to the `Factory`.
* (x/bank) [#14224](https://github.com/cosmos/cosmos-sdk/pull/14224) `InputOutputCoins` now only allows for a single `Input`.

### Client Breaking Changes

Expand Down
26 changes: 9 additions & 17 deletions tests/integration/bank/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -426,14 +426,12 @@ func TestInputOutputNewAccount(t *testing.T) {
assert.Assert(t, f.accountKeeper.GetAccount(ctx, addr2) == nil)
assert.Assert(t, f.bankKeeper.GetAllBalances(ctx, addr2).Empty())

inputs := []types.Input{
{Address: addr1.String(), Coins: sdk.NewCoins(newFooCoin(30), newBarCoin(10))},
}
input := types.Input{Address: addr1.String(), Coins: sdk.NewCoins(newFooCoin(30), newBarCoin(10))}
outputs := []types.Output{
{Address: addr2.String(), Coins: sdk.NewCoins(newFooCoin(30), newBarCoin(10))},
}

assert.NilError(t, f.bankKeeper.InputOutputCoins(ctx, inputs, outputs))
assert.NilError(t, f.bankKeeper.InputOutputCoins(ctx, input, outputs))

expected := sdk.NewCoins(newFooCoin(30), newBarCoin(10))
acc2Balances := f.bankKeeper.GetAllBalances(ctx, addr2)
Expand All @@ -460,9 +458,7 @@ func TestInputOutputCoins(t *testing.T) {
acc3 := f.accountKeeper.NewAccountWithAddress(ctx, addr3)
f.accountKeeper.SetAccount(ctx, acc3)

input := []types.Input{
{Address: addr1.String(), Coins: sdk.NewCoins(newFooCoin(60), newBarCoin(20))},
}
input := types.Input{Address: addr1.String(), Coins: sdk.NewCoins(newFooCoin(60), newBarCoin(20))}
outputs := []types.Output{
{Address: addr2.String(), Coins: sdk.NewCoins(newFooCoin(30), newBarCoin(10))},
{Address: addr3.String(), Coins: sdk.NewCoins(newFooCoin(30), newBarCoin(10))},
Expand All @@ -473,11 +469,9 @@ func TestInputOutputCoins(t *testing.T) {

assert.NilError(t, banktestutil.FundAccount(f.bankKeeper, ctx, addr1, balances))

insufficientInput := []types.Input{
{
Address: addr1.String(),
Coins: sdk.NewCoins(newFooCoin(300), newBarCoin(100)),
},
insufficientInput := types.Input{
Address: addr1.String(),
Coins: sdk.NewCoins(newFooCoin(300), newBarCoin(100)),
}
insufficientOutputs := []types.Output{
{Address: addr2.String(), Coins: sdk.NewCoins(newFooCoin(300), newBarCoin(100))},
Expand Down Expand Up @@ -722,11 +716,9 @@ func TestMsgMultiSendEvents(t *testing.T) {
coins := sdk.NewCoins(sdk.NewInt64Coin(fooDenom, 50), sdk.NewInt64Coin(barDenom, 100))
newCoins := sdk.NewCoins(sdk.NewInt64Coin(fooDenom, 50))
newCoins2 := sdk.NewCoins(sdk.NewInt64Coin(barDenom, 100))
input := []types.Input{
{
Address: addr.String(),
Coins: coins,
},
input := types.Input{
Address: addr.String(),
Coins: coins,
}
outputs := []types.Output{
{Address: addr3.String(), Coins: newCoins},
Expand Down
3 changes: 3 additions & 0 deletions x/bank/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,9 @@ accounts. The send keeper does not alter the total supply (mint or burn coins).
type SendKeeper interface {
ViewKeeper

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.


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

Expand Down
12 changes: 12 additions & 0 deletions x/bank/keeper/export_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package keeper

// 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 SendRestrictionFn) {
k.sendRestriction.Fn = restriction
}

func (k BaseSendKeeper) GetSendRestrictionFn() SendRestrictionFn {
return k.sendRestriction.Fn
}
17 changes: 2 additions & 15 deletions x/bank/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,6 @@ type BaseKeeper struct {
mintCoinsRestrictionFn MintingRestrictionFn
}

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

// GetPaginatedTotalSupply queries for the supply, ignoring 0 coins, with a given pagination
func (k BaseKeeper) GetPaginatedTotalSupply(ctx sdk.Context, pagination *query.PageRequest) (sdk.Coins, *query.PageResponse, error) {
store := ctx.KVStore(k.storeKey)
Expand Down Expand Up @@ -112,7 +110,7 @@ func NewBaseKeeper(
ak: ak,
cdc: cdc,
storeKey: storeKey,
mintCoinsRestrictionFn: func(ctx sdk.Context, coins sdk.Coins) error { return nil },
mintCoinsRestrictionFn: NoOpMintingRestrictionFn,
}
}

Expand All @@ -122,18 +120,7 @@ func NewBaseKeeper(
//
// bankKeeper.WithMintCoinsRestriction(restriction1).WithMintCoinsRestriction(restriction2)
func (k BaseKeeper) WithMintCoinsRestriction(check MintingRestrictionFn) BaseKeeper {
oldRestrictionFn := k.mintCoinsRestrictionFn
k.mintCoinsRestrictionFn = func(ctx sdk.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
}
k.mintCoinsRestrictionFn = check.Then(k.mintCoinsRestrictionFn)
return k
}

Expand Down
Loading