From 45e9c102e1614cc6517f48b757af0780fbf73f01 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 4 Mar 2024 13:16:20 +0100 Subject: [PATCH 1/4] refactor(x/authz): use router service --- core/router/service.go | 2 +- runtime/router.go | 24 +++++++++++++++--------- x/authz/CHANGELOG.md | 13 +++++-------- x/authz/keeper/genesis_test.go | 4 ++-- x/authz/keeper/keeper.go | 31 ++++--------------------------- x/authz/keeper/keeper_test.go | 4 ++-- x/authz/keeper/msg_server.go | 4 ++-- x/authz/module/abci_test.go | 4 ++-- x/authz/module/depinject.go | 14 ++++++-------- 9 files changed, 39 insertions(+), 61 deletions(-) diff --git a/core/router/service.go b/core/router/service.go index 4ab582085b20..d4f13d21b2ce 100644 --- a/core/router/service.go +++ b/core/router/service.go @@ -16,7 +16,7 @@ type Service interface { // Router is the interface that wraps the basic methods for a router. type Router interface { // CanInvoke returns an error if the given request cannot be invoked. - CanInvoke(ctx context.Context, req protoiface.MessageV1) error + CanInvoke(ctx context.Context, typeURL string) error // InvokeTyped execute a message or query. It should be used when the called knows the type of the response. InvokeTyped(ctx context.Context, req, res protoiface.MessageV1) error // InvokeUntyped execute a message or query. It should be used when the called doesn't know the type of the response. diff --git a/runtime/router.go b/runtime/router.go index 972949ad8b98..441ad935e479 100644 --- a/runtime/router.go +++ b/runtime/router.go @@ -54,11 +54,14 @@ type msgRouterService struct { } // CanInvoke returns an error if the given message cannot be invoked. -func (m *msgRouterService) CanInvoke(ctx context.Context, msg protoiface.MessageV1) error { - messageName := msgTypeURL(msg) - handler := m.router.HybridHandlerByMsgName(messageName) +func (m *msgRouterService) CanInvoke(ctx context.Context, typeURL string) error { + if typeURL == "" { + return fmt.Errorf("missing type url") + } + + handler := m.router.HybridHandlerByMsgName(typeURL) if handler == nil { - return fmt.Errorf("unknown message: %s", messageName) + return fmt.Errorf("unknown message: %s", typeURL) } return nil @@ -106,13 +109,16 @@ type queryRouterService struct { } // CanInvoke returns an error if the given request cannot be invoked. -func (m *queryRouterService) CanInvoke(ctx context.Context, req protoiface.MessageV1) error { - reqName := msgTypeURL(req) - handlers := m.router.HybridHandlerByRequestName(reqName) +func (m *queryRouterService) CanInvoke(ctx context.Context, typeURL string) error { + if typeURL == "" { + return fmt.Errorf("missing type url") + } + + handlers := m.router.HybridHandlerByRequestName(typeURL) if len(handlers) == 0 { - return fmt.Errorf("unknown request: %s", reqName) + return fmt.Errorf("unknown request: %s", typeURL) } else if len(handlers) > 1 { - return fmt.Errorf("ambiguous request, query have multiple handlers: %s", reqName) + return fmt.Errorf("ambiguous request, query have multiple handlers: %s", typeURL) } return nil diff --git a/x/authz/CHANGELOG.md b/x/authz/CHANGELOG.md index 4954a50128b5..1418695e8160 100644 --- a/x/authz/CHANGELOG.md +++ b/x/authz/CHANGELOG.md @@ -25,20 +25,17 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## [Unreleased] -### Consens Breaking Changes - -* [#19188](https://github.com/cosmos/cosmos-sdk/pull/19188) Remove creation of `BaseAccount` when sending a message to an account that does not exist - ### Features * [#18737](https://github.com/cosmos/cosmos-sdk/pull/18737) Added a limit of 200 grants pruned per `BeginBlock` and the `PruneExpiredGrants` message that prunes 75 expired grants on every run. -### Improvements - ### API Breaking Changes -* [#19490](https://github.com/cosmos/cosmos-sdk/pull/19490) `appmodule.Environment` is received on the Keeper to get access to different application services +* []() `NewKeeper` doesn't take a message router anymore. Set the message router in the `appmodule.Environment` instead. +* [#19490](https://github.com/cosmos/cosmos-sdk/pull/19490) `appmodule.Environment` is received on the Keeper to get access to different application services. * [#18737](https://github.com/cosmos/cosmos-sdk/pull/18737) Update the keeper method `DequeueAndDeleteExpiredGrants` to take a limit argument for the number of grants to prune. * [#16509](https://github.com/cosmos/cosmos-sdk/pull/16509) `AcceptResponse` has been moved to sdk/types/authz and the `Updated` field is now of the type `sdk.Msg` instead of `authz.Authorization`. -### Bug Fixes +### Consens Breaking Changes + +* [#19188](https://github.com/cosmos/cosmos-sdk/pull/19188) Remove creation of `BaseAccount` when sending a message to an account that does not exist. diff --git a/x/authz/keeper/genesis_test.go b/x/authz/keeper/genesis_test.go index e3ced43a0eb5..8ae9811c019c 100644 --- a/x/authz/keeper/genesis_test.go +++ b/x/authz/keeper/genesis_test.go @@ -47,7 +47,6 @@ func (suite *GenesisTestSuite) SetupTest() { storeService := runtime.NewKVStoreService(key) testCtx := testutil.DefaultContextWithDB(suite.T(), key, storetypes.NewTransientStoreKey("transient_test")) suite.ctx = testCtx.Ctx.WithHeaderInfo(header.Info{Height: 1}) - env := runtime.NewEnvironment(storeService, log.NewNopLogger()) suite.encCfg = moduletestutil.MakeTestEncodingConfig(authzmodule.AppModule{}) @@ -68,8 +67,9 @@ func (suite *GenesisTestSuite) SetupTest() { msr := suite.baseApp.MsgServiceRouter() msr.SetInterfaceRegistry(suite.encCfg.InterfaceRegistry) + env := runtime.NewEnvironment(storeService, log.NewNopLogger(), runtime.EnvWithRouterService(nil, msr)) - suite.keeper = keeper.NewKeeper(env, suite.encCfg.Codec, msr, suite.accountKeeper) + suite.keeper = keeper.NewKeeper(env, suite.encCfg.Codec, suite.accountKeeper) } func (suite *GenesisTestSuite) TestImportExportGenesis() { diff --git a/x/authz/keeper/keeper.go b/x/authz/keeper/keeper.go index 05546384281f..2613e4085327 100644 --- a/x/authz/keeper/keeper.go +++ b/x/authz/keeper/keeper.go @@ -4,19 +4,16 @@ import ( "bytes" "context" "fmt" - "strconv" "time" "github.com/cosmos/gogoproto/proto" "cosmossdk.io/core/appmodule" - "cosmossdk.io/core/event" errorsmod "cosmossdk.io/errors" "cosmossdk.io/log" storetypes "cosmossdk.io/store/types" "cosmossdk.io/x/authz" - "github.com/cosmos/cosmos-sdk/baseapp" "github.com/cosmos/cosmos-sdk/codec" codectypes "github.com/cosmos/cosmos-sdk/codec/types" "github.com/cosmos/cosmos-sdk/runtime" @@ -32,16 +29,14 @@ const gasCostPerIteration = uint64(20) type Keeper struct { environment appmodule.Environment cdc codec.Codec - router baseapp.MessageRouter authKeeper authz.AccountKeeper } // NewKeeper constructs a message authorization Keeper -func NewKeeper(env appmodule.Environment, cdc codec.Codec, router baseapp.MessageRouter, ak authz.AccountKeeper) Keeper { +func NewKeeper(env appmodule.Environment, cdc codec.Codec, ak authz.AccountKeeper) Keeper { return Keeper{ environment: env, cdc: cdc, - router: router, authKeeper: ak, } } @@ -151,29 +146,11 @@ func (k Keeper) DispatchActions(ctx context.Context, grantee sdk.AccAddress, msg } } - handler := k.router.Handler(msg) - if handler == nil { - return nil, sdkerrors.ErrUnknownRequest.Wrapf("unrecognized message route: %s", sdk.MsgTypeURL(msg)) - } - - msgResp, err := handler(sdkCtx, msg) + // no need to use the branch service here, as if the transaction fails, the transaction will be reverted + _, err = k.environment.RouterService.MessageRouterService().InvokeUntyped(ctx, msg) if err != nil { - return nil, errorsmod.Wrapf(err, "failed to execute message; message %v", msg) + return nil, fmt.Errorf("failed to execute message %d; message %v: %w", i, msg, err) } - - results[i] = msgResp.Data - - // emit the events from the dispatched actions - for i = range msgResp.Events { - err := k.environment.EventService.EventManager(ctx).EmitKV( - "dispatch actions", - event.NewAttribute("authz_msg_index", strconv.Itoa(i)), - ) - if err != nil { - return nil, err - } - } - } return results, nil diff --git a/x/authz/keeper/keeper_test.go b/x/authz/keeper/keeper_test.go index b984aeac2a1a..444787fcdf7d 100644 --- a/x/authz/keeper/keeper_test.go +++ b/x/authz/keeper/keeper_test.go @@ -52,7 +52,6 @@ func (s *TestSuite) SetupTest() { testCtx := testutil.DefaultContextWithDB(s.T(), key, storetypes.NewTransientStoreKey("transient_test")) s.ctx = testCtx.Ctx.WithHeaderInfo(header.Info{Time: time.Now().Round(0).UTC()}) s.encCfg = moduletestutil.MakeTestEncodingConfig(authzmodule.AppModule{}) - env := runtime.NewEnvironment(storeService, log.NewNopLogger()) s.baseApp = baseapp.NewBaseApp( "authz", @@ -75,7 +74,8 @@ func (s *TestSuite) SetupTest() { banktypes.RegisterInterfaces(s.encCfg.InterfaceRegistry) banktypes.RegisterMsgServer(s.baseApp.MsgServiceRouter(), s.bankKeeper) - s.authzKeeper = authzkeeper.NewKeeper(env, s.encCfg.Codec, s.baseApp.MsgServiceRouter(), s.accountKeeper) + env := runtime.NewEnvironment(storeService, log.NewNopLogger(), runtime.EnvWithRouterService(s.baseApp.GRPCQueryRouter(), s.baseApp.MsgServiceRouter())) + s.authzKeeper = authzkeeper.NewKeeper(env, s.encCfg.Codec, s.accountKeeper) queryHelper := baseapp.NewQueryServerTestHelper(s.ctx, s.encCfg.InterfaceRegistry) authz.RegisterQueryServer(queryHelper, s.authzKeeper) diff --git a/x/authz/keeper/msg_server.go b/x/authz/keeper/msg_server.go index 1a3e5f29f047..d6dad9d6f71d 100644 --- a/x/authz/keeper/msg_server.go +++ b/x/authz/keeper/msg_server.go @@ -40,8 +40,8 @@ func (k Keeper) Grant(ctx context.Context, msg *authz.MsgGrant) (*authz.MsgGrant } t := authorization.MsgTypeURL() - if k.router.HandlerByTypeURL(t) == nil { - return nil, sdkerrors.ErrInvalidType.Wrapf("%s doesn't exist.", t) + if k.environment.RouterService.MessageRouterService().CanInvoke(ctx, t) == nil { + return nil, sdkerrors.ErrInvalidType.Wrapf("%s doesn't exist", t) } err = k.SaveGrant(ctx, grantee, granter, authorization, msg.Grant.Expiration) diff --git a/x/authz/module/abci_test.go b/x/authz/module/abci_test.go index f0494f267c84..f45f7e004974 100644 --- a/x/authz/module/abci_test.go +++ b/x/authz/module/abci_test.go @@ -32,7 +32,6 @@ func TestExpiredGrantsQueue(t *testing.T) { testCtx := testutil.DefaultContextWithDB(t, key, storetypes.NewTransientStoreKey("transient_test")) encCfg := moduletestutil.MakeTestEncodingConfig(authzmodule.AppModule{}) ctx := testCtx.Ctx - env := runtime.NewEnvironment(storeService, log.NewNopLogger()) baseApp := baseapp.NewBaseApp( "authz", @@ -66,7 +65,8 @@ func TestExpiredGrantsQueue(t *testing.T) { accountKeeper.EXPECT().AddressCodec().Return(address.NewBech32Codec("cosmos")).AnyTimes() - authzKeeper := keeper.NewKeeper(env, encCfg.Codec, baseApp.MsgServiceRouter(), accountKeeper) + env := runtime.NewEnvironment(storeService, log.NewNopLogger(), runtime.EnvWithRouterService(baseApp.GRPCQueryRouter(), baseApp.MsgServiceRouter())) + authzKeeper := keeper.NewKeeper(env, encCfg.Codec, accountKeeper) save := func(grantee sdk.AccAddress, exp *time.Time) { err := authzKeeper.SaveGrant(ctx, grantee, granter, sendAuthz, exp) diff --git a/x/authz/module/depinject.go b/x/authz/module/depinject.go index e3b11f50fe5c..849471f8fa27 100644 --- a/x/authz/module/depinject.go +++ b/x/authz/module/depinject.go @@ -8,7 +8,6 @@ import ( "cosmossdk.io/x/authz" "cosmossdk.io/x/authz/keeper" - "github.com/cosmos/cosmos-sdk/baseapp" "github.com/cosmos/cosmos-sdk/codec" cdctypes "github.com/cosmos/cosmos-sdk/codec/types" ) @@ -28,12 +27,11 @@ func init() { type ModuleInputs struct { depinject.In - Cdc codec.Codec - AccountKeeper authz.AccountKeeper - BankKeeper authz.BankKeeper - Registry cdctypes.InterfaceRegistry - MsgServiceRouter baseapp.MessageRouter - Environment appmodule.Environment + Cdc codec.Codec + AccountKeeper authz.AccountKeeper + BankKeeper authz.BankKeeper + Registry cdctypes.InterfaceRegistry + Environment appmodule.Environment } type ModuleOutputs struct { @@ -44,7 +42,7 @@ type ModuleOutputs struct { } func ProvideModule(in ModuleInputs) ModuleOutputs { - k := keeper.NewKeeper(in.Environment, in.Cdc, in.MsgServiceRouter, in.AccountKeeper) + k := keeper.NewKeeper(in.Environment, in.Cdc, in.AccountKeeper) m := NewAppModule(in.Cdc, k, in.AccountKeeper, in.BankKeeper, in.Registry) return ModuleOutputs{AuthzKeeper: k, Module: m} } From 9ec85f06788d2706792d75614474389c8bc47f62 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 4 Mar 2024 13:18:29 +0100 Subject: [PATCH 2/4] changelog --- x/authz/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/authz/CHANGELOG.md b/x/authz/CHANGELOG.md index 1418695e8160..0ea558d5d11e 100644 --- a/x/authz/CHANGELOG.md +++ b/x/authz/CHANGELOG.md @@ -31,7 +31,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### API Breaking Changes -* []() `NewKeeper` doesn't take a message router anymore. Set the message router in the `appmodule.Environment` instead. +* [#19637](https://github.com/cosmos/cosmos-sdk/pull/19637) `NewKeeper` doesn't take a message router anymore. Set the message router in the `appmodule.Environment` instead. * [#19490](https://github.com/cosmos/cosmos-sdk/pull/19490) `appmodule.Environment` is received on the Keeper to get access to different application services. * [#18737](https://github.com/cosmos/cosmos-sdk/pull/18737) Update the keeper method `DequeueAndDeleteExpiredGrants` to take a limit argument for the number of grants to prune. * [#16509](https://github.com/cosmos/cosmos-sdk/pull/16509) `AcceptResponse` has been moved to sdk/types/authz and the `Updated` field is now of the type `sdk.Msg` instead of `authz.Authorization`. From b6e6022c6cc64fe9fb4aeaa7804b47d9684eb2ca Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 4 Mar 2024 13:20:28 +0100 Subject: [PATCH 3/4] app legacy wiring --- simapp/app.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/simapp/app.go b/simapp/app.go index 2c1e47b95bf5..87cc1be99684 100644 --- a/simapp/app.go +++ b/simapp/app.go @@ -349,7 +349,7 @@ func NewSimApp( app.CircuitKeeper = circuitkeeper.NewKeeper(runtime.NewEnvironment(runtime.NewKVStoreService(keys[circuittypes.StoreKey]), logger), appCodec, authtypes.NewModuleAddress(govtypes.ModuleName).String(), app.AuthKeeper.AddressCodec()) app.BaseApp.SetCircuitBreaker(&app.CircuitKeeper) - app.AuthzKeeper = authzkeeper.NewKeeper(runtime.NewEnvironment(runtime.NewKVStoreService(keys[authzkeeper.StoreKey]), logger), appCodec, app.MsgServiceRouter(), app.AuthKeeper) + app.AuthzKeeper = authzkeeper.NewKeeper(runtime.NewEnvironment(runtime.NewKVStoreService(keys[authzkeeper.StoreKey]), logger, runtime.EnvWithRouterService(app.GRPCQueryRouter(), app.MsgServiceRouter())), appCodec, app.AuthKeeper) groupConfig := group.DefaultConfig() /* From b43128d50289aec35b1ebd6913802c12b4a1b8e8 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 4 Mar 2024 15:09:58 +0100 Subject: [PATCH 4/4] feedback Co-authored-by: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com> --- x/authz/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/authz/CHANGELOG.md b/x/authz/CHANGELOG.md index 0ea558d5d11e..0e4e5795d1c1 100644 --- a/x/authz/CHANGELOG.md +++ b/x/authz/CHANGELOG.md @@ -36,6 +36,6 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [#18737](https://github.com/cosmos/cosmos-sdk/pull/18737) Update the keeper method `DequeueAndDeleteExpiredGrants` to take a limit argument for the number of grants to prune. * [#16509](https://github.com/cosmos/cosmos-sdk/pull/16509) `AcceptResponse` has been moved to sdk/types/authz and the `Updated` field is now of the type `sdk.Msg` instead of `authz.Authorization`. -### Consens Breaking Changes +### Consensus Breaking Changes * [#19188](https://github.com/cosmos/cosmos-sdk/pull/19188) Remove creation of `BaseAccount` when sending a message to an account that does not exist.