Fix legacy querying tx ("unregistered type" bug) (#7992)

* Add tests for query txs

* Add test for IBC

* Refactor checkSignModeError

* Fix lint

* Add successful IBC test

* Remove logs

* break

* Remove known errors

* Update error message

* Better comments

* Revert variable renaming

* Fix lint

* Add mention of gRPC as TODO

docs/migrations/rest.md

@@ -16,6 +16,7 @@ Some important information concerning all legacy REST endpoints:
 
 | Legacy REST Endpoint      | Description        | Breaking Change                                                                                                                                                                                                                            |
 | ------------------------- | ------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
+| `POST /txs`               | Query tx by hash   | Endpoint will error when trying to broadcast transactions that don't support Amino serialization (e.g. IBC txs)<sup>1</sup>.                                                                                                               |
 | `GET /txs/{hash}`         | Query tx by hash   | Endpoint will error when trying to output transactions that don't support Amino serialization (e.g. IBC txs)<sup>1</sup>.                                                                                                                  |
 | `GET /txs`                | Query tx by events | Endpoint will error when trying to output transactions that don't support Amino serialization (e.g. IBC txs)<sup>1</sup>.                                                                                                                  |
 | `GET /staking/validators` | Get all validators | BondStatus is now a protobuf enum instead of an int32, and JSON serialized using its protobuf name, so expect query parameters like `?status=BOND_STATUS_{BONDED,UNBONDED,UNBONDING}` as opposed to `?status={bonded,unbonded,unbonding}`. |
@@ -87,3 +88,7 @@ Some modules expose legacy `POST` endpoints to generate unsigned transactions fo
 | `POST /upgrade/*`                                                               | Create unsigned `Msg`s for upgrade                                  | N/A, use Protobuf directly                                                                            |
 | `GET /upgrade/current`                                                          | Get the current plan                                                | `GET /cosmos/upgrade/v1beta1/current_plan`                                                            |
 | `GET /upgrade/applied_plan/{name}`                                              | Get a previously applied plan                                       | `GET /cosmos/upgrade/v1beta1/applied/{name}`                                                          |
+
+## Migrating to gRPC
+
+Instead of hitting REST endpoints as described in the previous paragraph, the SDK also exposes a gRPC server. Any client can use gRPC instead of REST to interact with the node. An overview of different ways to communicate with a node can be found [here (TODO)](https://github.com/cosmos/cosmos-sdk/issues/7657), and a concrete tutorial for setting up a gRPC client [here (TODO)](https://github.com/cosmos/cosmos-sdk/issues/7657).

x/auth/client/rest/decode.go

@@ -55,9 +55,10 @@ func DecodeTxRequestHandlerFn(clientCtx client.Context) http.HandlerFunc {
 
 		response := DecodeResp(stdTx)
 
-		err = checkSignModeError(w, clientCtx, response, "/cosmos/tx/v1beta1/txs/decode")
+		err = checkSignModeError(clientCtx, response, "/cosmos/tx/v1beta1/txs/decode")
 		if err != nil {
-			// Error is already returned by checkSignModeError.
+			rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
+
 			return
 		}
 

x/auth/client/rest/query.go

@@ -18,8 +18,6 @@ import (
 	genutilrest "github.com/cosmos/cosmos-sdk/x/genutil/client/rest"
 )
 
-const unRegisteredConcreteTypeErr = "unregistered concrete type"
-
 // QueryAccountRequestHandlerFn is the query accountREST Handler.
 func QueryAccountRequestHandlerFn(storeName string, clientCtx client.Context) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
@@ -110,9 +108,10 @@ func QueryTxsRequestHandlerFn(clientCtx client.Context) http.HandlerFunc {
 			packStdTxResponse(w, clientCtx, txRes)
 		}
 
-		err = checkSignModeError(w, clientCtx, searchResult, "/cosmos/tx/v1beta1/txs")
+		err = checkSignModeError(clientCtx, searchResult, "/cosmos/tx/v1beta1/txs")
 		if err != nil {
-			// Error is already returned by checkSignModeError.
+			rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
+
 			return
 		}
 
@@ -152,9 +151,10 @@ func QueryTxRequestHandlerFn(clientCtx client.Context) http.HandlerFunc {
 			rest.WriteErrorResponse(w, http.StatusNotFound, fmt.Sprintf("no transaction found with hash %s", hashHexStr))
 		}
 
-		err = checkSignModeError(w, clientCtx, output, "/cosmos/tx/v1beta1/tx/{txhash}")
+		err = checkSignModeError(clientCtx, output, "/cosmos/tx/v1beta1/tx/{txhash}")
 		if err != nil {
-			// Error is already returned by checkSignModeError.
+			rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
+
 			return
 		}
 
@@ -198,18 +198,22 @@ func packStdTxResponse(w http.ResponseWriter, clientCtx client.Context, txRes *s
 	return nil
 }
 
-func checkSignModeError(w http.ResponseWriter, ctx client.Context, resp interface{}, grpcEndPoint string) error {
+// checkSignModeError checks if there are errors with marshalling non-amino
+// txs with amino.
+func checkSignModeError(ctx client.Context, resp interface{}, grpcEndPoint string) error {
 	// LegacyAmino used intentionally here to handle the SignMode errors
 	marshaler := ctx.LegacyAmino
 
 	_, err := marshaler.MarshalJSON(resp)
-	if err != nil && strings.Contains(err.Error(), unRegisteredConcreteTypeErr) {
+	if err != nil {
-		rest.WriteErrorResponse(w, http.StatusInternalServerError,
+
-			"This transaction was created with the new SIGN_MODE_DIRECT signing method, and therefore cannot be displayed"+
+		// If there's an unmarshalling error, we assume that it's because we're
-				" via legacy REST handlers, please use CLI or directly query the Tendermint RPC endpoint to query"+
+		// using amino to unmarshal a non-amino tx.
-				" this transaction. gRPC gateway endpoint is "+grpcEndPoint+". Please also see the REST endpoints migration"+
+		return fmt.Errorf("this transaction cannot be displayed via legacy REST endpoints, because it does not support"+
-				" guide at "+clientrest.DeprecationURL+".")
+			" Amino serialization. Please either use CLI, gRPC, gRPC-gateway, or directly query the Tendermint RPC"+
-		return err
+			" endpoint to query this transaction. The new REST endpoint (via gRPC-gateway) is %s. Please also see the"+
+			"REST endpoints migration guide at %s for more info", grpcEndPoint, clientrest.DeprecationURL)
+
 	}
 
 	return nil

x/auth/client/rest/rest_test.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/spf13/cobra"
 	"github.com/stretchr/testify/suite"
 
 	"github.com/cosmos/cosmos-sdk/client/flags"
@@ -16,16 +17,16 @@ import (
 	"github.com/cosmos/cosmos-sdk/testutil/testdata"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/types/rest"
+	txtypes "github.com/cosmos/cosmos-sdk/types/tx"
 	"github.com/cosmos/cosmos-sdk/types/tx/signing"
 	authclient "github.com/cosmos/cosmos-sdk/x/auth/client"
 	authcli "github.com/cosmos/cosmos-sdk/x/auth/client/cli"
-	bankcli "github.com/cosmos/cosmos-sdk/x/bank/client/testutil"
-	ibccli "github.com/cosmos/cosmos-sdk/x/ibc/core/04-channel/client/cli"
-
-	txtypes "github.com/cosmos/cosmos-sdk/types/tx"
 	rest2 "github.com/cosmos/cosmos-sdk/x/auth/client/rest"
 	"github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx"
+	bankcli "github.com/cosmos/cosmos-sdk/x/bank/client/testutil"
 	"github.com/cosmos/cosmos-sdk/x/bank/types"
+	ibccli "github.com/cosmos/cosmos-sdk/x/ibc/core/04-channel/client/cli"
+	ibcsolomachinecli "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/06-solomachine/client/cli"
 )
 
 type IntegrationTestSuite struct {
@@ -129,51 +130,99 @@ func (s *IntegrationTestSuite) TestBroadcastTxRequest() {
 	s.Require().NotEmpty(txRes.TxHash)
 }
 
-func (s *IntegrationTestSuite) TestQueryTxByHash() {
+// Helper function to test querying txs. We will use it to query StdTx and service `Msg`s.
+func (s *IntegrationTestSuite) testQueryTx(txHeight int64, txHash, txRecipient string) {
+	val0 := s.network.Validators[0]
+
+	testCases := []struct {
+		desc     string
+		malleate func() *sdk.TxResponse
+	}{
+		{
+			"Query by hash",
+			func() *sdk.TxResponse {
+				txJSON, err := rest.GetRequest(fmt.Sprintf("%s/txs/%s", val0.APIAddress, txHash))
+				s.Require().NoError(err)
+
+				var txResAmino sdk.TxResponse
+				s.Require().NoError(val0.ClientCtx.LegacyAmino.UnmarshalJSON(txJSON, &txResAmino))
+				return &txResAmino
+			},
+		},
+		{
+			"Query by height",
+			func() *sdk.TxResponse {
+				txJSON, err := rest.GetRequest(fmt.Sprintf("%s/txs?limit=10&page=1&tx.height=%d", val0.APIAddress, txHeight))
+				s.Require().NoError(err)
+
+				var searchtxResult sdk.SearchTxsResult
+				s.Require().NoError(val0.ClientCtx.LegacyAmino.UnmarshalJSON(txJSON, &searchtxResult))
+				s.Require().Len(searchtxResult.Txs, 1)
+				return searchtxResult.Txs[0]
+			},
+		},
+		{
+			"Query by event (transfer.recipient)",
+			func() *sdk.TxResponse {
+				txJSON, err := rest.GetRequest(fmt.Sprintf("%s/txs?transfer.recipient=%s", val0.APIAddress, txRecipient))
+				s.Require().NoError(err)
+
+				var searchtxResult sdk.SearchTxsResult
+				s.Require().NoError(val0.ClientCtx.LegacyAmino.UnmarshalJSON(txJSON, &searchtxResult))
+				s.Require().Len(searchtxResult.Txs, 1)
+				return searchtxResult.Txs[0]
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		s.Run(fmt.Sprintf("Case %s", tc.desc), func() {
+			txResponse := tc.malleate()
+
+			// Check that the height is correct.
+			s.Require().Equal(txHeight, txResponse.Height)
+
+			// Check that the events are correct.
+			s.Require().Contains(
+				txResponse.RawLog,
+				fmt.Sprintf("{\"key\":\"recipient\",\"value\":\"%s\"}", txRecipient),
+			)
+
+			// Check that the Msg is correct.
+			stdTx, ok := txResponse.Tx.GetCachedValue().(legacytx.StdTx)
+			s.Require().True(ok)
+			msgs := stdTx.GetMsgs()
+			s.Require().Equal(len(msgs), 1)
+			msg, ok := msgs[0].(*types.MsgSend)
+			s.Require().True(ok)
+			s.Require().Equal(txRecipient, msg.ToAddress)
+		})
+	}
+}
+
+func (s *IntegrationTestSuite) TestQueryTxWithStdTx() {
 	val0 := s.network.Validators[0]
 
 	// We broadcasted a StdTx in SetupSuite.
-	// we just check for a non-empty TxHash here, the actual hash will depend on the underlying tx configuration
+	// We just check for a non-empty TxHash here, the actual hash will depend on the underlying tx configuration
 	s.Require().NotEmpty(s.stdTxRes.TxHash)
 
-	// We now fetch the tx by hash on the `/tx/{hash}` route.
+	s.testQueryTx(s.stdTxRes.Height, s.stdTxRes.TxHash, val0.Address.String())
-	txJSON, err := rest.GetRequest(fmt.Sprintf("%s/txs/%s", val0.APIAddress, s.stdTxRes.TxHash))
-	s.Require().NoError(err)
-
-	// txJSON should contain the whole tx, we just make sure that our custom
-	// memo is there.
-	s.Require().Contains(string(txJSON), s.stdTx.Memo)
 }
 
-func (s *IntegrationTestSuite) TestQueryTxByHeight() {
+func (s *IntegrationTestSuite) TestQueryTxWithServiceMessage() {
-	val0 := s.network.Validators[0]
-
-	// We broadcasted a StdTx in SetupSuite.
-	// we just check for a non-empty height here, as we'll need to for querying.
-	s.Require().NotEmpty(s.stdTxRes.Height)
-
-	// We now fetch the tx on `/txs` route, filtering by `tx.height`
-	txJSON, err := rest.GetRequest(fmt.Sprintf("%s/txs?limit=10&page=1&tx.height=%d", val0.APIAddress, s.stdTxRes.Height))
-	s.Require().NoError(err)
-
-	// txJSON should contain the whole tx, we just make sure that our custom
-	// memo is there.
-	s.Require().Contains(string(txJSON), s.stdTx.Memo)
-}
-
-func (s *IntegrationTestSuite) TestQueryTxByHashWithServiceMessage() {
 	val := s.network.Validators[0]
 
 	sendTokens := sdk.NewInt64Coin(s.cfg.BondDenom, 10)
+	_, _, addr := testdata.KeyTestPubAddr()
 
-	// Right after this line, we're sending a tx. Might need to wait a block
+	// Might need to wait a block to refresh sequences from previous setups.
-	// to refresh sequences.
 	s.Require().NoError(s.network.WaitForNextBlock())
 
 	out, err := bankcli.ServiceMsgSendExec(
 		val.ClientCtx,
 		val.Address,
-		val.Address,
+		addr,
 		sdk.NewCoins(sendTokens),
 		fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation),
 		fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock),
@@ -188,17 +237,7 @@ func (s *IntegrationTestSuite) TestQueryTxByHashWithServiceMessage() {
 
 	s.Require().NoError(s.network.WaitForNextBlock())
 
-	txJSON, err := rest.GetRequest(fmt.Sprintf("%s/txs/%s", val.APIAddress, txRes.TxHash))
+	s.testQueryTx(txRes.Height, txRes.TxHash, addr.String())
-	s.Require().NoError(err)
-
-	var txResAmino sdk.TxResponse
-	s.Require().NoError(val.ClientCtx.LegacyAmino.UnmarshalJSON(txJSON, &txResAmino))
-	stdTx, ok := txResAmino.Tx.GetCachedValue().(legacytx.StdTx)
-	s.Require().True(ok)
-	msgs := stdTx.GetMsgs()
-	s.Require().Equal(len(msgs), 1)
-	_, ok = msgs[0].(*types.MsgSend)
-	s.Require().True(ok)
 }
 
 func (s *IntegrationTestSuite) TestMultipleSyncBroadcastTxRequests() {
@@ -303,42 +342,43 @@ func (s *IntegrationTestSuite) broadcastReq(stdTx legacytx.StdTx, mode string) (
 	return rest.PostRequest(fmt.Sprintf("%s/txs", val.APIAddress), "application/json", bz)
 }
 
-func (s *IntegrationTestSuite) TestLegacyRestErrMessages() {
+// testQueryIBCTx is a helper function to test querying txs which:
+// - show an error message on legacy REST endpoints
+// - succeed using gRPC
+// In practise, we call this function on IBC txs.
+func (s *IntegrationTestSuite) testQueryIBCTx(txRes sdk.TxResponse, cmd *cobra.Command, args []string) {
 	val := s.network.Validators[0]
 
-	args := []string{
+	errMsg := "this transaction cannot be displayed via legacy REST endpoints, because it does not support" +
-		"121",       // dummy port-id
+		" Amino serialization. Please either use CLI, gRPC, gRPC-gateway, or directly query the Tendermint RPC" +
-		"channel-0", // dummy channel-id
+		" endpoint to query this transaction. The new REST endpoint (via gRPC-gateway) is "
-		fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation),
+
-		fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock),
+	// Test that legacy endpoint return the above error message on IBC txs.
-		fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()),
+	testCases := []struct {
-		fmt.Sprintf("--gas=%d", flags.DefaultGasLimit),
+		desc string
-		fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()),
+		url  string
-		fmt.Sprintf("--%s=foobar", flags.FlagMemo),
+	}{
+		{
+			"Query by hash",
+			fmt.Sprintf("%s/txs/%s", val.APIAddress, txRes.TxHash),
+		},
+		{
+			"Query by height",
+			fmt.Sprintf("%s/txs?tx.height=%d", val.APIAddress, txRes.Height),
+		},
 	}
 
-	// created a dummy txn for IBC, eventually it fails. Our intension is to test the error message of querying a
+	for _, tc := range testCases {
-	// message which is signed with proto, since IBC won't support legacy amino at all we are considering a message from IBC module.
+		s.Run(fmt.Sprintf("Case %s", tc.desc), func() {
-	out, err := clitestutil.ExecTestCLICmd(val.ClientCtx, ibccli.NewChannelCloseInitCmd(), args)
+			txJSON, err := rest.GetRequest(tc.url)
-	s.Require().NoError(err)
-
-	var txRes sdk.TxResponse
-	s.Require().NoError(val.ClientCtx.JSONMarshaler.UnmarshalJSON(out.Bytes(), &txRes))
-
-	s.Require().NoError(s.network.WaitForNextBlock())
-
-	// try to fetch the txn using legacy rest, this won't work since the ibc module doesn't support amino.
-	txJSON, err := rest.GetRequest(fmt.Sprintf("%s/txs/%s", val.APIAddress, txRes.TxHash))
 			s.Require().NoError(err)
 
 			var errResp rest.ErrorResponse
 			s.Require().NoError(val.ClientCtx.LegacyAmino.UnmarshalJSON(txJSON, &errResp))
 
-	errMsg := "This transaction was created with the new SIGN_MODE_DIRECT signing method, " +
-		"and therefore cannot be displayed via legacy REST handlers, please use CLI or directly query the Tendermint " +
-		"RPC endpoint to query this transaction."
-
 			s.Require().Contains(errResp.Error, errMsg)
+		})
+	}
 
 	// try fetching the txn using gRPC req, it will fetch info since it has proto codec.
 	grpcJSON, err := rest.GetRequest(fmt.Sprintf("%s/cosmos/tx/v1beta1/tx/%s", val.APIAddress, txRes.TxHash))
@@ -350,7 +390,7 @@ func (s *IntegrationTestSuite) TestLegacyRestErrMessages() {
 
 	// generate broadcast only txn.
 	args = append(args, fmt.Sprintf("--%s=true", flags.FlagGenerateOnly))
-	out, err = clitestutil.ExecTestCLICmd(val.ClientCtx, ibccli.NewChannelCloseInitCmd(), args)
+	out, err := clitestutil.ExecTestCLICmd(val.ClientCtx, cmd, args)
 	s.Require().NoError(err)
 
 	txFile, cleanup := testutil.WriteToNewTempFile(s.T(), string(out.Bytes()))
@@ -368,10 +408,80 @@ func (s *IntegrationTestSuite) TestLegacyRestErrMessages() {
 	res, err := rest.PostRequest(fmt.Sprintf("%s/txs/decode", val.APIAddress), "application/json", bz)
 	s.Require().NoError(err)
 
+	var errResp rest.ErrorResponse
 	s.Require().NoError(val.ClientCtx.LegacyAmino.UnmarshalJSON(res, &errResp))
 	s.Require().Contains(errResp.Error, errMsg)
 }
 
+// TestLegacyRestErrMessages creates two IBC txs, one that fails, one that
+// succeeds, and make sure we cannot query any of them (with pretty error msg).
+// Our intension is to test the error message of querying a message which is
+// signed with proto, since IBC won't support legacy amino at all we are
+// considering a message from IBC module.
+func (s *IntegrationTestSuite) TestLegacyRestErrMessages() {
+	val := s.network.Validators[0]
+
+	// Write consensus json to temp file, used for an IBC message.
+	consensusJSON, cleanup := testutil.WriteToNewTempFile(
+		s.T(),
+		`{"public_key":{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"A/3SXL2ONYaOkxpdR5P8tHTlSlPv1AwQwSFxKRee5JQW"},"diversifier":"diversifier","timestamp":"10"}`,
+	)
+	defer cleanup()
+
+	testCases := []struct {
+		desc string
+		cmd  *cobra.Command
+		args []string
+		code uint32
+	}{
+		{
+			"Failing IBC message",
+			ibccli.NewChannelCloseInitCmd(),
+			[]string{
+				"121",       // dummy port-id
+				"channel-0", // dummy channel-id
+				fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation),
+				fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock),
+				fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()),
+				fmt.Sprintf("--gas=%d", flags.DefaultGasLimit),
+				fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()),
+				fmt.Sprintf("--%s=foobar", flags.FlagMemo),
+			},
+			uint32(7),
+		},
+		{
+			"Successful IBC message",
+			ibcsolomachinecli.NewCreateClientCmd(),
+			[]string{
+				"21212121212",        // dummy client-id
+				"1",                  // dummy sequence
+				consensusJSON.Name(), // path to consensus json,
+				fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation),
+				fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock),
+				fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()),
+				fmt.Sprintf("--gas=%d", flags.DefaultGasLimit),
+				fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()),
+				fmt.Sprintf("--%s=foobar", flags.FlagMemo),
+			},
+			uint32(0),
+		},
+	}
+
+	for _, tc := range testCases {
+		s.Run(fmt.Sprintf("Case %s", tc.desc), func() {
+			out, err := clitestutil.ExecTestCLICmd(val.ClientCtx, tc.cmd, tc.args)
+			s.Require().NoError(err)
+			var txRes sdk.TxResponse
+			s.Require().NoError(val.ClientCtx.JSONMarshaler.UnmarshalJSON(out.Bytes(), &txRes))
+			s.Require().Equal(tc.code, txRes.Code)
+
+			s.Require().NoError(s.network.WaitForNextBlock())
+
+			s.testQueryIBCTx(txRes, tc.cmd, tc.args)
+		})
+	}
+}
+
 func TestIntegrationTestSuite(t *testing.T) {
 	suite.Run(t, new(IntegrationTestSuite))
 }