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/v2): Introduce global send restriction #21925

Merged
merged 4 commits into from
Oct 2, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
6 changes: 6 additions & 0 deletions x/bank/proto/cosmos/bank/module/v2/module.proto
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,10 @@ message Module {

// authority defines the custom module authority. If not set, defaults to the governance module.
string authority = 1;

// restrictions_order specifies the order of send restrictions and should be
// a list of module names which provide a send restriction instance. If no
// order is provided, then restrictions will be applied in alphabetical order
// of module names.
repeated string restrictions_order = 2;
}
42 changes: 42 additions & 0 deletions x/bank/v2/depinject.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
package bankv2

import (
"fmt"
"maps"
"slices"
"sort"

"cosmossdk.io/core/address"
"cosmossdk.io/core/appmodule"
"cosmossdk.io/depinject"
Expand All @@ -22,6 +27,7 @@ func init() {
appconfig.RegisterModule(
&moduletypes.Module{},
appconfig.Provide(ProvideModule),
appconfig.Invoke(InvokeSetSendRestrictions),
)
}

Expand Down Expand Up @@ -61,3 +67,39 @@ func ProvideModule(in ModuleInputs) ModuleOutputs {
Module: m,
}
}

func InvokeSetSendRestrictions(
config *moduletypes.Module,
keeper keeper.Keeper,
restrictions map[string]types.SendRestrictionFn,
) error {
if config == nil {
return nil
}

modules := slices.Collect(maps.Keys(restrictions))
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove unnecessary use of slices.Collect

The function slices.Collect is not part of the standard slices package and may cause a compilation error. Since maps.Keys already returns a slice of keys, you can simplify the code by removing slices.Collect.

Apply this fix to eliminate the undefined function:

-modules := slices.Collect(maps.Keys(restrictions))
+modules := maps.Keys(restrictions)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
modules := slices.Collect(maps.Keys(restrictions))
modules := maps.Keys(restrictions)

order := config.RestrictionsOrder
if len(order) == 0 {
order = modules
sort.Strings(order)
}

if len(order) != len(modules) {
return fmt.Errorf("len(restrictions order: %v) != len(restriction modules: %v)", order, modules)
}

if len(modules) == 0 {
return nil
}

for _, module := range order {
restriction, ok := restrictions[module]
if !ok {
return fmt.Errorf("can't find send restriction for module %s", module)
}

keeper.AppendSendRestriction(restriction)
}

return nil
}
35 changes: 28 additions & 7 deletions x/bank/v2/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,21 @@ type Keeper struct {
params collections.Item[types.Params]
balances *collections.IndexedMap[collections.Pair[[]byte, string], math.Int, BalancesIndexes]
supply collections.Map[string, math.Int]

sendRestriction *sendRestriction
}

func NewKeeper(authority []byte, addressCodec address.Codec, env appmodulev2.Environment, cdc codec.BinaryCodec) *Keeper {
sb := collections.NewSchemaBuilder(env.KVStoreService)

k := &Keeper{
Environment: env,
authority: authority,
addressCodec: addressCodec, // TODO(@julienrbrt): Should we add address codec to the environment?
params: collections.NewItem(sb, types.ParamsKey, "params", codec.CollValue[types.Params](cdc)),
balances: collections.NewIndexedMap(sb, types.BalancesPrefix, "balances", collections.PairKeyCodec(collections.BytesKey, collections.StringKey), sdk.IntValue, newBalancesIndexes(sb)),
supply: collections.NewMap(sb, types.SupplyKey, "supply", collections.StringKey, sdk.IntValue),
Environment: env,
authority: authority,
addressCodec: addressCodec, // TODO(@julienrbrt): Should we add address codec to the environment?
params: collections.NewItem(sb, types.ParamsKey, "params", codec.CollValue[types.Params](cdc)),
balances: collections.NewIndexedMap(sb, types.BalancesPrefix, "balances", collections.PairKeyCodec(collections.BytesKey, collections.StringKey), sdk.IntValue, newBalancesIndexes(sb)),
supply: collections.NewMap(sb, types.SupplyKey, "supply", collections.StringKey, sdk.IntValue),
sendRestriction: newSendRestriction(),
}

schema, err := sb.Build()
Expand Down Expand Up @@ -94,7 +97,10 @@ func (k Keeper) SendCoins(ctx context.Context, from, to []byte, amt sdk.Coins) e
}

var err error
// TODO: Send restriction
to, err = k.sendRestriction.apply(ctx, from, to, amt)
if err != nil {
return err
}

