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

* 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: # e2e/testsuite/codec.go # e2e/testsuite/tx.go # modules/apps/27-interchain-accounts/host/keeper/keeper.go # modules/apps/27-interchain-accounts/host/keeper/keeper_test.go # modules/apps/27-interchain-accounts/host/keeper/msg_server.go # modules/apps/27-interchain-accounts/host/keeper/msg_server_test.go # modules/apps/27-interchain-accounts/host/types/codec.go # modules/apps/27-interchain-accounts/host/types/codec_test.go # modules/apps/27-interchain-accounts/host/types/host.pb.go # modules/apps/27-interchain-accounts/host/types/msgs.go # modules/apps/27-interchain-accounts/host/types/tx.pb.go # modules/apps/callbacks/testing/simapp/app.go # modules/light-clients/08-wasm/testing/simapp/app.go # proto/ibc/applications/interchain_accounts/host/v1/tx.proto # testing/simapp/app.go * fix conflicts * fix tests * gofumpt * do not set ica query router for callbacks * pls * fix comment * panic if query router is nil * break loop earlier * workaround for using module safe queries of x/staking * add comment --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
cosmos · May 2, 2024 · b78afb0 · b78afb0
1 parent 47cf563
commit b78afb0
Show file tree

Hide file tree

Showing 17 changed files with 1,538 additions and 23 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -49,6 +49,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 ### Features
 
 * (apps/27-interchain-accounts) [\#5633](https://github.com/cosmos/ibc-go/pull/5633) Allow new ICA channels to use unordered ordering.
+* (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
@@ -1,9 +1,16 @@
 package keeper
 
 import (
+	"errors"
 	"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"
 	"github.com/cometbft/cometbft/libs/log"
 
@@ -36,7 +43,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
 }
 
 // NewKeeper creates a new interchain accounts host Keeper instance
@@ -65,6 +76,7 @@ func NewKeeper(
 		accountKeeper: accountKeeper,
 		scopedKeeper:  scopedKeeper,
 		msgRouter:     msgRouter,
+		mqsAllowList:  newModuleQuerySafeAllowList(),
 	}
 }
 
@@ -75,6 +87,18 @@ 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 (k Keeper) Logger(ctx sdk.Context) log.Logger {
 	return ctx.Logger().With("module", fmt.Sprintf("x/%s-%s", exported.ModuleName, icatypes.ModuleName))
@@ -225,3 +249,40 @@ func (k Keeper) SetInterchainAccountAddress(ctx sdk.Context, connectionID, portI
 	store := ctx.KVStore(k.storeKey)
 	store.Set(icatypes.KeyOwnerAccount(portID, connectionID), []byte(address))
 }
+
+// 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
@@ -6,7 +6,11 @@ import (
 
 	"github.com/stretchr/testify/suite"
 
+	authkeeper "github.com/cosmos/cosmos-sdk/x/auth/keeper"
+
 	genesistypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/genesis/types"
+	"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host/keeper"
+	"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host/types"
 	icatypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/types"
 	ibcfeekeeper "github.com/cosmos/ibc-go/v7/modules/apps/29-fee/keeper"
 	channelkeeper "github.com/cosmos/ibc-go/v7/modules/core/04-channel/keeper"
@@ -143,6 +147,80 @@ func (suite *KeeperTestSuite) TestIsBound() {
 	suite.Require().True(isBound)
 }
 
+func (suite *KeeperTestSuite) TestNewKeeper() {
+	testCases := []struct {
+		name          string
+		instantiateFn func()
+		expPass       bool
+	}{
+		{"success", func() {
+			keeper.NewKeeper(
+				suite.chainA.GetSimApp().AppCodec(),
+				suite.chainA.GetSimApp().GetKey(types.StoreKey),
+				suite.chainA.GetSimApp().GetSubspace(types.SubModuleName),
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				&suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
+				suite.chainA.GetSimApp().AccountKeeper,
+				suite.chainA.GetSimApp().ScopedICAHostKeeper,
+				suite.chainA.GetSimApp().MsgServiceRouter(),
+			)
+		}, true},
+		{"failure: interchain accounts module account does not exist", func() {
+			keeper.NewKeeper(
+				suite.chainA.GetSimApp().AppCodec(),
+				suite.chainA.GetSimApp().GetKey(types.StoreKey),
+				suite.chainA.GetSimApp().GetSubspace(types.SubModuleName),
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				&suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
+				authkeeper.AccountKeeper{}, // empty account keeper
+				suite.chainA.GetSimApp().ScopedICAHostKeeper,
+				suite.chainA.GetSimApp().MsgServiceRouter(),
+			)
+		}, false},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		suite.SetupTest()
+
+		suite.Run(tc.name, func() {
+			if tc.expPass {
+				suite.Require().NotPanics(
+					tc.instantiateFn,
+				)
+			} else {
+				suite.Require().Panics(
+					tc.instantiateFn,
+				)
+			}
+		})
+	}
+}
+
+func (suite *KeeperTestSuite) TestNewModuleQuerySafeAllowList() {
+	// Currently, all queries in bank, staking and auth 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.auth.v1beta1.Query/Accounts")
+	suite.Require().Contains(allowList, "/cosmos.auth.v1beta1.Query/ModuleAccountByName")
+	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
@@ -0,0 +1,71 @@
+package keeper
+
+import (
+	"context"
+
+	errorsmod "cosmossdk.io/errors"
+
+	_ "cosmossdk.io/api/cosmos/staking/v1beta1" // workaround to successfully retrieve staking module safe queries
+	sdk "github.com/cosmos/cosmos-sdk/types"
+
+	abci "github.com/cometbft/cometbft/abci/types"
+
+	"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host/types"
+	ibcerrors "github.com/cosmos/ibc-go/v7/modules/core/errors"
+)
+
+var _ types.MsgServer = (*msgServer)(nil)
+
+type msgServer struct {
+	*Keeper
+}
+
+// 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 {
+		var isModuleQuerySafe bool
+		for _, allowedQueryPath := range m.mqsAllowList {
+			if allowedQueryPath == query.Path {
+				isModuleQuerySafe = true
+				break
+			}
+		}
+		if !isModuleQuerySafe {
+			return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidRequest, "not module query safe: %s", query.Path)
+		}
+
+		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.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
+}