From 52576f222feea679b794784874c5226dd99d441d Mon Sep 17 00:00:00 2001 From: Facundo Medica Date: Sun, 4 Jun 2023 12:16:50 +0200 Subject: [PATCH 01/14] progress --- x/circuit/keeper/keeper.go | 54 ++++++++++++++++++++++---------------- x/circuit/module.go | 15 ++++++----- 2 files changed, 39 insertions(+), 30 deletions(-) diff --git a/x/circuit/keeper/keeper.go b/x/circuit/keeper/keeper.go index 115c71f0774b..d495bbf9070f 100644 --- a/x/circuit/keeper/keeper.go +++ b/x/circuit/keeper/keeper.go @@ -1,17 +1,19 @@ package keeper import ( + context "context" + proto "github.com/cosmos/gogoproto/proto" "cosmossdk.io/core/address" + "cosmossdk.io/core/store" storetypes "cosmossdk.io/store/types" "cosmossdk.io/x/circuit/types" - sdk "github.com/cosmos/cosmos-sdk/types" ) // Keeper defines the circuit module's keeper. type Keeper struct { - storekey storetypes.StoreKey + storeService store.KVStoreService authority []byte @@ -19,14 +21,14 @@ type Keeper struct { } // NewKeeper constructs a new Circuit Keeper instance -func NewKeeper(storeKey storetypes.StoreKey, authority string, addressCodec address.Codec) Keeper { +func NewKeeper(storeService store.KVStoreService, authority string, addressCodec address.Codec) Keeper { auth, err := addressCodec.StringToBytes(authority) if err != nil { panic(err) } return Keeper{ - storekey: storeKey, + storeService: storeService, authority: auth, addressCodec: addressCodec, } @@ -36,11 +38,14 @@ func (k *Keeper) GetAuthority() []byte { return k.authority } -func (k *Keeper) GetPermissions(ctx sdk.Context, address []byte) (*types.Permissions, error) { - store := ctx.KVStore(k.storekey) +func (k *Keeper) GetPermissions(ctx context.Context, address []byte) (*types.Permissions, error) { + store := k.storeService.OpenKVStore(ctx) key := types.CreateAddressPrefix(address) - bz := store.Get(key) + bz, err := store.Get(key) + if err != nil { + return nil, err + } perms := &types.Permissions{} if err := proto.Unmarshal(bz, perms); err != nil { @@ -50,8 +55,8 @@ func (k *Keeper) GetPermissions(ctx sdk.Context, address []byte) (*types.Permiss return perms, nil } -func (k *Keeper) SetPermissions(ctx sdk.Context, address []byte, perms *types.Permissions) error { - store := ctx.KVStore(k.storekey) +func (k *Keeper) SetPermissions(ctx context.Context, address []byte, perms *types.Permissions) error { + store := k.storeService.OpenKVStore(ctx) bz, err := proto.Marshal(perms) if err != nil { @@ -59,27 +64,30 @@ func (k *Keeper) SetPermissions(ctx sdk.Context, address []byte, perms *types.Pe } key := types.CreateAddressPrefix(address) - store.Set(key, bz) - - return nil + return store.Set(key, bz) } -func (k *Keeper) IsAllowed(ctx sdk.Context, msgURL string) bool { - store := ctx.KVStore(k.storekey) - return !store.Has(types.CreateDisableMsgPrefix(msgURL)) +func (k *Keeper) IsAllowed(ctx context.Context, msgURL string) (bool, error) { + store := k.storeService.OpenKVStore(ctx) + has, err := store.Has(types.CreateDisableMsgPrefix(msgURL)) + return !has, err } -func (k *Keeper) DisableMsg(ctx sdk.Context, msgURL string) { - ctx.KVStore(k.storekey).Set(types.CreateDisableMsgPrefix(msgURL), []byte{}) +func (k *Keeper) DisableMsg(ctx context.Context, msgURL string) error { + return k.storeService.OpenKVStore(ctx).Set(types.CreateDisableMsgPrefix(msgURL), []byte{}) } -func (k *Keeper) EnableMsg(ctx sdk.Context, msgURL string) { - ctx.KVStore(k.storekey).Delete(types.CreateDisableMsgPrefix(msgURL)) +func (k *Keeper) EnableMsg(ctx context.Context, msgURL string) error { + return k.storeService.OpenKVStore(ctx).Delete(types.CreateDisableMsgPrefix(msgURL)) } -func (k *Keeper) IteratePermissions(ctx sdk.Context, cb func(address []byte, perms types.Permissions) (stop bool)) { - store := ctx.KVStore(k.storekey) - iter := storetypes.KVStorePrefixIterator(store, types.AccountPermissionPrefix) +func (k *Keeper) IteratePermissions(ctx context.Context, cb func(address []byte, perms types.Permissions) (stop bool)) error { + store := k.storeService.OpenKVStore(ctx) + iter, err := store.Iterator(types.AccountPermissionPrefix, storetypes.PrefixEndBytes(types.AccountPermissionPrefix)) + if err != nil { + return err + } + defer func(iter storetypes.Iterator) { err := iter.Close() if err != nil { @@ -100,7 +108,7 @@ func (k *Keeper) IteratePermissions(ctx sdk.Context, cb func(address []byte, per } } -func (k *Keeper) IterateDisableLists(ctx sdk.Context, cb func(url []byte, perms types.Permissions) (stop bool)) { +func (k *Keeper) IterateDisableLists(ctx context.Context, cb func(url []byte, perms types.Permissions) (stop bool)) { store := ctx.KVStore(k.storekey) iter := storetypes.KVStorePrefixIterator(store, types.AccountPermissionPrefix) diff --git a/x/circuit/module.go b/x/circuit/module.go index 3b95da6045dc..4ff8732335b7 100644 --- a/x/circuit/module.go +++ b/x/circuit/module.go @@ -9,10 +9,13 @@ import ( modulev1 "cosmossdk.io/api/cosmos/circuit/module/v1" "cosmossdk.io/core/address" "cosmossdk.io/core/appmodule" + "cosmossdk.io/core/store" "cosmossdk.io/depinject" - store "cosmossdk.io/store/types" "cosmossdk.io/x/circuit/client/cli" abci "github.com/cometbft/cometbft/abci/types" + gwruntime "github.com/grpc-ecosystem/grpc-gateway/runtime" + "github.com/spf13/cobra" + "github.com/cosmos/cosmos-sdk/baseapp" "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/codec" @@ -22,8 +25,6 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/module" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" - gwruntime "github.com/grpc-ecosystem/grpc-gateway/runtime" - "github.com/spf13/cobra" "cosmossdk.io/x/circuit/keeper" "cosmossdk.io/x/circuit/types" @@ -149,9 +150,9 @@ func init() { type ModuleInputs struct { depinject.In - Config *modulev1.Module - Cdc codec.Codec - Key *store.KVStoreKey + Config *modulev1.Module + Cdc codec.Codec + StoreService store.KVStoreService AddressCodec address.Codec } @@ -172,7 +173,7 @@ func ProvideModule(in ModuleInputs) ModuleOutputs { } circuitkeeper := keeper.NewKeeper( - in.Key, + in.StoreService, authority.String(), in.AddressCodec, ) From 8e1a12de7d9b84ecc797495f7210d61c8e8dad2f Mon Sep 17 00:00:00 2001 From: Facundo Medica Date: Sun, 4 Jun 2023 14:48:53 +0200 Subject: [PATCH 02/14] progress --- baseapp/circuit.go | 6 +- baseapp/msg_service_router.go | 7 +- x/circuit/ante/circuit.go | 7 +- x/circuit/ante/circuit_test.go | 12 ++- x/circuit/keeper/genesis.go | 27 +++-- x/circuit/keeper/keeper.go | 120 +++++++--------------- x/circuit/keeper/keeper_test.go | 87 ++++++++++------ x/circuit/keeper/msg_server.go | 82 +++++++++------ x/circuit/keeper/msg_server_test.go | 149 +++++++++++++++------------- x/circuit/keeper/query.go | 23 ++--- x/circuit/keeper/query_test.go | 42 ++++---- x/circuit/keeper/test_utils.go | 47 --------- x/circuit/module.go | 1 + x/circuit/types/keys.go | 20 +--- 14 files changed, 297 insertions(+), 333 deletions(-) delete mode 100644 x/circuit/keeper/test_utils.go diff --git a/baseapp/circuit.go b/baseapp/circuit.go index 022ee6632c2e..3db0bc1bdcda 100644 --- a/baseapp/circuit.go +++ b/baseapp/circuit.go @@ -1,10 +1,8 @@ package baseapp -import ( - sdk "github.com/cosmos/cosmos-sdk/types" -) +import "context" // CircuitBreaker is an interface that defines the methods for a circuit breaker. type CircuitBreaker interface { - IsAllowed(ctx sdk.Context, typeURL string) bool + IsAllowed(ctx context.Context, typeURL string) (bool, error) } diff --git a/baseapp/msg_service_router.go b/baseapp/msg_service_router.go index d0031180f00b..b3876075af98 100644 --- a/baseapp/msg_service_router.go +++ b/baseapp/msg_service_router.go @@ -135,7 +135,12 @@ func (msr *MsgServiceRouter) RegisterService(sd *grpc.ServiceDesc, handler inter if msr.circuitBreaker != nil { msgURL := sdk.MsgTypeURL(msg) - if !msr.circuitBreaker.IsAllowed(ctx, msgURL) { + isAllowed, err := msr.circuitBreaker.IsAllowed(ctx, msgURL) + if err != nil { + return nil, err + } + + if !isAllowed { return nil, fmt.Errorf("circuit breaker disables execution of this message: %s", msgURL) } } diff --git a/x/circuit/ante/circuit.go b/x/circuit/ante/circuit.go index e33c5d2d6f55..72b5f6908357 100644 --- a/x/circuit/ante/circuit.go +++ b/x/circuit/ante/circuit.go @@ -1,13 +1,16 @@ package ante import ( + "context" + "github.com/cockroachdb/errors" + sdk "github.com/cosmos/cosmos-sdk/types" ) // CircuitBreaker is an interface that defines the methods for a circuit breaker. type CircuitBreaker interface { - IsAllowed(ctx sdk.Context, typeURL string) bool + IsAllowed(ctx context.Context, typeURL string) bool } // CircuitBreakerDecorator is an AnteDecorator that checks if the transaction type is allowed to enter the mempool or be executed @@ -21,7 +24,7 @@ func NewCircuitBreakerDecorator(ck CircuitBreaker) CircuitBreakerDecorator { } } -func (cbd CircuitBreakerDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { +func (cbd CircuitBreakerDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (context.Context, error) { // loop through all the messages and check if the message type is allowed for _, msg := range tx.GetMsgs() { if !cbd.circuitKeeper.IsAllowed(ctx, sdk.MsgTypeURL(msg)) { diff --git a/x/circuit/ante/circuit_test.go b/x/circuit/ante/circuit_test.go index 659060a6b088..a305ca17cbe1 100644 --- a/x/circuit/ante/circuit_test.go +++ b/x/circuit/ante/circuit_test.go @@ -1,12 +1,14 @@ package ante_test import ( + "context" "testing" storetypes "cosmossdk.io/store/types" cbtypes "cosmossdk.io/x/circuit/types" abci "github.com/cometbft/cometbft/abci/types" cmproto "github.com/cometbft/cometbft/proto/tendermint/types" + "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/testutil" clitestutil "github.com/cosmos/cosmos-sdk/testutil/cli" @@ -16,12 +18,13 @@ import ( "github.com/cosmos/cosmos-sdk/x/bank" "cosmossdk.io/x/circuit/ante" - sdk "github.com/cosmos/cosmos-sdk/types" "github.com/stretchr/testify/require" + + sdk "github.com/cosmos/cosmos-sdk/types" ) type fixture struct { - ctx sdk.Context + ctx context.Context mockStoreKey storetypes.StoreKey mockMsgURL string mockclientCtx client.Context @@ -32,7 +35,7 @@ type MockCircuitBreaker struct { isAllowed bool } -func (m MockCircuitBreaker) IsAllowed(ctx sdk.Context, typeURL string) bool { +func (m MockCircuitBreaker) IsAllowed(ctx context.Context, typeURL string) bool { return typeURL == "/cosmos.circuit.v1.MsgAuthorizeCircuitBreaker" } @@ -78,7 +81,8 @@ func TestCircuitBreakerDecorator(t *testing.T) { f.txBuilder.SetMsgs(tc.msg) tx := f.txBuilder.GetTx() - _, err := decorator.AnteHandle(f.ctx, tx, false, func(ctx sdk.Context, tx sdk.Tx, simulate bool) (newCtx sdk.Context, err error) { + sdkCtx := sdk.UnwrapSDKContext(f.ctx) + _, err := decorator.AnteHandle(sdkCtx, tx, false, func(ctx sdk.Context, tx sdk.Tx, simulate bool) (newCtx sdk.Context, err error) { return ctx, nil }) diff --git a/x/circuit/keeper/genesis.go b/x/circuit/keeper/genesis.go index bb8b2b2930b3..8c1db8670ce6 100644 --- a/x/circuit/keeper/genesis.go +++ b/x/circuit/keeper/genesis.go @@ -1,20 +1,21 @@ package keeper import ( + context "context" + "cosmossdk.io/x/circuit/types" - sdk "github.com/cosmos/cosmos-sdk/types" ) -func (k *Keeper) ExportGenesis(ctx sdk.Context) (data *types.GenesisState) { +func (k *Keeper) ExportGenesis(ctx context.Context) (data *types.GenesisState) { var ( permissions []*types.GenesisAccountPermissions disabledMsgs []string ) - k.IteratePermissions(ctx, func(address []byte, perm types.Permissions) (stop bool) { + err := k.Permissions.Walk(ctx, nil, func(address []byte, perm types.Permissions) (stop bool, err error) { add, err := k.addressCodec.BytesToString(address) if err != nil { - panic(err) + return true, err } // Convert the Permissions struct to a GenesisAccountPermissions struct // and add it to the permissions slice @@ -22,13 +23,19 @@ func (k *Keeper) ExportGenesis(ctx sdk.Context) (data *types.GenesisState) { Address: add, Permissions: &perm, }) - return false + return false, nil }) + if err != nil { + panic(err) + } - k.IterateDisableLists(ctx, func(address []byte, perm types.Permissions) (stop bool) { - disabledMsgs = append(disabledMsgs, perm.LimitTypeUrls...) - return false + err = k.DisableList.Walk(ctx, nil, func(msgUrl string) (stop bool, err error) { + disabledMsgs = append(disabledMsgs, msgUrl) + return false, nil }) + if err != nil { + panic(err) + } return &types.GenesisState{ AccountPermissions: permissions, @@ -37,7 +44,7 @@ func (k *Keeper) ExportGenesis(ctx sdk.Context) (data *types.GenesisState) { } // InitGenesis initializes the circuit module's state from a given genesis state. -func (k *Keeper) InitGenesis(ctx sdk.Context, genState *types.GenesisState) { +func (k *Keeper) InitGenesis(ctx context.Context, genState *types.GenesisState) { for _, accounts := range genState.AccountPermissions { add, err := k.addressCodec.StringToBytes(accounts.Address) if err != nil { @@ -45,7 +52,7 @@ func (k *Keeper) InitGenesis(ctx sdk.Context, genState *types.GenesisState) { } // Set the permissions for the account - if err := k.SetPermissions(ctx, add, accounts.Permissions); err != nil { + if err := k.Permissions.Set(ctx, add, *accounts.Permissions); err != nil { panic(err) } } diff --git a/x/circuit/keeper/keeper.go b/x/circuit/keeper/keeper.go index d495bbf9070f..79aebabca2ae 100644 --- a/x/circuit/keeper/keeper.go +++ b/x/circuit/keeper/keeper.go @@ -3,131 +3,79 @@ package keeper import ( context "context" - proto "github.com/cosmos/gogoproto/proto" + "github.com/cosmos/cosmos-sdk/codec" + "cosmossdk.io/collections" "cosmossdk.io/core/address" "cosmossdk.io/core/store" - storetypes "cosmossdk.io/store/types" "cosmossdk.io/x/circuit/types" ) // Keeper defines the circuit module's keeper. type Keeper struct { + cdc codec.BinaryCodec storeService store.KVStoreService authority []byte addressCodec address.Codec + + Schema collections.Schema + Permissions collections.Map[[]byte, types.Permissions] + DisableList collections.KeySet[string] } // NewKeeper constructs a new Circuit Keeper instance -func NewKeeper(storeService store.KVStoreService, authority string, addressCodec address.Codec) Keeper { +func NewKeeper(cdc codec.BinaryCodec, storeService store.KVStoreService, authority string, addressCodec address.Codec) Keeper { auth, err := addressCodec.StringToBytes(authority) if err != nil { panic(err) } - return Keeper{ + sb := collections.NewSchemaBuilder(storeService) + + k := Keeper{ + cdc: cdc, storeService: storeService, authority: auth, addressCodec: addressCodec, + Permissions: collections.NewMap( + sb, + types.AccountPermissionPrefix, + "permissions", + collections.BytesKey, + codec.CollValue[types.Permissions](cdc), + ), + DisableList: collections.NewKeySet( + sb, + types.DisableListPrefix, + "disable_list", + collections.StringKey, + ), } -} - -func (k *Keeper) GetAuthority() []byte { - return k.authority -} -func (k *Keeper) GetPermissions(ctx context.Context, address []byte) (*types.Permissions, error) { - store := k.storeService.OpenKVStore(ctx) - - key := types.CreateAddressPrefix(address) - bz, err := store.Get(key) + schema, err := sb.Build() if err != nil { - return nil, err - } - - perms := &types.Permissions{} - if err := proto.Unmarshal(bz, perms); err != nil { - return &types.Permissions{}, err + panic(err) } + k.Schema = schema - return perms, nil + return k } -func (k *Keeper) SetPermissions(ctx context.Context, address []byte, perms *types.Permissions) error { - store := k.storeService.OpenKVStore(ctx) - - bz, err := proto.Marshal(perms) - if err != nil { - return err - } - - key := types.CreateAddressPrefix(address) - return store.Set(key, bz) +func (k *Keeper) GetAuthority() []byte { + return k.authority } func (k *Keeper) IsAllowed(ctx context.Context, msgURL string) (bool, error) { - store := k.storeService.OpenKVStore(ctx) - has, err := store.Has(types.CreateDisableMsgPrefix(msgURL)) + has, err := k.DisableList.Has(ctx, msgURL) return !has, err } func (k *Keeper) DisableMsg(ctx context.Context, msgURL string) error { - return k.storeService.OpenKVStore(ctx).Set(types.CreateDisableMsgPrefix(msgURL), []byte{}) + return k.DisableList.Set(ctx, msgURL) } func (k *Keeper) EnableMsg(ctx context.Context, msgURL string) error { - return k.storeService.OpenKVStore(ctx).Delete(types.CreateDisableMsgPrefix(msgURL)) -} - -func (k *Keeper) IteratePermissions(ctx context.Context, cb func(address []byte, perms types.Permissions) (stop bool)) error { - store := k.storeService.OpenKVStore(ctx) - iter, err := store.Iterator(types.AccountPermissionPrefix, storetypes.PrefixEndBytes(types.AccountPermissionPrefix)) - if err != nil { - return err - } - - defer func(iter storetypes.Iterator) { - err := iter.Close() - if err != nil { - return - } - }(iter) - - for ; iter.Valid(); iter.Next() { - var perms types.Permissions - err := proto.Unmarshal(iter.Value(), &perms) - if err != nil { - panic(err) - } - - if cb(iter.Key()[len(types.AccountPermissionPrefix):], perms) { - break - } - } -} - -func (k *Keeper) IterateDisableLists(ctx context.Context, cb func(url []byte, perms types.Permissions) (stop bool)) { - store := ctx.KVStore(k.storekey) - - iter := storetypes.KVStorePrefixIterator(store, types.AccountPermissionPrefix) - defer func(iter storetypes.Iterator) { - err := iter.Close() - if err != nil { - return - } - }(iter) - - for ; iter.Valid(); iter.Next() { - var perms types.Permissions - err := proto.Unmarshal(iter.Value(), &perms) - if err != nil { - panic(err) - } - - if cb(iter.Key()[len(types.DisableListPrefix):], perms) { - break - } - } + return k.DisableList.Remove(ctx, msgURL) } diff --git a/x/circuit/keeper/keeper_test.go b/x/circuit/keeper/keeper_test.go index 3b22ff7f9fbd..1a8342b03fe9 100644 --- a/x/circuit/keeper/keeper_test.go +++ b/x/circuit/keeper/keeper_test.go @@ -2,32 +2,48 @@ package keeper_test import ( "bytes" + context "context" "testing" cmproto "github.com/cometbft/cometbft/proto/tendermint/types" "github.com/stretchr/testify/require" + "cosmossdk.io/core/address" storetypes "cosmossdk.io/store/types" + "cosmossdk.io/x/circuit" "cosmossdk.io/x/circuit/keeper" "cosmossdk.io/x/circuit/types" + addresscodec "github.com/cosmos/cosmos-sdk/codec/address" + "github.com/cosmos/cosmos-sdk/runtime" "github.com/cosmos/cosmos-sdk/testutil" - sdk "github.com/cosmos/cosmos-sdk/types" + moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" ) +var addresses = []string{ + "cosmos1zglwfu6xjzvzagqcmvzewyzjp9xwqw5qwrr8n9", + "cosmos1p8s0p6gqc6c9gt77lgr2qqujz49huhu6a80smx", + "cosmos1qasf9ehx8m7cnat39ndc74rx3fg7z66u8lw0fd", + "cosmos1uxrdj5zfuudhypsmmjxnj4gpu432ycht06a05a", + "cosmos1wn7k8a7fwpmrwnm94ndj0germfnxnhl6hs8spj", +} + type fixture struct { - ctx sdk.Context + ctx context.Context keeper keeper.Keeper mockAddr []byte mockPerms types.Permissions mockMsgURL string + ac address.Codec } func initFixture(t *testing.T) *fixture { + encCfg := moduletestutil.MakeTestEncodingConfig(circuit.AppModuleBasic{}) ac := addresscodec.NewBech32Codec("cosmos") mockStoreKey := storetypes.NewKVStoreKey("test") - k := keeper.NewKeeper(mockStoreKey, authtypes.NewModuleAddress("gov").String(), ac) + storeService := runtime.NewKVStoreService(mockStoreKey) + k := keeper.NewKeeper(encCfg.Codec, storeService, authtypes.NewModuleAddress("gov").String(), ac) bz, err := ac.StringToBytes(authtypes.NewModuleAddress("gov").String()) require.NoError(t, err) @@ -37,9 +53,11 @@ func initFixture(t *testing.T) *fixture { keeper: k, mockAddr: bz, mockPerms: types.Permissions{ - Level: 3, + Level: 3, + LimitTypeUrls: []string{"test"}, }, mockMsgURL: "mock_url", + ac: ac, } } @@ -55,15 +73,15 @@ func TestGetAndSetPermissions(t *testing.T) { f := initFixture(t) // Set the permissions for the mock address. - err := f.keeper.SetPermissions(f.ctx, f.mockAddr, &f.mockPerms) + err := f.keeper.Permissions.Set(f.ctx, f.mockAddr, f.mockPerms) require.NoError(t, err) // Retrieve the permissions for the mock address. - perms, err := f.keeper.GetPermissions(f.ctx, f.mockAddr) + perms, err := f.keeper.Permissions.Get(f.ctx, f.mockAddr) require.NoError(t, err) //// Assert that the retrieved permissions match the expected value. - require.Equal(t, &f.mockPerms, perms) + require.Equal(t, f.mockPerms, perms) } func TestIteratePermissions(t *testing.T) { @@ -83,17 +101,18 @@ func TestIteratePermissions(t *testing.T) { []byte("mock_address_3"), } for i, addr := range mockAddrs { - f.keeper.SetPermissions(f.ctx, addr, &mockPerms[i]) + f.keeper.Permissions.Set(f.ctx, addr, mockPerms[i]) } // Define a variable to store the returned permissions var returnedPerms []types.Permissions // Iterate through the permissions and append them to the returnedPerms slice - f.keeper.IteratePermissions(f.ctx, func(address []byte, perms types.Permissions) (stop bool) { + err := f.keeper.Permissions.Walk(f.ctx, nil, func(address []byte, perms types.Permissions) (stop bool, err error) { returnedPerms = append(returnedPerms, perms) - return false + return false, nil }) + require.NoError(t, err) // Assert that the returned permissions match the set mock permissions require.Equal(t, mockPerms, returnedPerms) @@ -103,33 +122,41 @@ func TestIterateDisabledList(t *testing.T) { t.Parallel() f := initFixture(t) - mockPerms := []types.Permissions{ - {Level: types.Permissions_LEVEL_SUPER_ADMIN, LimitTypeUrls: []string{"url1", "url2"}}, - {Level: types.Permissions_LEVEL_ALL_MSGS}, - {Level: types.Permissions_LEVEL_NONE_UNSPECIFIED}, + mockMsgs := []string{ + "mockUrl1", + "mockUrl2", + "mockUrl3", } - // Set the permissions for a set of mock addresses - mockAddrs := [][]byte{ - []byte("mock_address_1"), - []byte("mock_address_2"), - []byte("mock_address_3"), - } - - for i, addr := range mockAddrs { - f.keeper.SetPermissions(f.ctx, addr, &mockPerms[i]) + for _, url := range mockMsgs { + f.keeper.DisableMsg(f.ctx, url) } // Define a variable to store the returned disabled URLs - var returnedDisabled []types.Permissions + var returnedDisabled []string - f.keeper.IterateDisableLists(f.ctx, func(address []byte, perms types.Permissions) bool { - returnedDisabled = append(returnedDisabled, perms) - return false + err := f.keeper.DisableList.Walk(f.ctx, nil, func(msgUrl string) (bool, error) { + returnedDisabled = append(returnedDisabled, msgUrl) + return false, nil }) + require.NoError(t, err) // Assert that the returned disabled URLs match the set mock disabled URLs - require.Equal(t, mockPerms[0].LimitTypeUrls, returnedDisabled[0].LimitTypeUrls) - require.Equal(t, mockPerms[1].LimitTypeUrls, returnedDisabled[1].LimitTypeUrls) - require.Equal(t, mockPerms[2].LimitTypeUrls, returnedDisabled[2].LimitTypeUrls) + require.Equal(t, mockMsgs[0], returnedDisabled[0]) + require.Equal(t, mockMsgs[1], returnedDisabled[1]) + require.Equal(t, mockMsgs[2], returnedDisabled[2]) + + // re-enable mockMsgs[0] + f.keeper.EnableMsg(f.ctx, mockMsgs[0]) + returnedDisabled = []string{} + + err = f.keeper.DisableList.Walk(f.ctx, nil, func(msgUrl string) (bool, error) { + returnedDisabled = append(returnedDisabled, msgUrl) + return false, nil + }) + require.NoError(t, err) + + require.Len(t, returnedDisabled, 2) + require.Equal(t, mockMsgs[1], returnedDisabled[0]) + require.Equal(t, mockMsgs[2], returnedDisabled[1]) } diff --git a/x/circuit/keeper/msg_server.go b/x/circuit/keeper/msg_server.go index d2aeafaddfbf..f2585b4b9ca2 100644 --- a/x/circuit/keeper/msg_server.go +++ b/x/circuit/keeper/msg_server.go @@ -3,12 +3,15 @@ package keeper import ( "bytes" context "context" + "errors" fmt "fmt" "strings" + "cosmossdk.io/collections" errorsmod "cosmossdk.io/errors" "cosmossdk.io/x/circuit/types" + sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) @@ -25,9 +28,7 @@ func NewMsgServerImpl(keeper Keeper) types.MsgServer { return &msgServer{Keeper: keeper} } -func (srv msgServer) AuthorizeCircuitBreaker(goCtx context.Context, msg *types.MsgAuthorizeCircuitBreaker) (*types.MsgAuthorizeCircuitBreakerResponse, error) { - ctx := sdk.UnwrapSDKContext(goCtx) - +func (srv msgServer) AuthorizeCircuitBreaker(ctx context.Context, msg *types.MsgAuthorizeCircuitBreaker) (*types.MsgAuthorizeCircuitBreakerResponse, error) { address, err := srv.addressCodec.StringToBytes(msg.Granter) if err != nil { return nil, err @@ -36,7 +37,7 @@ func (srv msgServer) AuthorizeCircuitBreaker(goCtx context.Context, msg *types.M // if the granter is the module authority no need to check perms if !bytes.Equal(address, srv.GetAuthority()) { // Check that the authorizer has the permission level of "super admin" - perms, err := srv.GetPermissions(ctx, address) + perms, err := srv.Permissions.Get(ctx, address) if err != nil { return nil, fmt.Errorf("user permission does not exist %w", err) } @@ -51,12 +52,17 @@ func (srv msgServer) AuthorizeCircuitBreaker(goCtx context.Context, msg *types.M return nil, err } + if msg.Permissions == nil { + return nil, errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "permissions cannot be nil") + } + // Append the account in the msg to the store's set of authorized super admins - if err = srv.SetPermissions(ctx, grantee, msg.Permissions); err != nil { + if err = srv.Permissions.Set(ctx, grantee, *msg.Permissions); err != nil { return nil, err } - ctx.EventManager().EmitEvents(sdk.Events{ + sdkCtx := sdk.UnwrapSDKContext(ctx) + sdkCtx.EventManager().EmitEvents(sdk.Events{ sdk.NewEvent( "authorize_circuit_breaker", sdk.NewAttribute("granter", msg.Granter), @@ -70,40 +76,52 @@ func (srv msgServer) AuthorizeCircuitBreaker(goCtx context.Context, msg *types.M }, nil } -func (srv msgServer) TripCircuitBreaker(goCtx context.Context, msg *types.MsgTripCircuitBreaker) (*types.MsgTripCircuitBreakerResponse, error) { - ctx := sdk.UnwrapSDKContext(goCtx) - +func (srv msgServer) TripCircuitBreaker(ctx context.Context, msg *types.MsgTripCircuitBreaker) (*types.MsgTripCircuitBreakerResponse, error) { address, err := srv.addressCodec.StringToBytes(msg.Authority) if err != nil { return nil, err } // Check that the account has the permissions - perms, err := srv.GetPermissions(ctx, address) - if err != nil { - return nil, fmt.Errorf("user permission does not exist %w", err) + perms, err := srv.Permissions.Get(ctx, address) + if err != nil && !errors.Is(err, collections.ErrNotFound) { + return nil, err } - store := ctx.KVStore(srv.storekey) - switch { case perms.Level == types.Permissions_LEVEL_SUPER_ADMIN || perms.Level == types.Permissions_LEVEL_ALL_MSGS || bytes.Equal(address, srv.GetAuthority()): for _, msgTypeURL := range msg.MsgTypeUrls { // check if the message is in the list of allowed messages - if !srv.IsAllowed(ctx, msgTypeURL) { + isAllowed, err := srv.IsAllowed(ctx, msgTypeURL) + if err != nil { + return nil, err + } + + if !isAllowed { return nil, fmt.Errorf("message %s is already disabled", msgTypeURL) } - store.Set(types.CreateDisableMsgPrefix(msgTypeURL), []byte{0x01}) + + if err = srv.DisableList.Set(ctx, msgTypeURL); err != nil { + return nil, err + } + } case perms.Level == types.Permissions_LEVEL_SOME_MSGS: for _, msgTypeURL := range msg.MsgTypeUrls { // check if the message is in the list of allowed messages - if !srv.IsAllowed(ctx, msgTypeURL) { + isAllowed, err := srv.IsAllowed(ctx, msgTypeURL) + if err != nil { + return nil, err + } + + if !isAllowed { return nil, fmt.Errorf("message %s is already disabled", msgTypeURL) } for _, msgurl := range perms.LimitTypeUrls { if msgTypeURL == msgurl { - store.Set(types.CreateDisableMsgPrefix(msgTypeURL), []byte{0x01}) + if err = srv.DisableList.Set(ctx, msgTypeURL); err != nil { + return nil, err + } } else { return nil, errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "account does not have permission to trip circuit breaker for message %s", msgTypeURL) } @@ -118,7 +136,8 @@ func (srv msgServer) TripCircuitBreaker(goCtx context.Context, msg *types.MsgTri urls = strings.Join(msg.GetMsgTypeUrls(), ",") } - ctx.EventManager().EmitEvents(sdk.Events{ + sdkCtx := sdk.UnwrapSDKContext(ctx) + sdkCtx.EventManager().EmitEvents(sdk.Events{ sdk.NewEvent( "trip_circuit_breaker", sdk.NewAttribute("authority", msg.Authority), @@ -133,30 +152,34 @@ func (srv msgServer) TripCircuitBreaker(goCtx context.Context, msg *types.MsgTri // ResetCircuitBreaker resumes processing of Msg's in the state machine that // have been been paused using TripCircuitBreaker. -func (srv msgServer) ResetCircuitBreaker(goCtx context.Context, msg *types.MsgResetCircuitBreaker) (*types.MsgResetCircuitBreakerResponse, error) { - ctx := sdk.UnwrapSDKContext(goCtx) +func (srv msgServer) ResetCircuitBreaker(ctx context.Context, msg *types.MsgResetCircuitBreaker) (*types.MsgResetCircuitBreakerResponse, error) { keeper := srv.Keeper - address, err := srv.addressCodec.StringToBytes(msg.Authority) if err != nil { return nil, err } // Get the permissions for the account specified in the msg.Authority field - perms, err := keeper.GetPermissions(ctx, address) - if err != nil { + perms, err := keeper.Permissions.Get(ctx, address) + if err != nil && !errors.Is(err, collections.ErrNotFound) { return nil, fmt.Errorf("user permission does not exist %w", err) } - store := ctx.KVStore(srv.storekey) - if perms.Level == types.Permissions_LEVEL_SUPER_ADMIN || perms.Level == types.Permissions_LEVEL_ALL_MSGS || perms.Level == types.Permissions_LEVEL_SOME_MSGS || bytes.Equal(address, srv.GetAuthority()) { // add all msg type urls to the disable list for _, msgTypeURL := range msg.MsgTypeUrls { - if srv.IsAllowed(ctx, msgTypeURL) { + isAllowed, err := srv.IsAllowed(ctx, msgTypeURL) + if err != nil { + return nil, err + } + + if isAllowed { return nil, fmt.Errorf("message %s is not disabled", msgTypeURL) } - store.Delete(types.CreateDisableMsgPrefix(msgTypeURL)) + + if err = srv.DisableList.Remove(ctx, msgTypeURL); err != nil { + return nil, err + } } } else { return nil, errorsmod.Wrap(sdkerrors.ErrUnauthorized, "account does not have permission to reset circuit breaker") @@ -167,7 +190,8 @@ func (srv msgServer) ResetCircuitBreaker(goCtx context.Context, msg *types.MsgRe urls = strings.Join(msg.GetMsgTypeUrls(), ",") } - ctx.EventManager().EmitEvents(sdk.Events{ + sdkCtx := sdk.UnwrapSDKContext(ctx) + sdkCtx.EventManager().EmitEvents(sdk.Events{ sdk.NewEvent( "reset_circuit_breaker", sdk.NewAttribute("authority", msg.Authority), diff --git a/x/circuit/keeper/msg_server_test.go b/x/circuit/keeper/msg_server_test.go index e6f96e8b89e3..b5fcfc284553 100644 --- a/x/circuit/keeper/msg_server_test.go +++ b/x/circuit/keeper/msg_server_test.go @@ -1,8 +1,10 @@ -package keeper +package keeper_test import ( "testing" + "cosmossdk.io/collections" + "cosmossdk.io/x/circuit/keeper" "cosmossdk.io/x/circuit/types" "github.com/stretchr/testify/require" ) @@ -10,36 +12,36 @@ import ( const msgSend = "cosmos.bank.v1beta1.MsgSend" func Test_AuthorizeCircuitBreaker(t *testing.T) { - ft := setupFixture(t) + ft := initFixture(t) - srv := msgServer{ - Keeper: ft.Keeper, - } + srv := keeper.NewMsgServerImpl(ft.keeper) + authority, err := ft.ac.BytesToString(ft.mockAddr) + require.NoError(t, err) // add a new super admin - adminPerms := &types.Permissions{Level: types.Permissions_LEVEL_SUPER_ADMIN, LimitTypeUrls: []string{""}} - msg := &types.MsgAuthorizeCircuitBreaker{Granter: addresses[0], Grantee: addresses[1], Permissions: adminPerms} - _, err := srv.AuthorizeCircuitBreaker(ft.Ctx, msg) + adminPerms := types.Permissions{Level: types.Permissions_LEVEL_SUPER_ADMIN, LimitTypeUrls: []string{""}} + msg := &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[1], Permissions: &adminPerms} + _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) require.NoError(t, err) - add1, err := ft.Keeper.addressCodec.StringToBytes(addresses[1]) + add1, err := ft.ac.StringToBytes(addresses[1]) require.NoError(t, err) - perms, err := ft.Keeper.GetPermissions(ft.Ctx, add1) + perms, err := ft.keeper.Permissions.Get(ft.ctx, add1) require.NoError(t, err) require.Equal(t, adminPerms, perms, "admin perms are not the same") // add a super user - allmsgs := &types.Permissions{Level: types.Permissions_LEVEL_ALL_MSGS, LimitTypeUrls: []string{""}} - msg = &types.MsgAuthorizeCircuitBreaker{Granter: addresses[0], Grantee: addresses[2], Permissions: allmsgs} - _, err = srv.AuthorizeCircuitBreaker(ft.Ctx, msg) + allmsgs := types.Permissions{Level: types.Permissions_LEVEL_ALL_MSGS, LimitTypeUrls: []string{""}} + msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[2], Permissions: &allmsgs} + _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) require.NoError(t, err) - add2, err := ft.Keeper.addressCodec.StringToBytes(addresses[2]) + add2, err := ft.ac.StringToBytes(addresses[2]) require.NoError(t, err) - perms, err = ft.Keeper.GetPermissions(ft.Ctx, add2) + perms, err = ft.keeper.Permissions.Get(ft.ctx, add2) require.NoError(t, err) require.Equal(t, allmsgs, perms, "admin perms are not the same") @@ -47,155 +49,162 @@ func Test_AuthorizeCircuitBreaker(t *testing.T) { // unauthorized user who does not have perms trying to authorize superPerms := &types.Permissions{Level: types.Permissions_LEVEL_SUPER_ADMIN, LimitTypeUrls: []string{}} msg = &types.MsgAuthorizeCircuitBreaker{Granter: addresses[3], Grantee: addresses[2], Permissions: superPerms} - _, err = srv.AuthorizeCircuitBreaker(ft.Ctx, msg) + _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) require.Error(t, err, "user with no permission should fail in authorizing others") // user with permission level all_msgs tries to grant another user perms somePerms := &types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{}} msg = &types.MsgAuthorizeCircuitBreaker{Granter: addresses[2], Grantee: addresses[3], Permissions: somePerms} - _, err = srv.AuthorizeCircuitBreaker(ft.Ctx, msg) + _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) require.Error(t, err, "user[2] does not have permission to grant others permission") // user successfully grants another user perms to a specific permission - somemsgs := &types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{msgSend}} - msg = &types.MsgAuthorizeCircuitBreaker{Granter: addresses[0], Grantee: addresses[3], Permissions: somemsgs} - _, err = srv.AuthorizeCircuitBreaker(ft.Ctx, msg) + somemsgs := types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{msgSend}} + msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[3], Permissions: &somemsgs} + _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) require.NoError(t, err) - add3, err := ft.Keeper.addressCodec.StringToBytes(addresses[3]) + add3, err := ft.ac.StringToBytes(addresses[3]) require.NoError(t, err) - perms, err = ft.Keeper.GetPermissions(ft.Ctx, add3) + perms, err = ft.keeper.Permissions.Get(ft.ctx, add3) require.NoError(t, err) require.Equal(t, somemsgs, perms, "admin perms are not the same") - add4, err := ft.Keeper.addressCodec.StringToBytes(addresses[4]) + add4, err := ft.ac.StringToBytes(addresses[4]) require.NoError(t, err) - perms, err = ft.Keeper.GetPermissions(ft.Ctx, add4) - require.NoError(t, err) + perms, err = ft.keeper.Permissions.Get(ft.ctx, add4) + require.ErrorIs(t, err, collections.ErrNotFound, "user should have no perms by default") - require.Equal(t, &types.Permissions{Level: types.Permissions_LEVEL_NONE_UNSPECIFIED, LimitTypeUrls: nil}, perms, "user should have no perms by default") + require.Equal(t, types.Permissions{Level: types.Permissions_LEVEL_NONE_UNSPECIFIED, LimitTypeUrls: nil}, perms, "user should have no perms by default") // admin tries grants another user permission ALL_MSGS with limited urls populated - invalidmsgs := &types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{msgSend}} - msg = &types.MsgAuthorizeCircuitBreaker{Granter: addresses[0], Grantee: addresses[4], Permissions: invalidmsgs} - _, err = srv.AuthorizeCircuitBreaker(ft.Ctx, msg) + invalidmsgs := types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{msgSend}} + msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[4], Permissions: &invalidmsgs} + _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) require.NoError(t, err) } func Test_TripCircuitBreaker(t *testing.T) { - ft := setupFixture(t) + ft := initFixture(t) - srv := msgServer{ - Keeper: ft.Keeper, - } + srv := keeper.NewMsgServerImpl(ft.keeper) url := msgSend + authority, err := ft.ac.BytesToString(ft.mockAddr) + require.NoError(t, err) + // admin trips circuit breaker - admintrip := &types.MsgTripCircuitBreaker{Authority: addresses[0], MsgTypeUrls: []string{url}} - _, err := srv.TripCircuitBreaker(ft.Ctx, admintrip) + admintrip := &types.MsgTripCircuitBreaker{Authority: authority, MsgTypeUrls: []string{url}} + _, err = srv.TripCircuitBreaker(ft.ctx, admintrip) require.NoError(t, err) - allowed := ft.Keeper.IsAllowed(ft.Ctx, url) + allowed, err := ft.keeper.IsAllowed(ft.ctx, url) + require.NoError(t, err) require.False(t, allowed, "circuit breaker should be tripped") // user with all messages trips circuit breaker // add a super user allmsgs := &types.Permissions{Level: types.Permissions_LEVEL_ALL_MSGS, LimitTypeUrls: []string{""}} - msg := &types.MsgAuthorizeCircuitBreaker{Granter: addresses[0], Grantee: addresses[1], Permissions: allmsgs} - _, err = srv.AuthorizeCircuitBreaker(ft.Ctx, msg) + msg := &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[1], Permissions: allmsgs} + _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) require.NoError(t, err) // try to trip the circuit breaker url2 := "cosmos.staking.v1beta1.MsgDelegate" superTrip := &types.MsgTripCircuitBreaker{Authority: addresses[1], MsgTypeUrls: []string{url2}} - _, err = srv.TripCircuitBreaker(ft.Ctx, superTrip) + _, err = srv.TripCircuitBreaker(ft.ctx, superTrip) require.NoError(t, err) - allowed = ft.Keeper.IsAllowed(ft.Ctx, url2) + allowed, err = ft.keeper.IsAllowed(ft.ctx, url2) + require.NoError(t, err) require.False(t, allowed, "circuit breaker should be tripped") // user with no permission attempts to trips circuit breaker unknownTrip := &types.MsgTripCircuitBreaker{Authority: addresses[4], MsgTypeUrls: []string{url}} - _, err = srv.TripCircuitBreaker(ft.Ctx, unknownTrip) + _, err = srv.TripCircuitBreaker(ft.ctx, unknownTrip) require.Error(t, err) // user has permission to trip circuit breaker for two messages but only has permission for one url, url2 = "cosmos.staking.v1beta1.MsgCreateValidator", "cosmos.staking.v1beta1.MsgEditValidator" somemsgs := &types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{url}} - msg = &types.MsgAuthorizeCircuitBreaker{Granter: addresses[0], Grantee: addresses[2], Permissions: somemsgs} - _, err = srv.AuthorizeCircuitBreaker(ft.Ctx, msg) + msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[2], Permissions: somemsgs} + _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) require.NoError(t, err) // try to trip two messages but user only has permission for one someTrip := &types.MsgTripCircuitBreaker{Authority: addresses[2], MsgTypeUrls: []string{url, url2}} - _, err = srv.TripCircuitBreaker(ft.Ctx, someTrip) + _, err = srv.TripCircuitBreaker(ft.ctx, someTrip) require.ErrorContains(t, err, "MsgEditValidator") // user tries to trip an already tripped circuit breaker alreadyTripped := "cosmos.bank.v1beta1.MsgSend" twoTrip := &types.MsgTripCircuitBreaker{Authority: addresses[1], MsgTypeUrls: []string{alreadyTripped}} - _, err = srv.TripCircuitBreaker(ft.Ctx, twoTrip) + _, err = srv.TripCircuitBreaker(ft.ctx, twoTrip) require.Error(t, err) } func Test_ResetCircuitBreaker(t *testing.T) { - ft := setupFixture(t) + ft := initFixture(t) + authority, err := ft.ac.BytesToString(ft.mockAddr) + require.NoError(t, err) - srv := msgServer{ - Keeper: ft.Keeper, - } + srv := keeper.NewMsgServerImpl(ft.keeper) // admin resets circuit breaker url := "cosmos.bank.v1beta1.MsgSend" // admin trips circuit breaker - admintrip := &types.MsgTripCircuitBreaker{Authority: addresses[0], MsgTypeUrls: []string{url}} - _, err := srv.TripCircuitBreaker(ft.Ctx, admintrip) + admintrip := &types.MsgTripCircuitBreaker{Authority: authority, MsgTypeUrls: []string{url}} + _, err = srv.TripCircuitBreaker(ft.ctx, admintrip) require.NoError(t, err) - allowed := ft.Keeper.IsAllowed(ft.Ctx, url) + allowed, err := ft.keeper.IsAllowed(ft.ctx, url) + require.NoError(t, err) require.False(t, allowed, "circuit breaker should be tripped") - adminReset := &types.MsgResetCircuitBreaker{Authority: addresses[0], MsgTypeUrls: []string{url}} - _, err = srv.ResetCircuitBreaker(ft.Ctx, adminReset) + adminReset := &types.MsgResetCircuitBreaker{Authority: authority, MsgTypeUrls: []string{url}} + _, err = srv.ResetCircuitBreaker(ft.ctx, adminReset) require.NoError(t, err) - allowed = ft.Keeper.IsAllowed(ft.Ctx, url) + allowed, err = ft.keeper.IsAllowed(ft.ctx, url) + require.NoError(t, err) require.True(t, allowed, "circuit breaker should be reset") // user has no permission to reset circuit breaker // admin trips circuit breaker - _, err = srv.TripCircuitBreaker(ft.Ctx, admintrip) + _, err = srv.TripCircuitBreaker(ft.ctx, admintrip) require.NoError(t, err) - allowed = ft.Keeper.IsAllowed(ft.Ctx, url) + allowed, err = ft.keeper.IsAllowed(ft.ctx, url) + require.NoError(t, err) require.False(t, allowed, "circuit breaker should be tripped") unknownUserReset := &types.MsgResetCircuitBreaker{Authority: addresses[1], MsgTypeUrls: []string{url}} - _, err = srv.ResetCircuitBreaker(ft.Ctx, unknownUserReset) + _, err = srv.ResetCircuitBreaker(ft.ctx, unknownUserReset) require.Error(t, err) - allowed = ft.Keeper.IsAllowed(ft.Ctx, url) + allowed, err = ft.keeper.IsAllowed(ft.ctx, url) + require.NoError(t, err) require.False(t, allowed, "circuit breaker should be reset") // user with all messages resets circuit breaker allmsgs := &types.Permissions{Level: types.Permissions_LEVEL_ALL_MSGS, LimitTypeUrls: []string{""}} - msg := &types.MsgAuthorizeCircuitBreaker{Granter: addresses[0], Grantee: addresses[1], Permissions: allmsgs} - _, err = srv.AuthorizeCircuitBreaker(ft.Ctx, msg) + msg := &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[1], Permissions: allmsgs} + _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) require.NoError(t, err) // trip the circuit breaker url2 := "cosmos.staking.v1beta1.MsgDelegate" - admintrip = &types.MsgTripCircuitBreaker{Authority: addresses[0], MsgTypeUrls: []string{url2}} - _, err = srv.TripCircuitBreaker(ft.Ctx, admintrip) + admintrip = &types.MsgTripCircuitBreaker{Authority: authority, MsgTypeUrls: []string{url2}} + _, err = srv.TripCircuitBreaker(ft.ctx, admintrip) require.NoError(t, err) // user with all messages resets circuit breaker allMsgsReset := &types.MsgResetCircuitBreaker{Authority: addresses[1], MsgTypeUrls: []string{url}} - _, err = srv.ResetCircuitBreaker(ft.Ctx, allMsgsReset) + _, err = srv.ResetCircuitBreaker(ft.ctx, allMsgsReset) require.NoError(t, err) // user tries to reset an message they dont have permission to reset @@ -203,21 +212,21 @@ func Test_ResetCircuitBreaker(t *testing.T) { url = "cosmos.staking.v1beta1.MsgCreateValidator" // give restricted perms to a user someMsgs := &types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{url2}} - msg = &types.MsgAuthorizeCircuitBreaker{Granter: addresses[0], Grantee: addresses[2], Permissions: someMsgs} - _, err = srv.AuthorizeCircuitBreaker(ft.Ctx, msg) + msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[2], Permissions: someMsgs} + _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) require.NoError(t, err) - admintrip = &types.MsgTripCircuitBreaker{Authority: addresses[0], MsgTypeUrls: []string{url}} - _, err = srv.TripCircuitBreaker(ft.Ctx, admintrip) + admintrip = &types.MsgTripCircuitBreaker{Authority: authority, MsgTypeUrls: []string{url}} + _, err = srv.TripCircuitBreaker(ft.ctx, admintrip) require.NoError(t, err) // user with all messages resets circuit breaker someMsgsReset := &types.MsgResetCircuitBreaker{Authority: addresses[2], MsgTypeUrls: []string{url}} - _, err = srv.ResetCircuitBreaker(ft.Ctx, someMsgsReset) + _, err = srv.ResetCircuitBreaker(ft.ctx, someMsgsReset) require.NoError(t, err) // user tries to reset an already reset circuit breaker admintrip = &types.MsgTripCircuitBreaker{Authority: addresses[1], MsgTypeUrls: []string{url2}} - _, err = srv.TripCircuitBreaker(ft.Ctx, admintrip) + _, err = srv.TripCircuitBreaker(ft.ctx, admintrip) require.Error(t, err) } diff --git a/x/circuit/keeper/query.go b/x/circuit/keeper/query.go index 4b8f505f6769..4b8510073fd2 100644 --- a/x/circuit/keeper/query.go +++ b/x/circuit/keeper/query.go @@ -6,9 +6,11 @@ import ( "cosmossdk.io/store/prefix" "cosmossdk.io/x/circuit/types" + "github.com/cosmos/gogoproto/proto" + + "github.com/cosmos/cosmos-sdk/runtime" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/query" - "github.com/cosmos/gogoproto/proto" ) var _ types.QueryServer = QueryServer{} @@ -32,21 +34,20 @@ func (qs QueryServer) Account(c context.Context, req *types.QueryAccountRequest) return nil, err } - perms, err := qs.keeper.GetPermissions(sdkCtx, add) + perms, err := qs.keeper.Permissions.Get(sdkCtx, add) if err != nil { return nil, err } - return &types.AccountResponse{Permission: perms}, nil + return &types.AccountResponse{Permission: &perms}, nil } // Account returns account permissions. -func (qs QueryServer) Accounts(c context.Context, req *types.QueryAccountsRequest) (*types.AccountsResponse, error) { - sdkCtx := sdk.UnwrapSDKContext(c) +func (qs QueryServer) Accounts(ctx context.Context, req *types.QueryAccountsRequest) (*types.AccountsResponse, error) { // Iterate over accounts and perform the callback var accounts []*types.GenesisAccountPermissions - store := sdkCtx.KVStore(qs.keeper.storekey) + store := runtime.KVStoreAdapter(qs.keeper.storeService.OpenKVStore(ctx)) accountsStore := prefix.NewStore(store, types.AccountPermissionPrefix) pageRes, err := query.Paginate(accountsStore, req.Pagination, func(key, value []byte) error { @@ -76,14 +77,12 @@ func (qs QueryServer) Accounts(c context.Context, req *types.QueryAccountsReques } // DisabledList returns a list of disabled message urls -func (qs QueryServer) DisabledList(c context.Context, req *types.QueryDisabledListRequest) (*types.DisabledListResponse, error) { - sdkCtx := sdk.UnwrapSDKContext(c) +func (qs QueryServer) DisabledList(ctx context.Context, req *types.QueryDisabledListRequest) (*types.DisabledListResponse, error) { // Iterate over disabled list and perform the callback - var msgs []string - qs.keeper.IterateDisableLists(sdkCtx, func(address []byte, perm types.Permissions) (stop bool) { - msgs = append(msgs, perm.LimitTypeUrls...) - return false + qs.keeper.DisableList.Walk(ctx, nil, func(msgUrl string) (bool, error) { + msgs = append(msgs, msgUrl) + return false, nil }) return &types.DisabledListResponse{DisabledList: msgs}, nil diff --git a/x/circuit/keeper/query_test.go b/x/circuit/keeper/query_test.go index c8749696336b..825778931555 100644 --- a/x/circuit/keeper/query_test.go +++ b/x/circuit/keeper/query_test.go @@ -1,28 +1,30 @@ -package keeper +package keeper_test import ( "testing" + "cosmossdk.io/x/circuit/keeper" "cosmossdk.io/x/circuit/types" - "github.com/cosmos/cosmos-sdk/types/query" "github.com/stretchr/testify/require" + + "github.com/cosmos/cosmos-sdk/types/query" ) func TestQueryAccount(t *testing.T) { t.Parallel() - f := setupFixture(t) + f := initFixture(t) - add, err := f.Keeper.addressCodec.StringToBytes(addresses[0]) + add, err := f.ac.StringToBytes(addresses[0]) require.NoError(t, err) - err = f.Keeper.SetPermissions(f.Ctx, add, &f.MockPerms) + err = f.keeper.Permissions.Set(f.ctx, add, f.mockPerms) require.NoError(t, err) // create a new query server - qs := QueryServer{keeper: f.Keeper} + qs := keeper.NewQueryServer(f.keeper) // test the Account method - res, err := qs.Account(f.Ctx, &types.QueryAccountRequest{Address: addresses[0]}) + res, err := qs.Account(f.ctx, &types.QueryAccountRequest{Address: addresses[0]}) require.NoError(t, err) require.Equal(t, res.Permission.Level, types.Permissions_Level(3)) require.Equal(t, res.Permission.LimitTypeUrls, []string{ @@ -32,26 +34,26 @@ func TestQueryAccount(t *testing.T) { func TestQueryAccounts(t *testing.T) { t.Parallel() - f := setupFixture(t) + f := initFixture(t) - add, err := f.Keeper.addressCodec.StringToBytes(addresses[0]) + add, err := f.ac.StringToBytes(addresses[0]) require.NoError(t, err) - err = f.Keeper.SetPermissions(f.Ctx, add, &f.MockPerms) + err = f.keeper.Permissions.Set(f.ctx, add, f.mockPerms) require.NoError(t, err) // create a new query server - qs := QueryServer{keeper: f.Keeper} + qs := keeper.NewQueryServer(f.keeper) // test the Accounts method - res1, err := qs.Accounts(f.Ctx, &types.QueryAccountsRequest{ + res1, err := qs.Accounts(f.ctx, &types.QueryAccountsRequest{ Pagination: &query.PageRequest{Limit: 10}, }) require.NoError(t, err) for _, a := range res1.Accounts { require.Equal(t, addresses[0], a.Address) - require.Equal(t, f.MockPerms, *a.Permissions) + require.Equal(t, f.mockPerms, *a.Permissions) } require.NotNil(t, res1) @@ -59,19 +61,15 @@ func TestQueryAccounts(t *testing.T) { func TestQueryDisabledList(t *testing.T) { t.Parallel() - f := setupFixture(t) + f := initFixture(t) - add, err := f.Keeper.addressCodec.StringToBytes(addresses[0]) - require.NoError(t, err) - - err = f.Keeper.SetPermissions(f.Ctx, add, &f.MockPerms) - require.NoError(t, err) + f.keeper.DisableMsg(f.ctx, f.mockMsgURL) // create a new query server - qs := QueryServer{keeper: f.Keeper} + qs := keeper.NewQueryServer(f.keeper) // test the DisabledList method - disabledList, err := qs.DisabledList(f.Ctx, &types.QueryDisabledListRequest{}) + disabledList, err := qs.DisabledList(f.ctx, &types.QueryDisabledListRequest{}) require.NoError(t, err) - require.Equal(t, []string{"test"}, disabledList.DisabledList) + require.Equal(t, []string{f.mockMsgURL}, disabledList.DisabledList) } diff --git a/x/circuit/keeper/test_utils.go b/x/circuit/keeper/test_utils.go deleted file mode 100644 index 7c9f2658c577..000000000000 --- a/x/circuit/keeper/test_utils.go +++ /dev/null @@ -1,47 +0,0 @@ -package keeper - -import ( - "testing" - - storetypes "cosmossdk.io/store/types" - cmproto "github.com/cometbft/cometbft/proto/tendermint/types" - addresscodec "github.com/cosmos/cosmos-sdk/codec/address" - "github.com/cosmos/cosmos-sdk/testutil" - sdk "github.com/cosmos/cosmos-sdk/types" - - "cosmossdk.io/x/circuit/types" -) - -var addresses = []string{ - "cosmos1zglwfu6xjzvzagqcmvzewyzjp9xwqw5qwrr8n9", - "cosmos1p8s0p6gqc6c9gt77lgr2qqujz49huhu6a80smx", - "cosmos1qasf9ehx8m7cnat39ndc74rx3fg7z66u8lw0fd", - "cosmos1uxrdj5zfuudhypsmmjxnj4gpu432ycht06a05a", - "cosmos1wn7k8a7fwpmrwnm94ndj0germfnxnhl6hs8spj", -} - -type fixture struct { - Ctx sdk.Context - Keeper Keeper - MockPerms types.Permissions - MockMsgURL string -} - -func setupFixture(t *testing.T) *fixture { - mockStoreKey := storetypes.NewKVStoreKey("circuit") - keeperX := NewKeeper(mockStoreKey, addresses[0], addresscodec.NewBech32Codec("cosmos")) - mockMsgURL := "mock_url" - mockCtx := testutil.DefaultContextWithDB(t, mockStoreKey, storetypes.NewTransientStoreKey("transient_test")) - ctx := mockCtx.Ctx.WithBlockHeader(cmproto.Header{}) - mockPerms := types.Permissions{ - Level: 3, - LimitTypeUrls: []string{"test"}, - } - - return &fixture{ - Ctx: ctx, - Keeper: keeperX, - MockPerms: mockPerms, - MockMsgURL: mockMsgURL, - } -} diff --git a/x/circuit/module.go b/x/circuit/module.go index 4ff8732335b7..4a4f73557be8 100644 --- a/x/circuit/module.go +++ b/x/circuit/module.go @@ -173,6 +173,7 @@ func ProvideModule(in ModuleInputs) ModuleOutputs { } circuitkeeper := keeper.NewKeeper( + in.Cdc, in.StoreService, authority.String(), in.AddressCodec, diff --git a/x/circuit/types/keys.go b/x/circuit/types/keys.go index e315590e6c6f..db4e1dc6203e 100644 --- a/x/circuit/types/keys.go +++ b/x/circuit/types/keys.go @@ -1,5 +1,7 @@ package types +import "cosmossdk.io/collections" + const ( // ModuleName defines the module name ModuleName = "circuit" @@ -13,20 +15,6 @@ const ( // KVStore keys var ( - AccountPermissionPrefix = []byte{0x01} - DisableListPrefix = []byte{0x02} + AccountPermissionPrefix = collections.NewPrefix(1) + DisableListPrefix = collections.NewPrefix(2) ) - -func CreateAddressPrefix(account []byte) []byte { - key := make([]byte, len(AccountPermissionPrefix)+len(account)+1) - copy(key, AccountPermissionPrefix) - copy(key[len(AccountPermissionPrefix):], account) - return key -} - -func CreateDisableMsgPrefix(msgURL string) []byte { - key := make([]byte, len(DisableListPrefix)+len(msgURL)+1) - copy(key, DisableListPrefix) - copy(key[len(DisableListPrefix):], msgURL) - return key -} From 25e1b9ddf8adbeefb341ddbd8a63643faca0502d Mon Sep 17 00:00:00 2001 From: Facundo Medica Date: Sun, 4 Jun 2023 14:50:59 +0200 Subject: [PATCH 03/14] fix ante handle --- x/circuit/ante/circuit.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/circuit/ante/circuit.go b/x/circuit/ante/circuit.go index 72b5f6908357..2ae3b66f1251 100644 --- a/x/circuit/ante/circuit.go +++ b/x/circuit/ante/circuit.go @@ -24,7 +24,7 @@ func NewCircuitBreakerDecorator(ck CircuitBreaker) CircuitBreakerDecorator { } } -func (cbd CircuitBreakerDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (context.Context, error) { +func (cbd CircuitBreakerDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { // loop through all the messages and check if the message type is allowed for _, msg := range tx.GetMsgs() { if !cbd.circuitKeeper.IsAllowed(ctx, sdk.MsgTypeURL(msg)) { From 70cfb68c3e49d309527c17c6855972ab07ddd2e6 Mon Sep 17 00:00:00 2001 From: Facundo Medica Date: Sun, 4 Jun 2023 14:51:55 +0200 Subject: [PATCH 04/14] fix legacy app --- simapp/app.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/simapp/app.go b/simapp/app.go index 57c6baf2e8e9..e4bd72b367ae 100644 --- a/simapp/app.go +++ b/simapp/app.go @@ -15,6 +15,7 @@ import ( reflectionv1 "cosmossdk.io/api/cosmos/reflection/v1" "cosmossdk.io/client/v2/autocli" "cosmossdk.io/core/appmodule" + "github.com/cosmos/cosmos-sdk/codec/address" authcodec "github.com/cosmos/cosmos-sdk/x/auth/codec" @@ -339,7 +340,7 @@ func NewSimApp( stakingtypes.NewMultiStakingHooks(app.DistrKeeper.Hooks(), app.SlashingKeeper.Hooks()), ) - app.CircuitKeeper = circuitkeeper.NewKeeper(keys[circuittypes.StoreKey], authtypes.NewModuleAddress(govtypes.ModuleName).String(), app.AccountKeeper.AddressCodec()) + app.CircuitKeeper = circuitkeeper.NewKeeper(runtime.NewKVStoreService(keys[circuittypes.StoreKey]), authtypes.NewModuleAddress(govtypes.ModuleName).String(), app.AccountKeeper.AddressCodec()) app.BaseApp.SetCircuitBreaker(&app.CircuitKeeper) app.AuthzKeeper = authzkeeper.NewKeeper(runtime.NewKVStoreService(keys[authzkeeper.StoreKey]), appCodec, app.MsgServiceRouter(), app.AccountKeeper) From 3e3aa0441a9f9bc4275b3838d21a2f349cdb9724 Mon Sep 17 00:00:00 2001 From: Facundo Medica Date: Mon, 5 Jun 2023 14:22:38 +0200 Subject: [PATCH 05/14] finish switch to collections --- x/circuit/keeper/query.go | 31 ++++++++----------------------- 1 file changed, 8 insertions(+), 23 deletions(-) diff --git a/x/circuit/keeper/query.go b/x/circuit/keeper/query.go index 4b8510073fd2..6a8864ea466b 100644 --- a/x/circuit/keeper/query.go +++ b/x/circuit/keeper/query.go @@ -1,14 +1,10 @@ package keeper import ( - "bytes" "context" - "cosmossdk.io/store/prefix" "cosmossdk.io/x/circuit/types" - "github.com/cosmos/gogoproto/proto" - "github.com/cosmos/cosmos-sdk/runtime" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/query" ) @@ -44,33 +40,22 @@ func (qs QueryServer) Account(c context.Context, req *types.QueryAccountRequest) // Account returns account permissions. func (qs QueryServer) Accounts(ctx context.Context, req *types.QueryAccountsRequest) (*types.AccountsResponse, error) { - // Iterate over accounts and perform the callback - var accounts []*types.GenesisAccountPermissions - store := runtime.KVStoreAdapter(qs.keeper.storeService.OpenKVStore(ctx)) - accountsStore := prefix.NewStore(store, types.AccountPermissionPrefix) - - pageRes, err := query.Paginate(accountsStore, req.Pagination, func(key, value []byte) error { - perm := &types.Permissions{} - if err := proto.Unmarshal(value, perm); err != nil { - return err - } + results, pageRes, err := query.CollectionPaginate[[]byte, types.Permissions](ctx, qs.keeper.Permissions, req.Pagination) + if err != nil { + return nil, err + } - // remove key suffix - address, err := qs.keeper.addressCodec.BytesToString(bytes.TrimRight(key, "\x00")) + for _, result := range results { + address, err := qs.keeper.addressCodec.BytesToString(result.Key) if err != nil { - return err + return nil, err } accounts = append(accounts, &types.GenesisAccountPermissions{ Address: address, - Permissions: perm, + Permissions: &result.Value, }) - - return nil - }) - if err != nil { - return nil, err } return &types.AccountsResponse{Accounts: accounts, Pagination: pageRes}, nil From e3211559b12aab4fcc3b60459aaf41dd6532fb47 Mon Sep 17 00:00:00 2001 From: Facundo Medica Date: Mon, 5 Jun 2023 14:29:12 +0200 Subject: [PATCH 06/14] changelog --- x/circuit/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/x/circuit/CHANGELOG.md b/x/circuit/CHANGELOG.md index 16de809f2717..6047ce566e60 100644 --- a/x/circuit/CHANGELOG.md +++ b/x/circuit/CHANGELOG.md @@ -30,3 +30,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ # Changelog ## [Unreleased] + +### API Breaking + +* (x/circuit) [#16415](https://github.com/cosmos/cosmos-sdk/issues/16415) `NewKeeper` now takes a `KVStoreService` instead of a `StoreKey` and requires a codec. Methods in the `Keeper` now take a `context.Context` instead of a `sdk.Context` and return an `error`. Migrated to collections. `IsAllowed` from the `CircuitBreaker` interface now takes a `context.Context` instead of a `sdk.Context` and returns `bool, error`. \ No newline at end of file From bf427f602d0f625c6f379d201b833565249ebef9 Mon Sep 17 00:00:00 2001 From: Facundo Medica Date: Mon, 5 Jun 2023 14:30:08 +0200 Subject: [PATCH 07/14] upgrading --- UPGRADING.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/UPGRADING.md b/UPGRADING.md index 1c50a7ac75d8..d084c8ed618d 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -80,6 +80,7 @@ The following modules `NewKeeper` function now take a `KVStoreService` instead o * `x/auth` * `x/authz` * `x/bank` +* `x/circuit` * `x/consensus` * `x/crisis` * `x/distribution` @@ -105,6 +106,7 @@ The following modules' `Keeper` methods now take in a `context.Context` instead * `x/authz` * `x/bank` +* `x/circuit` * `x/mint` * `x/crisis` * `x/distribution` From 7744afaaac848bbd7c177c0d7ff73acde34011e5 Mon Sep 17 00:00:00 2001 From: Facundo Medica Date: Mon, 5 Jun 2023 14:32:39 +0200 Subject: [PATCH 08/14] fix changelog --- CHANGELOG.md | 1 + x/circuit/CHANGELOG.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8dc5cf3fa9e2..daf8cb72a509 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -135,6 +135,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### API Breaking Changes +* (baseapp) [#16415](https://github.com/cosmos/cosmos-sdk/issues/16415) `IsAllowed` from the `CircuitBreaker` interface now takes a `context.Context` instead of a `sdk.Context` and returns `bool, error`. * (x/slashing) [#16246](https://github.com/cosmos/cosmos-sdk/issues/16246) `NewKeeper` now takes a `KVStoreService` instead of a `StoreKey`, and methods in the `Keeper` now take a `context.Context` instead of a `sdk.Context` and return an `error`. `GetValidatorSigningInfo` now returns an error instead of a `found bool`, the error can be `nil` (found), `ErrNoSigningInfoFound` (not found) and any other error. * (x/mint) [#16179](https://github.com/cosmos/cosmos-sdk/issues/16179) `NewKeeper` now takes a `KVStoreService` instead of a `StoreKey`, and methods in the `Keeper` now take a `context.Context` instead of a `sdk.Context` and return an `error`. * (x/crisis) [#16216](https://github.com/cosmos/cosmos-sdk/issues/16216) `NewKeeper` now takes a `KVStoreService` instead of a `StoreKey`, methods in the `Keeper` now take a `context.Context` instead of a `sdk.Context` and return an `error` instead of panicking. diff --git a/x/circuit/CHANGELOG.md b/x/circuit/CHANGELOG.md index 6047ce566e60..7f47539b8471 100644 --- a/x/circuit/CHANGELOG.md +++ b/x/circuit/CHANGELOG.md @@ -33,4 +33,4 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### API Breaking -* (x/circuit) [#16415](https://github.com/cosmos/cosmos-sdk/issues/16415) `NewKeeper` now takes a `KVStoreService` instead of a `StoreKey` and requires a codec. Methods in the `Keeper` now take a `context.Context` instead of a `sdk.Context` and return an `error`. Migrated to collections. `IsAllowed` from the `CircuitBreaker` interface now takes a `context.Context` instead of a `sdk.Context` and returns `bool, error`. \ No newline at end of file +* (x/circuit) [#16415](https://github.com/cosmos/cosmos-sdk/issues/16415) `NewKeeper` now takes a `KVStoreService` instead of a `StoreKey` and requires a codec. Methods in the `Keeper` now take a `context.Context` instead of a `sdk.Context` and return an `error`. Migrated to collections. \ No newline at end of file From 3eab85b5dd8d6a1c7fa90e4d8c581c5f080908a1 Mon Sep 17 00:00:00 2001 From: Facundo Medica Date: Mon, 5 Jun 2023 14:49:52 +0200 Subject: [PATCH 09/14] fix legacy app and interface --- simapp/app.go | 2 +- x/circuit/CHANGELOG.md | 2 +- x/circuit/ante/circuit.go | 9 +++++++-- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/simapp/app.go b/simapp/app.go index e4bd72b367ae..c1b0a96b9d18 100644 --- a/simapp/app.go +++ b/simapp/app.go @@ -340,7 +340,7 @@ func NewSimApp( stakingtypes.NewMultiStakingHooks(app.DistrKeeper.Hooks(), app.SlashingKeeper.Hooks()), ) - app.CircuitKeeper = circuitkeeper.NewKeeper(runtime.NewKVStoreService(keys[circuittypes.StoreKey]), authtypes.NewModuleAddress(govtypes.ModuleName).String(), app.AccountKeeper.AddressCodec()) + app.CircuitKeeper = circuitkeeper.NewKeeper(appCodec, runtime.NewKVStoreService(keys[circuittypes.StoreKey]), authtypes.NewModuleAddress(govtypes.ModuleName).String(), app.AccountKeeper.AddressCodec()) app.BaseApp.SetCircuitBreaker(&app.CircuitKeeper) app.AuthzKeeper = authzkeeper.NewKeeper(runtime.NewKVStoreService(keys[authzkeeper.StoreKey]), appCodec, app.MsgServiceRouter(), app.AccountKeeper) diff --git a/x/circuit/CHANGELOG.md b/x/circuit/CHANGELOG.md index 7f47539b8471..6047ce566e60 100644 --- a/x/circuit/CHANGELOG.md +++ b/x/circuit/CHANGELOG.md @@ -33,4 +33,4 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### API Breaking -* (x/circuit) [#16415](https://github.com/cosmos/cosmos-sdk/issues/16415) `NewKeeper` now takes a `KVStoreService` instead of a `StoreKey` and requires a codec. Methods in the `Keeper` now take a `context.Context` instead of a `sdk.Context` and return an `error`. Migrated to collections. \ No newline at end of file +* (x/circuit) [#16415](https://github.com/cosmos/cosmos-sdk/issues/16415) `NewKeeper` now takes a `KVStoreService` instead of a `StoreKey` and requires a codec. Methods in the `Keeper` now take a `context.Context` instead of a `sdk.Context` and return an `error`. Migrated to collections. `IsAllowed` from the `CircuitBreaker` interface now takes a `context.Context` instead of a `sdk.Context` and returns `bool, error`. \ No newline at end of file diff --git a/x/circuit/ante/circuit.go b/x/circuit/ante/circuit.go index 2ae3b66f1251..80ed8ce5f231 100644 --- a/x/circuit/ante/circuit.go +++ b/x/circuit/ante/circuit.go @@ -10,7 +10,7 @@ import ( // CircuitBreaker is an interface that defines the methods for a circuit breaker. type CircuitBreaker interface { - IsAllowed(ctx context.Context, typeURL string) bool + IsAllowed(ctx context.Context, typeURL string) (bool, error) } // CircuitBreakerDecorator is an AnteDecorator that checks if the transaction type is allowed to enter the mempool or be executed @@ -27,7 +27,12 @@ func NewCircuitBreakerDecorator(ck CircuitBreaker) CircuitBreakerDecorator { func (cbd CircuitBreakerDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { // loop through all the messages and check if the message type is allowed for _, msg := range tx.GetMsgs() { - if !cbd.circuitKeeper.IsAllowed(ctx, sdk.MsgTypeURL(msg)) { + isAllowed, err := cbd.circuitKeeper.IsAllowed(ctx, sdk.MsgTypeURL(msg)) + if err != nil { + return ctx, err + } + + if !isAllowed { return ctx, errors.New("tx type not allowed") } } From 875668851334680cc594383cf9d3f8ce69ebc37c Mon Sep 17 00:00:00 2001 From: Facundo Medica Date: Mon, 5 Jun 2023 15:04:25 +0200 Subject: [PATCH 10/14] last fixes --- x/auth/ante/testutil/expected_keepers_mocks.go | 15 +++++++++++++++ x/circuit/ante/circuit_test.go | 4 ++-- x/circuit/keeper/genesis.go | 6 ++++-- x/circuit/keeper/msg_server.go | 5 ++--- x/circuit/keeper/query.go | 7 ++++++- 5 files changed, 29 insertions(+), 8 deletions(-) diff --git a/x/auth/ante/testutil/expected_keepers_mocks.go b/x/auth/ante/testutil/expected_keepers_mocks.go index 1302bd240b83..f6c54d54179d 100644 --- a/x/auth/ante/testutil/expected_keepers_mocks.go +++ b/x/auth/ante/testutil/expected_keepers_mocks.go @@ -8,6 +8,7 @@ import ( context "context" reflect "reflect" + address "cosmossdk.io/core/address" types "github.com/cosmos/cosmos-sdk/types" types0 "github.com/cosmos/cosmos-sdk/x/auth/types" gomock "github.com/golang/mock/gomock" @@ -36,6 +37,20 @@ func (m *MockAccountKeeper) EXPECT() *MockAccountKeeperMockRecorder { return m.recorder } +// AddressCodec mocks base method. +func (m *MockAccountKeeper) AddressCodec() address.Codec { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AddressCodec") + ret0, _ := ret[0].(address.Codec) + return ret0 +} + +// AddressCodec indicates an expected call of AddressCodec. +func (mr *MockAccountKeeperMockRecorder) AddressCodec() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddressCodec", reflect.TypeOf((*MockAccountKeeper)(nil).AddressCodec)) +} + // GetAccount mocks base method. func (m *MockAccountKeeper) GetAccount(ctx context.Context, addr types.AccAddress) types.AccountI { m.ctrl.T.Helper() diff --git a/x/circuit/ante/circuit_test.go b/x/circuit/ante/circuit_test.go index a305ca17cbe1..4f4610520c63 100644 --- a/x/circuit/ante/circuit_test.go +++ b/x/circuit/ante/circuit_test.go @@ -35,8 +35,8 @@ type MockCircuitBreaker struct { isAllowed bool } -func (m MockCircuitBreaker) IsAllowed(ctx context.Context, typeURL string) bool { - return typeURL == "/cosmos.circuit.v1.MsgAuthorizeCircuitBreaker" +func (m MockCircuitBreaker) IsAllowed(ctx context.Context, typeURL string) (bool, error) { + return typeURL == "/cosmos.circuit.v1.MsgAuthorizeCircuitBreaker", nil } func initFixture(t *testing.T) *fixture { diff --git a/x/circuit/keeper/genesis.go b/x/circuit/keeper/genesis.go index 8c1db8670ce6..6ae64280d5fd 100644 --- a/x/circuit/keeper/genesis.go +++ b/x/circuit/keeper/genesis.go @@ -3,6 +3,8 @@ package keeper import ( context "context" + "cosmossdk.io/collections" + errorsmod "cosmossdk.io/errors" "cosmossdk.io/x/circuit/types" ) @@ -25,7 +27,7 @@ func (k *Keeper) ExportGenesis(ctx context.Context) (data *types.GenesisState) { }) return false, nil }) - if err != nil { + if err != nil && !errorsmod.IsOf(err, collections.ErrInvalidIterator) { panic(err) } @@ -33,7 +35,7 @@ func (k *Keeper) ExportGenesis(ctx context.Context) (data *types.GenesisState) { disabledMsgs = append(disabledMsgs, msgUrl) return false, nil }) - if err != nil { + if err != nil && !errorsmod.IsOf(err, collections.ErrInvalidIterator) { panic(err) } diff --git a/x/circuit/keeper/msg_server.go b/x/circuit/keeper/msg_server.go index f2585b4b9ca2..99766c49887d 100644 --- a/x/circuit/keeper/msg_server.go +++ b/x/circuit/keeper/msg_server.go @@ -3,7 +3,6 @@ package keeper import ( "bytes" context "context" - "errors" fmt "fmt" "strings" @@ -84,7 +83,7 @@ func (srv msgServer) TripCircuitBreaker(ctx context.Context, msg *types.MsgTripC // Check that the account has the permissions perms, err := srv.Permissions.Get(ctx, address) - if err != nil && !errors.Is(err, collections.ErrNotFound) { + if err != nil && !errorsmod.IsOf(err, collections.ErrNotFound) { return nil, err } @@ -161,7 +160,7 @@ func (srv msgServer) ResetCircuitBreaker(ctx context.Context, msg *types.MsgRese // Get the permissions for the account specified in the msg.Authority field perms, err := keeper.Permissions.Get(ctx, address) - if err != nil && !errors.Is(err, collections.ErrNotFound) { + if err != nil && !errorsmod.IsOf(err, collections.ErrNotFound) { return nil, fmt.Errorf("user permission does not exist %w", err) } diff --git a/x/circuit/keeper/query.go b/x/circuit/keeper/query.go index 6a8864ea466b..f633c104ec42 100644 --- a/x/circuit/keeper/query.go +++ b/x/circuit/keeper/query.go @@ -3,6 +3,8 @@ package keeper import ( "context" + "cosmossdk.io/collections" + errorsmod "cosmossdk.io/errors" "cosmossdk.io/x/circuit/types" sdk "github.com/cosmos/cosmos-sdk/types" @@ -65,10 +67,13 @@ func (qs QueryServer) Accounts(ctx context.Context, req *types.QueryAccountsRequ func (qs QueryServer) DisabledList(ctx context.Context, req *types.QueryDisabledListRequest) (*types.DisabledListResponse, error) { // Iterate over disabled list and perform the callback var msgs []string - qs.keeper.DisableList.Walk(ctx, nil, func(msgUrl string) (bool, error) { + err := qs.keeper.DisableList.Walk(ctx, nil, func(msgUrl string) (bool, error) { msgs = append(msgs, msgUrl) return false, nil }) + if err != nil && !errorsmod.IsOf(err, collections.ErrInvalidIterator) { + return nil, err + } return &types.DisabledListResponse{DisabledList: msgs}, nil } From b87d45549d69c0b2f01759739b5304fdc3a85dce Mon Sep 17 00:00:00 2001 From: Facundo Medica Date: Wed, 7 Jun 2023 09:21:28 +0200 Subject: [PATCH 11/14] suggestions --- UPGRADING.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/UPGRADING.md b/UPGRADING.md index 2b56e6282b7e..07711026ad39 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -80,7 +80,6 @@ The following modules `NewKeeper` function now take a `KVStoreService` instead o * `x/auth` * `x/authz` * `x/bank` -* `x/circuit` * `x/consensus` * `x/crisis` * `x/distribution` @@ -107,7 +106,6 @@ The following modules' `Keeper` methods now take in a `context.Context` instead * `x/authz` * `x/bank` -* `x/circuit` * `x/mint` * `x/crisis` * `x/distribution` From d868fb783f9ec866dda95c67fb5cc8b5b43d3f0e Mon Sep 17 00:00:00 2001 From: Facundo Medica Date: Wed, 7 Jun 2023 09:21:55 +0200 Subject: [PATCH 12/14] suggestions --- x/circuit/CHANGELOG.md | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/x/circuit/CHANGELOG.md b/x/circuit/CHANGELOG.md index 6047ce566e60..63534e456362 100644 --- a/x/circuit/CHANGELOG.md +++ b/x/circuit/CHANGELOG.md @@ -29,8 +29,4 @@ Ref: https://keepachangelog.com/en/1.0.0/ # Changelog -## [Unreleased] - -### API Breaking - -* (x/circuit) [#16415](https://github.com/cosmos/cosmos-sdk/issues/16415) `NewKeeper` now takes a `KVStoreService` instead of a `StoreKey` and requires a codec. Methods in the `Keeper` now take a `context.Context` instead of a `sdk.Context` and return an `error`. Migrated to collections. `IsAllowed` from the `CircuitBreaker` interface now takes a `context.Context` instead of a `sdk.Context` and returns `bool, error`. \ No newline at end of file +## [Unreleased] \ No newline at end of file From 30c159fd7fd62441489b9baf0e15588926a89c13 Mon Sep 17 00:00:00 2001 From: Facundo Medica Date: Wed, 7 Jun 2023 09:24:18 +0200 Subject: [PATCH 13/14] lint --- x/circuit/keeper/query.go | 1 + 1 file changed, 1 insertion(+) diff --git a/x/circuit/keeper/query.go b/x/circuit/keeper/query.go index f633c104ec42..3c889206a561 100644 --- a/x/circuit/keeper/query.go +++ b/x/circuit/keeper/query.go @@ -49,6 +49,7 @@ func (qs QueryServer) Accounts(ctx context.Context, req *types.QueryAccountsRequ } for _, result := range results { + result := result address, err := qs.keeper.addressCodec.BytesToString(result.Key) if err != nil { return nil, err From 7fff67ea72693762205b61f9706cf93d2efdf760 Mon Sep 17 00:00:00 2001 From: Facundo Medica Date: Wed, 7 Jun 2023 11:30:40 +0200 Subject: [PATCH 14/14] fixes and suggestions --- x/circuit/keeper/genesis.go | 4 +++- x/circuit/keeper/keeper.go | 12 +++--------- x/circuit/keeper/keeper_test.go | 4 ++-- x/circuit/keeper/msg_server.go | 4 ++-- x/circuit/keeper/query_test.go | 2 +- 5 files changed, 11 insertions(+), 15 deletions(-) diff --git a/x/circuit/keeper/genesis.go b/x/circuit/keeper/genesis.go index 6ae64280d5fd..50fd6af67c5f 100644 --- a/x/circuit/keeper/genesis.go +++ b/x/circuit/keeper/genesis.go @@ -60,6 +60,8 @@ func (k *Keeper) InitGenesis(ctx context.Context, genState *types.GenesisState) } for _, url := range genState.DisabledTypeUrls { // Set the disabled type urls - k.DisableMsg(ctx, url) + if err := k.DisableList.Set(ctx, url); err != nil { + panic(err) + } } } diff --git a/x/circuit/keeper/keeper.go b/x/circuit/keeper/keeper.go index 79aebabca2ae..e095a89c8b3f 100644 --- a/x/circuit/keeper/keeper.go +++ b/x/circuit/keeper/keeper.go @@ -20,8 +20,10 @@ type Keeper struct { addressCodec address.Codec - Schema collections.Schema + Schema collections.Schema + // Permissions contains the permissions for each account Permissions collections.Map[[]byte, types.Permissions] + // DisableList contains the message URLs that are disabled DisableList collections.KeySet[string] } @@ -71,11 +73,3 @@ func (k *Keeper) IsAllowed(ctx context.Context, msgURL string) (bool, error) { has, err := k.DisableList.Has(ctx, msgURL) return !has, err } - -func (k *Keeper) DisableMsg(ctx context.Context, msgURL string) error { - return k.DisableList.Set(ctx, msgURL) -} - -func (k *Keeper) EnableMsg(ctx context.Context, msgURL string) error { - return k.DisableList.Remove(ctx, msgURL) -} diff --git a/x/circuit/keeper/keeper_test.go b/x/circuit/keeper/keeper_test.go index 1a8342b03fe9..f9f42b9b35c0 100644 --- a/x/circuit/keeper/keeper_test.go +++ b/x/circuit/keeper/keeper_test.go @@ -129,7 +129,7 @@ func TestIterateDisabledList(t *testing.T) { } for _, url := range mockMsgs { - f.keeper.DisableMsg(f.ctx, url) + require.NoError(t, f.keeper.DisableList.Set(f.ctx, url)) } // Define a variable to store the returned disabled URLs @@ -147,7 +147,7 @@ func TestIterateDisabledList(t *testing.T) { require.Equal(t, mockMsgs[2], returnedDisabled[2]) // re-enable mockMsgs[0] - f.keeper.EnableMsg(f.ctx, mockMsgs[0]) + require.NoError(t, f.keeper.DisableList.Remove(f.ctx, mockMsgs[0])) returnedDisabled = []string{} err = f.keeper.DisableList.Walk(f.ctx, nil, func(msgUrl string) (bool, error) { diff --git a/x/circuit/keeper/msg_server.go b/x/circuit/keeper/msg_server.go index 99766c49887d..ec5ccf6a8312 100644 --- a/x/circuit/keeper/msg_server.go +++ b/x/circuit/keeper/msg_server.go @@ -38,7 +38,7 @@ func (srv msgServer) AuthorizeCircuitBreaker(ctx context.Context, msg *types.Msg // Check that the authorizer has the permission level of "super admin" perms, err := srv.Permissions.Get(ctx, address) if err != nil { - return nil, fmt.Errorf("user permission does not exist %w", err) + return nil, err } if perms.Level != types.Permissions_LEVEL_SUPER_ADMIN { @@ -161,7 +161,7 @@ func (srv msgServer) ResetCircuitBreaker(ctx context.Context, msg *types.MsgRese // Get the permissions for the account specified in the msg.Authority field perms, err := keeper.Permissions.Get(ctx, address) if err != nil && !errorsmod.IsOf(err, collections.ErrNotFound) { - return nil, fmt.Errorf("user permission does not exist %w", err) + return nil, err } if perms.Level == types.Permissions_LEVEL_SUPER_ADMIN || perms.Level == types.Permissions_LEVEL_ALL_MSGS || perms.Level == types.Permissions_LEVEL_SOME_MSGS || bytes.Equal(address, srv.GetAuthority()) { diff --git a/x/circuit/keeper/query_test.go b/x/circuit/keeper/query_test.go index 825778931555..c6b22c32fb8d 100644 --- a/x/circuit/keeper/query_test.go +++ b/x/circuit/keeper/query_test.go @@ -63,7 +63,7 @@ func TestQueryDisabledList(t *testing.T) { t.Parallel() f := initFixture(t) - f.keeper.DisableMsg(f.ctx, f.mockMsgURL) + require.NoError(t, f.keeper.DisableList.Set(f.ctx, f.mockMsgURL)) // create a new query server qs := keeper.NewQueryServer(f.keeper)