err = k.subUnlockedCoins(ctx, from, amt)
if err != nil {
Expand Down Expand Up @@ -252,3 +258,18 @@ func newBalancesIndexes(sb *collections.SchemaBuilder) BalancesIndexes {
type BalancesIndexes struct {
Denom *indexes.ReversePair[[]byte, string, math.Int]
}

// AppendSendRestriction adds the provided SendRestrictionFn to run after previously provided restrictions.
func (k Keeper) AppendSendRestriction(restriction types.SendRestrictionFn) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: can be move this in another file, and call the API Global?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it should relay in keeper I think or we have to make keeper.sendRestriction public

Copy link
Member

Choose a reason for hiding this comment

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

That's fine, I meant renaming to AppendGlobalSendRestriction and maybe put all sendrestrictions logic ousitde of keeper.go but still in the keeper

k.sendRestriction.append(restriction)
}

// PrependSendRestriction adds the provided SendRestrictionFn to run before previously provided restrictions.
func (k Keeper) PrependSendRestriction(restriction types.SendRestrictionFn) {
k.sendRestriction.prepend(restriction)
}

// ClearSendRestriction removes the send restriction (if there is one).
func (k Keeper) ClearSendRestriction() {
k.sendRestriction.clear()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use pointer receivers for methods that modify Keeper

The methods AppendSendRestriction, PrependSendRestriction, and ClearSendRestriction modify the Keeper by altering its sendRestriction field. According to Go best practices and the Uber Go Style Guide, methods that modify the receiver should have pointer receivers to ensure changes affect the original instance.

Apply this diff to change the receiver to a pointer:

-func (k Keeper) AppendSendRestriction(restriction types.SendRestrictionFn) {
+func (k *Keeper) AppendSendRestriction(restriction types.SendRestrictionFn) {
}
-func (k Keeper) PrependSendRestriction(restriction types.SendRestrictionFn) {
+func (k *Keeper) PrependSendRestriction(restriction types.SendRestrictionFn) {
}
-func (k Keeper) ClearSendRestriction() {
+func (k *Keeper) ClearSendRestriction() {
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (k Keeper) AppendSendRestriction(restriction types.SendRestrictionFn) {
k.sendRestriction.append(restriction)
}
// PrependSendRestriction adds the provided SendRestrictionFn to run before previously provided restrictions.
func (k Keeper) PrependSendRestriction(restriction types.SendRestrictionFn) {
k.sendRestriction.prepend(restriction)
}
// ClearSendRestriction removes the send restriction (if there is one).
func (k Keeper) ClearSendRestriction() {
k.sendRestriction.clear()
}
func (k *Keeper) AppendSendRestriction(restriction types.SendRestrictionFn) {
k.sendRestriction.append(restriction)
}
// PrependSendRestriction adds the provided SendRestrictionFn to run before previously provided restrictions.
func (k *Keeper) PrependSendRestriction(restriction types.SendRestrictionFn) {
k.sendRestriction.prepend(restriction)
}
// ClearSendRestriction removes the send restriction (if there is one).
func (k *Keeper) ClearSendRestriction() {
k.sendRestriction.clear()
}

52 changes: 52 additions & 0 deletions x/bank/v2/keeper/keeper_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package keeper_test

import (
"bytes"
"context"
"fmt"
"testing"
"time"

Expand Down Expand Up @@ -184,3 +186,53 @@ func (suite *KeeperTestSuite) TestSendCoins_Module_To_Module() {
mintBarBalance := suite.bankKeeper.GetBalance(ctx, mintAcc.GetAddress(), barDenom)
require.Equal(mintBarBalance.Amount, math.NewInt(0))
}

func (suite *KeeperTestSuite) TestSendCoins_WithRestriction() {
ctx := suite.ctx
require := suite.Require()
balances := sdk.NewCoins(newFooCoin(100), newBarCoin(50))
sendAmt := sdk.NewCoins(newFooCoin(10), newBarCoin(10))

require.NoError(banktestutil.FundAccount(ctx, suite.bankKeeper, accAddrs[0], balances))

// Add first restriction
addrRestrictFunc := func(ctx context.Context, from, to []byte, amount sdk.Coins) ([]byte, error) {
if bytes.Equal(from, to) {
return nil, fmt.Errorf("Can not send to same address")
}
return to, nil
}
Comment on lines +199 to +204
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Error message should start with lowercase and use "cannot"

Per the Uber Go Style Guide, error messages should start with a lowercase letter, and "cannot" should be a single word. Please update the error message accordingly.

Apply this diff to correct the error message:

-			return nil, fmt.Errorf("Can not send to same address")
+			return nil, fmt.Errorf("cannot send to same address")

Also, update the test assertion to match the new error message:

-	require.Contains(err.Error(), "Can not send to same address")
+	require.Contains(err.Error(), "cannot send to same address")

Committable suggestion was skipped due to low confidence.

suite.bankKeeper.AppendSendRestriction(addrRestrictFunc)

err := suite.bankKeeper.SendCoins(ctx, accAddrs[0], accAddrs[0], sendAmt)
require.Error(err)
require.Contains(err.Error(), "Can not send to same address")

// Add second restriction
amtRestrictFunc := func(ctx context.Context, from, to []byte, amount sdk.Coins) ([]byte, error) {
if len(amount) > 1 {
return nil, fmt.Errorf("Allow only one denom per one send")
}
return to, nil
}
Comment on lines +213 to +217
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Error message should start with lowercase and improve wording

According to the Uber Go Style Guide, error messages should start with a lowercase letter. Additionally, consider rephrasing the message for clarity.

Apply this diff to improve the error message:

-			return nil, fmt.Errorf("Allow only one denom per one send")
+			return nil, fmt.Errorf("allow only one denom per send")

Also, update the test assertion to match the new error message:

-	require.Contains(err.Error(), "Allow only one denom per one send")
+	require.Contains(err.Error(), "allow only one denom per send")

Committable suggestion was skipped due to low confidence.

suite.bankKeeper.AppendSendRestriction(amtRestrictFunc)

// Pass the 1st but failt at the 2nd
err = suite.bankKeeper.SendCoins(ctx, accAddrs[0], accAddrs[1], sendAmt)
require.Error(err)
require.Contains(err.Error(), "Allow only one denom per one send")

// Pass both 2 restrictions
err = suite.bankKeeper.SendCoins(ctx, accAddrs[0], accAddrs[1], sdk.NewCoins(newFooCoin(10)))
require.NoError(err)

// Check balances
acc0FooBalance := suite.bankKeeper.GetBalance(ctx, accAddrs[0], fooDenom)
require.Equal(acc0FooBalance.Amount, math.NewInt(90))
acc0BarBalance := suite.bankKeeper.GetBalance(ctx, accAddrs[0], barDenom)
require.Equal(acc0BarBalance.Amount, math.NewInt(50))
acc1FooBalance := suite.bankKeeper.GetBalance(ctx, accAddrs[1], fooDenom)
require.Equal(acc1FooBalance.Amount, math.NewInt(10))
acc1BarBalance := suite.bankKeeper.GetBalance(ctx, accAddrs[1], barDenom)
require.Equal(acc1BarBalance.Amount, math.ZeroInt())
}
47 changes: 47 additions & 0 deletions x/bank/v2/keeper/restriction.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package keeper

import (
"context"

"cosmossdk.io/x/bank/v2/types"

sdk "github.com/cosmos/cosmos-sdk/types"
)

// sendRestriction is a struct that houses a SendRestrictionFn.
// It exists so that the SendRestrictionFn can be updated in the SendKeeper without needing to have a pointer receiver.
type sendRestriction struct {
fn types.SendRestrictionFn
}

// newSendRestriction creates a new sendRestriction with nil send restriction.
func newSendRestriction() *sendRestriction {
return &sendRestriction{
fn: nil,
}
}

// append adds the provided restriction to this, to be run after the existing function.
func (r *sendRestriction) append(restriction types.SendRestrictionFn) {
r.fn = r.fn.Then(restriction)
}

// prepend adds the provided restriction to this, to be run before the existing function.
func (r *sendRestriction) prepend(restriction types.SendRestrictionFn) {
r.fn = restriction.Then(r.fn)
}

// clear removes the send restriction (sets it to nil).
func (r *sendRestriction) clear() {
r.fn = nil
}

var _ types.SendRestrictionFn = (*sendRestriction)(nil).apply

// apply applies the send restriction if there is one. If not, it's a no-op.
func (r *sendRestriction) apply(ctx context.Context, fromAddr, toAddr []byte, amt sdk.Coins) ([]byte, error) {
if r == nil || r.fn == nil {
return toAddr, nil
}
return r.fn(ctx, fromAddr, toAddr, amt)
}
Comment on lines +41 to +47
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

LGTM: Correct implementation of apply method with proper error handling.

The apply method correctly implements the send restriction logic, handling cases where the receiver or the function is nil. It properly returns the original toAddr if no restriction is applied.

Consider a minor optimization to reduce nesting:

 func (r *sendRestriction) apply(ctx context.Context, fromAddr, toAddr []byte, amt sdk.Coins) ([]byte, error) {
-	if r == nil || r.fn == nil {
+	if r == nil || r.fn == nil {
 		return toAddr, nil
 	}
-	return r.fn(ctx, fromAddr, toAddr, amt)
+	return r.fn(ctx, fromAddr, toAddr, amt)
 }

This change maintains the same logic but reduces nesting, potentially improving readability.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// apply applies the send restriction if there is one. If not, it's a no-op.
func (r *sendRestriction) apply(ctx context.Context, fromAddr, toAddr []byte, amt sdk.Coins) ([]byte, error) {
if r == nil || r.fn == nil {
return toAddr, nil
}
return r.fn(ctx, fromAddr, toAddr, amt)
}
// apply applies the send restriction if there is one. If not, it's a no-op.
func (r *sendRestriction) apply(ctx context.Context, fromAddr, toAddr []byte, amt sdk.Coins) ([]byte, error) {
if r == nil || r.fn == nil {
return toAddr, nil
}
return r.fn(ctx, fromAddr, toAddr, amt)
}

77 changes: 69 additions & 8 deletions x/bank/v2/types/module/module.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading