feat: allow module safe queries in ICA (backport #5785) (#6065)

* feat: allow module safe queries in ICA (#5785) * imp: initial modification of tx.proto * imp: ran 'make proto-all' * fix: compiler errors * imp: added query router interface * imp: added queryRouter to icahost keeper * imp: improved proto definitions * imp: ran 'make proto-all' * imp: added sdk.Msg helpers * feat: basic implementation * style: improved field names * imp: ran 'make proto-all' * fix: compiler errors * imp: ran gofumpt * feat: tests passing * feat: tests improved * test: removed unneeded code * imp: improved 'IsModuleSafe' function * imp: added IsModuleQuerySafe to msg_server * imp: added more test cases * fix: callbacks compiler * fix: non determinancy issues * fix: added query msg to codec * imp: whitelist logic added * e2e: new test added * fix: new test * fix: test * fix: e2e * fix: e2e * imp(e2e): added the QueryTxsByEvents function * fix: e2e * e2e: lint fix * fix: e2e * e2e: debug * fix: e2e * test: debug helpers * debug * test: added codec_test case * imp: additional test case * imp: added important unit test * r4r * e2e: debug * imp: added logs * fix: e2e * fix: e2e * fix: e2e * imp: added height to proto response * imp: ran 'make proto-all' * imp: added height * e2e: updated e2e to test height * imp: review suggestions * e2e: remove unneeded log * refactor: refactored 'ExtractValueFromEvents' * e2e: review item * imp: review item * nit: review item * docs: added godocs * test: unit test for mqsWhitelist added * imp: added logging * style: rename to allow list * add changelog --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit eecfa5c) # Conflicts: # CHANGELOG.md # e2e/testsuite/codec.go # e2e/testsuite/tx.go # modules/apps/27-interchain-accounts/host/types/msgs.go # modules/light-clients/08-wasm/testing/simapp/app.go * fix conflicts + add GetSigners to message * delete e2e and 08-wasm folders * add tests for message functions * use boolean for expected test result * revert API breaking change in ica host NewKeeper * lint * panic if query router is nil --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Damian Nolan <damiannolan@gmail.com>
cosmos · Apr 11, 2024 · 759897b · 759897b
1 parent da9f8db
commit 759897b
Show file tree

Hide file tree

Showing 18 changed files with 1,225 additions and 45 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -49,6 +49,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (core) [\#6055](https://github.com/cosmos/ibc-go/pull/6055) Introduce a new interface `ConsensusHost` used to validate an IBC `ClientState` and `ConsensusState` against the host chain's underlying consensus parameters.
 * (core/02-client) [\#5821](https://github.com/cosmos/ibc-go/pull/5821) Add rpc `VerifyMembershipProof` (querier approach for conditional clients).
 * (core/04-channel) [\#5788](https://github.com/cosmos/ibc-go/pull/5788) Add `NewErrorAcknowledgementWithCodespace` to allow codespaces in ack errors.
+* (apps/27-interchain-accounts) [\#5785](https://github.com/cosmos/ibc-go/pull/5785) Introduce a new tx message that ICA host submodule can use to query the chain (only those marked with `module_query_safe`) and write the responses to the acknowledgement.
 
 ### Bug Fixes
 

diff --git a/modules/apps/27-interchain-accounts/host/keeper/export_test.go b/modules/apps/27-interchain-accounts/host/keeper/export_test.go
@@ -20,3 +20,8 @@ func (k *Keeper) GetICS4Wrapper() porttypes.ICS4Wrapper {
 func (k Keeper) GetAppMetadata(ctx sdk.Context, portID, channelID string) (icatypes.Metadata, error) {
 	return k.getAppMetadata(ctx, portID, channelID)
 }
+
+// NewModuleQuerySafeAllowList is a wrapper around newModuleQuerySafeAllowList to allow the function to be directly called in tests.
+func NewModuleQuerySafeAllowList() []string {
+	return newModuleQuerySafeAllowList()
+}
diff --git a/modules/apps/27-interchain-accounts/host/keeper/keeper.go b/modules/apps/27-interchain-accounts/host/keeper/keeper.go
@@ -5,6 +5,12 @@ import (
 	"fmt"
 	"strings"
 
+	gogoproto "github.com/cosmos/gogoproto/proto"
+	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/reflect/protoreflect"
+
+	msgv1 "cosmossdk.io/api/cosmos/msg/v1"
+	queryv1 "cosmossdk.io/api/cosmos/query/v1"
 	errorsmod "cosmossdk.io/errors"
 	"cosmossdk.io/log"
 	storetypes "cosmossdk.io/store/types"
@@ -36,7 +42,11 @@ type Keeper struct {
 
 	scopedKeeper exported.ScopedKeeper
 
-	msgRouter icatypes.MessageRouter
+	msgRouter   icatypes.MessageRouter
+	queryRouter icatypes.QueryRouter
+
+	// mqsAllowList is a list of all module safe query paths
+	mqsAllowList []string
 
 	// the address capable of executing a MsgUpdateParams message. Typically, this
 	// should be the x/gov module account.
@@ -69,17 +79,30 @@ func NewKeeper(
 		accountKeeper:  accountKeeper,
 		scopedKeeper:   scopedKeeper,
 		msgRouter:      msgRouter,
+		mqsAllowList:   newModuleQuerySafeAllowList(),
 		authority:      authority,
 	}
 }
 
 // WithICS4Wrapper sets the ICS4Wrapper. This function may be used after
-// the keepers creation to set the middleware which is above this module
+// the keeper's creation to set the middleware which is above this module
 // in the IBC application stack.
 func (k *Keeper) WithICS4Wrapper(wrapper porttypes.ICS4Wrapper) {
 	k.ics4Wrapper = wrapper
 }
 
+// WithQueryRouter sets the QueryRouter. This function may be used after
+// the keeper's creation to set the query router to which queries in the
+// ICA packet data will be routed to if they are module_safe_query.
+// Panics if the queryRouter is nil.
+func (k *Keeper) WithQueryRouter(queryRouter icatypes.QueryRouter) {
+	if queryRouter == nil {
+		panic(errors.New("cannot set a nil query router"))
+	}
+
+	k.queryRouter = queryRouter
+}
+
 // Logger returns the application logger, scoped to the associated module
 func (Keeper) Logger(ctx sdk.Context) log.Logger {
 	return ctx.Logger().With("module", fmt.Sprintf("x/%s-%s", exported.ModuleName, icatypes.ModuleName))
@@ -256,3 +279,40 @@ func (k Keeper) SetParams(ctx sdk.Context, params types.Params) {
 	bz := k.cdc.MustMarshal(&params)
 	store.Set([]byte(types.ParamsKey), bz)
 }
+
+// newModuleQuerySafeAllowList returns a list of all query paths labeled with module_query_safe in the proto files.
+func newModuleQuerySafeAllowList() []string {
+	protoFiles, err := gogoproto.MergedRegistry()
+	if err != nil {
+		panic(err)
+	}
+
+	allowList := []string{}
+	protoFiles.RangeFiles(func(fd protoreflect.FileDescriptor) bool {
+		for i := 0; i < fd.Services().Len(); i++ {
+			// Get the service descriptor
+			sd := fd.Services().Get(i)
+
+			// Skip services that are annotated with the "cosmos.msg.v1.service" option.
+			if ext := proto.GetExtension(sd.Options(), msgv1.E_Service); ext != nil && ext.(bool) {
+				continue
+			}
+
+			for j := 0; j < sd.Methods().Len(); j++ {
+				// Get the method descriptor
+				md := sd.Methods().Get(j)
+
+				// Skip methods that are not annotated with the "cosmos.query.v1.module_query_safe" option.
+				if ext := proto.GetExtension(md.Options(), queryv1.E_ModuleQuerySafe); ext == nil || !ext.(bool) {
+					continue
+				}
+
+				// Add the method to the whitelist
+				allowList = append(allowList, fmt.Sprintf("/%s/%s", sd.FullName(), md.Name()))
+			}
+		}
+		return true
+	})
+
+	return allowList
+}
diff --git a/modules/apps/27-interchain-accounts/host/keeper/keeper_test.go b/modules/apps/27-interchain-accounts/host/keeper/keeper_test.go
@@ -198,6 +198,31 @@ func (suite *KeeperTestSuite) TestNewKeeper() {
 	}
 }
 
+func (suite *KeeperTestSuite) TestNewModuleQuerySafeAllowList() {
+	// Currently, all queries in bank, staking, auth, and circuit are marked safe
+	// Notably, the gov and distribution modules are not marked safe
+
+	var allowList []string
+	suite.Require().NotPanics(func() {
+		allowList = keeper.NewModuleQuerySafeAllowList()
+	})
+
+	suite.Require().NotEmpty(allowList)
+	suite.Require().Contains(allowList, "/cosmos.bank.v1beta1.Query/Balance")
+	suite.Require().Contains(allowList, "/cosmos.bank.v1beta1.Query/AllBalances")
+	suite.Require().Contains(allowList, "/cosmos.staking.v1beta1.Query/Validator")
+	suite.Require().Contains(allowList, "/cosmos.staking.v1beta1.Query/Validators")
+	suite.Require().Contains(allowList, "/cosmos.circuit.v1.Query/Account")
+	suite.Require().Contains(allowList, "/cosmos.circuit.v1.Query/DisabledList")
+	suite.Require().Contains(allowList, "/cosmos.auth.v1beta1.Query/Accounts")
+	suite.Require().Contains(allowList, "/cosmos.auth.v1beta1.Query/ModuleAccountByName")
+	suite.Require().Contains(allowList, "/ibc.core.client.v1.Query/VerifyMembership")
+	suite.Require().NotContains(allowList, "/cosmos.gov.v1beta1.Query/Proposals")
+	suite.Require().NotContains(allowList, "/cosmos.gov.v1.Query/Proposals")
+	suite.Require().NotContains(allowList, "/cosmos.distribution.v1beta1.Query/Params")
+	suite.Require().NotContains(allowList, "/cosmos.distribution.v1beta1.Query/DelegationRewards")
+}
+
 func (suite *KeeperTestSuite) TestGetInterchainAccountAddress() {
 	suite.SetupTest()
 

diff --git a/modules/apps/27-interchain-accounts/host/keeper/msg_server.go b/modules/apps/27-interchain-accounts/host/keeper/msg_server.go
@@ -2,11 +2,15 @@ package keeper
 
 import (
 	"context"
+	"errors"
+	"slices"
 
 	errorsmod "cosmossdk.io/errors"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
+	abci "github.com/cometbft/cometbft/abci/types"
+
 	"github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/types"
 	ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
 )
@@ -20,9 +24,51 @@ type msgServer struct {
 // NewMsgServerImpl returns an implementation of the ICS27 host MsgServer interface
 // for the provided Keeper.
 func NewMsgServerImpl(keeper *Keeper) types.MsgServer {
+	if keeper.queryRouter == nil {
+		panic("query router must not be nil")
+	}
 	return &msgServer{Keeper: keeper}
 }
 
+// ModuleQuerySafe routes the queries to the keeper's query router if they are module_query_safe.
+// This handler doesn't use the signer.
+func (m msgServer) ModuleQuerySafe(goCtx context.Context, msg *types.MsgModuleQuerySafe) (*types.MsgModuleQuerySafeResponse, error) {
+	ctx := sdk.UnwrapSDKContext(goCtx)
+
+	responses := make([][]byte, len(msg.Requests))
+	for i, query := range msg.Requests {
+		isModuleQuerySafe := slices.Contains(m.mqsAllowList, query.Path)
+		if !isModuleQuerySafe {
+			return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidRequest, "not module query safe: %s", query.Path)
+		}
+
+		if m.queryRouter == nil {
+			return nil, errors.New("query router must not be nil")
+		}
+
+		route := m.queryRouter.Route(query.Path)
+		if route == nil {
+			return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidRequest, "no route to query: %s", query.Path)
+		}
+
+		res, err := route(ctx, &abci.RequestQuery{
+			Path: query.Path,
+			Data: query.Data,
+		})
+		if err != nil {
+			m.Logger(ctx).Debug("query failed", "path", query.Path, "error", err)
+			return nil, err
+		}
+		if res == nil || res.Value == nil {
+			return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidRequest, "no response for query: %s", query.Path)
+		}
+
+		responses[i] = res.Value
+	}
+
+	return &types.MsgModuleQuerySafeResponse{Responses: responses, Height: uint64(ctx.BlockHeight())}, nil
+}
+
 // UpdateParams updates the host submodule's params.
 func (m msgServer) UpdateParams(goCtx context.Context, msg *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) {
 	if m.GetAuthority() != msg.Signer {

diff --git a/modules/apps/27-interchain-accounts/host/keeper/msg_server_test.go b/modules/apps/27-interchain-accounts/host/keeper/msg_server_test.go
@@ -1,10 +1,157 @@
 package keeper_test
 
 import (
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
+	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
+
 	"github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/keeper"
 	"github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/types"
+	transfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
+	ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
 )
 
+func (suite *KeeperTestSuite) TestModuleQuerySafe() {
+	var (
+		msg          *types.MsgModuleQuerySafe
+		expResponses [][]byte
+	)
+	testCases := []struct {
+		name     string
+		malleate func()
+		expErr   error
+	}{
+		{
+			"success",
+			func() {
+				balanceQueryBz, err := banktypes.NewQueryBalanceRequest(suite.chainA.SenderAccount.GetAddress(), sdk.DefaultBondDenom).Marshal()
+				suite.Require().NoError(err)
+
+				queryReq := types.QueryRequest{
+					Path: "/cosmos.bank.v1beta1.Query/Balance",
+					Data: balanceQueryBz,
+				}
+
+				msg = types.NewMsgModuleQuerySafe(suite.chainA.GetSimApp().ICAHostKeeper.GetAuthority(), []*types.QueryRequest{&queryReq})
+
+				balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), sdk.DefaultBondDenom)
+
+				expResp := banktypes.QueryBalanceResponse{Balance: &balance}
+				expRespBz, err := expResp.Marshal()
+				suite.Require().NoError(err)
+
+				expResponses = [][]byte{expRespBz}
+			},
+			nil,
+		},
+		{
+			"success: multiple queries",
+			func() {
+				balanceQueryBz, err := banktypes.NewQueryBalanceRequest(suite.chainA.SenderAccount.GetAddress(), sdk.DefaultBondDenom).Marshal()
+				suite.Require().NoError(err)
+
+				queryReq := types.QueryRequest{
+					Path: "/cosmos.bank.v1beta1.Query/Balance",
+					Data: balanceQueryBz,
+				}
+
+				paramsQuery := stakingtypes.QueryParamsRequest{}
+				paramsQueryBz, err := paramsQuery.Marshal()
+				suite.Require().NoError(err)
+
+				paramsQueryReq := types.QueryRequest{
+					Path: "/cosmos.staking.v1beta1.Query/Params",
+					Data: paramsQueryBz,
+				}
+
+				msg = types.NewMsgModuleQuerySafe(suite.chainA.GetSimApp().ICAHostKeeper.GetAuthority(), []*types.QueryRequest{&queryReq, &paramsQueryReq})
+
+				balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), sdk.DefaultBondDenom)
+
+				expResp := banktypes.QueryBalanceResponse{Balance: &balance}
+				expRespBz, err := expResp.Marshal()
+				suite.Require().NoError(err)
+
+				params, err := suite.chainA.GetSimApp().StakingKeeper.GetParams(suite.chainA.GetContext())
+				suite.Require().NoError(err)
+				expParamsResp := stakingtypes.QueryParamsResponse{Params: params}
+				expParamsRespBz, err := expParamsResp.Marshal()
+				suite.Require().NoError(err)
+
+				expResponses = [][]byte{expRespBz, expParamsRespBz}
+			},
+			nil,
+		},
+		{
+			"failure: not module query safe",
+			func() {
+				balanceQueryBz, err := banktypes.NewQueryBalanceRequest(suite.chainA.SenderAccount.GetAddress(), sdk.DefaultBondDenom).Marshal()
+				suite.Require().NoError(err)
+
+				queryReq := types.QueryRequest{
+					Path: "/cosmos.bank.v1beta1.Query/Balance",
+					Data: balanceQueryBz,
+				}
+
+				paramsQuery := transfertypes.QueryParamsRequest{}
+				paramsQueryBz, err := paramsQuery.Marshal()
+				suite.Require().NoError(err)
+
+				paramsQueryReq := types.QueryRequest{
+					Path: "/ibc.applications.transfer.v1.Query/Params",
+					Data: paramsQueryBz,
+				}
+
+				msg = types.NewMsgModuleQuerySafe(suite.chainA.GetSimApp().ICAHostKeeper.GetAuthority(), []*types.QueryRequest{&queryReq, &paramsQueryReq})
+			},
+			ibcerrors.ErrInvalidRequest,
+		},
+		{
+			"failure: invalid query path",
+			func() {
+				balanceQueryBz, err := banktypes.NewQueryBalanceRequest(suite.chainA.SenderAccount.GetAddress(), sdk.DefaultBondDenom).Marshal()
+				suite.Require().NoError(err)
+
+				queryReq := types.QueryRequest{
+					Path: "/cosmos.invalid.Query/Invalid",
+					Data: balanceQueryBz,
+				}
+
+				msg = types.NewMsgModuleQuerySafe(suite.chainA.GetSimApp().ICAHostKeeper.GetAuthority(), []*types.QueryRequest{&queryReq})
+			},
+			ibcerrors.ErrInvalidRequest,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+
+		suite.Run(tc.name, func() {
+			suite.SetupTest()
+
+			// reset
+			msg = nil
+			expResponses = nil
+
+			tc.malleate()
+
+			ctx := suite.chainA.GetContext()
+			msgServer := keeper.NewMsgServerImpl(&suite.chainA.GetSimApp().ICAHostKeeper)
+			res, err := msgServer.ModuleQuerySafe(ctx, msg)
+
+			if tc.expErr == nil {
+				suite.Require().NoError(err)
+				suite.Require().NotNil(res)
+
+				suite.Require().ElementsMatch(expResponses, res.Responses)
+			} else {
+				suite.Require().ErrorIs(err, tc.expErr)
+				suite.Require().Nil(res)
+			}
+		})
+	}
+}
+
 func (suite *KeeperTestSuite) TestUpdateParams() {
 	testCases := []struct {
 		name    string