From 20205a2034ff4dc5bde2447b836fcb8943f013e2 Mon Sep 17 00:00:00 2001 From: Amaury Martiny Date: Fri, 20 Nov 2020 11:52:39 +0100 Subject: [PATCH 01/13] Add tests for query txs --- x/auth/client/rest/rest_test.go | 110 +++++++++++++++++++++----------- 1 file changed, 74 insertions(+), 36 deletions(-) diff --git a/x/auth/client/rest/rest_test.go b/x/auth/client/rest/rest_test.go index b03b615b6aa..c9863ee0916 100644 --- a/x/auth/client/rest/rest_test.go +++ b/x/auth/client/rest/rest_test.go @@ -129,51 +129,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] - // 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 - s.Require().NotEmpty(s.stdTxRes.TxHash) - - // We now fetch the tx by hash on the `/tx/{hash}` route. - txJSON, err := rest.GetRequest(fmt.Sprintf("%s/txs/%s", val0.APIAddress, s.stdTxRes.TxHash)) - s.Require().NoError(err) + 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] + }, + }, + } - // txJSON should contain the whole tx, we just make sure that our custom - // memo is there. - s.Require().Contains(string(txJSON), s.stdTx.Memo) + 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) TestQueryTxByHeight() { +func (s *IntegrationTestSuite) TestQueryTxWithStdTx() { 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) + // 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) - // txJSON should contain the whole tx, we just make sure that our custom - // memo is there. - s.Require().Contains(string(txJSON), s.stdTx.Memo) + s.testQueryTx(s.stdTxRes.Height, s.stdTxRes.TxHash, val0.Address.String()) } -func (s *IntegrationTestSuite) TestQueryTxByHashWithServiceMessage() { +func (s *IntegrationTestSuite) TestQueryTxWithServiceMessage() { 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 - // to refresh sequences. + // Might need to wait a block to refresh sequences from previous setups. 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 +236,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.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) + s.testQueryTx(txRes.Height, txRes.TxHash, addr.String()) } func (s *IntegrationTestSuite) TestMultipleSyncBroadcastTxRequests() { From f8e4a00dbce855492de027a126ea5f6840dd19d5 Mon Sep 17 00:00:00 2001 From: Amaury Martiny Date: Fri, 20 Nov 2020 17:09:22 +0100 Subject: [PATCH 02/13] Add test for IBC --- x/auth/client/rest/query.go | 4 ++-- x/auth/client/rest/rest_test.go | 40 +++++++++++++++++++++++++-------- 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/x/auth/client/rest/query.go b/x/auth/client/rest/query.go index d60028abc5d..188281d7856 100644 --- a/x/auth/client/rest/query.go +++ b/x/auth/client/rest/query.go @@ -205,8 +205,8 @@ func checkSignModeError(w http.ResponseWriter, ctx client.Context, resp interfac if err != nil && strings.Contains(err.Error(), unRegisteredConcreteTypeErr) { rest.WriteErrorResponse(w, http.StatusInternalServerError, "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. gRPC gateway endpoint is "+grpcEndPoint) + " via legacy REST handlers. Please either use CLI, gRPC, gRPC-gateway, or directly query the Tendermint RPC"+ + " endpoint to query this transaction. The new REST endpoint (via gRPC-gateway) is "+grpcEndPoint) return err } diff --git a/x/auth/client/rest/rest_test.go b/x/auth/client/rest/rest_test.go index c9863ee0916..b5011435cea 100644 --- a/x/auth/client/rest/rest_test.go +++ b/x/auth/client/rest/rest_test.go @@ -362,21 +362,42 @@ func (s *IntegrationTestSuite) TestLegacyRestErrMessages() { var txRes sdk.TxResponse s.Require().NoError(val.ClientCtx.JSONMarshaler.UnmarshalJSON(out.Bytes(), &txRes)) + s.Require().NotEqual(0, txRes.Code) // It fails, see comment above. 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) + errMsg := "This transaction was created with the new SIGN_MODE_DIRECT signing method, and therefore cannot be displayed" + + " via legacy REST handlers. Please either use CLI, gRPC, gRPC-gateway, or directly query the Tendermint RPC" + + " endpoint to query this transaction. The new REST endpoint (via gRPC-gateway) is " - var errResp rest.ErrorResponse - s.Require().NoError(val.ClientCtx.LegacyAmino.UnmarshalJSON(txJSON, &errResp)) + // Test that legacy endpoint return the above error message on IBC txs. + testCases := []struct { + desc string + url string + }{ + { + "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), + }, + } - 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." + for _, tc := range testCases { + s.Run(fmt.Sprintf("Case %s", tc.desc), func() { + txJSON, err := rest.GetRequest(tc.url) + s.Require().NoError(err) - s.Require().Contains(errResp.Error, errMsg) + fmt.Println(string(txJSON)) + + var errResp rest.ErrorResponse + s.Require().NoError(val.ClientCtx.LegacyAmino.UnmarshalJSON(txJSON, &errResp)) + + 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)) @@ -406,6 +427,7 @@ 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) } From 5e5d6be17fcc09e7fcde258500a0ccf566719d75 Mon Sep 17 00:00:00 2001 From: Amaury Martiny Date: Fri, 20 Nov 2020 17:29:46 +0100 Subject: [PATCH 03/13] Refactor checkSignModeError --- x/auth/client/rest/decode.go | 5 +++-- x/auth/client/rest/query.go | 39 ++++++++++++++++++++++++--------- x/auth/client/rest/rest_test.go | 2 -- 3 files changed, 32 insertions(+), 14 deletions(-) diff --git a/x/auth/client/rest/decode.go b/x/auth/client/rest/decode.go index d048f91b4c3..3061d71cf48 100644 --- a/x/auth/client/rest/decode.go +++ b/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 } diff --git a/x/auth/client/rest/query.go b/x/auth/client/rest/query.go index 188281d7856..fd9c2dfc68f 100644 --- a/x/auth/client/rest/query.go +++ b/x/auth/client/rest/query.go @@ -17,7 +17,13 @@ import ( genutilrest "github.com/cosmos/cosmos-sdk/x/genutil/client/rest" ) -const unRegisteredConcreteTypeErr = "unregistered concrete type" +// knownErrors are errors that happen on unmarshalling new proto-only txs using +// amino. Also see https://github.com/cosmos/cosmos-sdk/issues/7639. +var knownErrors = []string{ + "unregistered interface", + "unregistered concrete type", + "wrong SignMode. Expected SIGN_MODE_LEGACY_AMINO_JSON, got SIGN_MODE_DIRECT", +} // query accountREST Handler func QueryAccountRequestHandlerFn(storeName string, clientCtx client.Context) http.HandlerFunc { @@ -109,9 +115,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 } @@ -151,9 +158,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 } @@ -197,16 +205,27 @@ 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 proto-only +// 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) { - rest.WriteErrorResponse(w, http.StatusInternalServerError, - "This transaction was created with the new SIGN_MODE_DIRECT signing method, and therefore cannot be displayed"+ + if err != nil { + isKnownError := false + for _, knownError := range knownErrors { + if strings.Contains(err.Error(), knownError) { + isKnownError = true + } + } + + if isKnownError { + return fmt.Errorf("This transaction was created with the new SIGN_MODE_DIRECT signing method, and therefore cannot be displayed"+ " via legacy REST handlers. Please either use CLI, gRPC, gRPC-gateway, or directly query the Tendermint RPC"+ - " endpoint to query this transaction. The new REST endpoint (via gRPC-gateway) is "+grpcEndPoint) + " endpoint to query this transaction. The new REST endpoint (via gRPC-gateway) is %s", grpcEndPoint) + } + return err } diff --git a/x/auth/client/rest/rest_test.go b/x/auth/client/rest/rest_test.go index b5011435cea..ab7fc1f0712 100644 --- a/x/auth/client/rest/rest_test.go +++ b/x/auth/client/rest/rest_test.go @@ -390,8 +390,6 @@ func (s *IntegrationTestSuite) TestLegacyRestErrMessages() { txJSON, err := rest.GetRequest(tc.url) s.Require().NoError(err) - fmt.Println(string(txJSON)) - var errResp rest.ErrorResponse s.Require().NoError(val.ClientCtx.LegacyAmino.UnmarshalJSON(txJSON, &errResp)) From 32225ecdea1db874911be83afd42340d2c1c80ff Mon Sep 17 00:00:00 2001 From: Amaury Martiny Date: Fri, 20 Nov 2020 17:32:58 +0100 Subject: [PATCH 04/13] Fix lint --- x/auth/client/rest/query.go | 2 +- x/auth/client/rest/rest_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/x/auth/client/rest/query.go b/x/auth/client/rest/query.go index fd9c2dfc68f..5aa4ee16d6c 100644 --- a/x/auth/client/rest/query.go +++ b/x/auth/client/rest/query.go @@ -221,7 +221,7 @@ func checkSignModeError(ctx client.Context, resp interface{}, grpcEndPoint strin } if isKnownError { - return fmt.Errorf("This transaction was created with the new SIGN_MODE_DIRECT signing method, and therefore cannot be displayed"+ + return fmt.Errorf("this transaction was created with the new SIGN_MODE_DIRECT signing method, and therefore cannot be displayed"+ " via legacy REST handlers. Please either use CLI, gRPC, gRPC-gateway, or directly query the Tendermint RPC"+ " endpoint to query this transaction. The new REST endpoint (via gRPC-gateway) is %s", grpcEndPoint) } diff --git a/x/auth/client/rest/rest_test.go b/x/auth/client/rest/rest_test.go index ab7fc1f0712..10f40e03c46 100644 --- a/x/auth/client/rest/rest_test.go +++ b/x/auth/client/rest/rest_test.go @@ -366,7 +366,7 @@ func (s *IntegrationTestSuite) TestLegacyRestErrMessages() { s.Require().NoError(s.network.WaitForNextBlock()) - errMsg := "This transaction was created with the new SIGN_MODE_DIRECT signing method, and therefore cannot be displayed" + + errMsg := "this transaction was created with the new SIGN_MODE_DIRECT signing method, and therefore cannot be displayed" + " via legacy REST handlers. Please either use CLI, gRPC, gRPC-gateway, or directly query the Tendermint RPC" + " endpoint to query this transaction. The new REST endpoint (via gRPC-gateway) is " From 9915df532e5434e647305429ddb7197e8568737f Mon Sep 17 00:00:00 2001 From: Amaury Martiny Date: Mon, 23 Nov 2020 11:26:59 +0100 Subject: [PATCH 05/13] Add successful IBC test --- x/auth/client/rest/rest_test.go | 148 ++++++++++++++++++++++---------- 1 file changed, 101 insertions(+), 47 deletions(-) diff --git a/x/auth/client/rest/rest_test.go b/x/auth/client/rest/rest_test.go index 10f40e03c46..7ff9b881d75 100644 --- a/x/auth/client/rest/rest_test.go +++ b/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 { @@ -35,7 +36,7 @@ type IntegrationTestSuite struct { network *network.Network stdTx legacytx.StdTx - stdTxRes sdk.TxResponse + stdtxRes sdk.TxResponse } func (s *IntegrationTestSuite) SetupSuite() { @@ -60,8 +61,8 @@ func (s *IntegrationTestSuite) SetupSuite() { s.Require().NoError(err) // NOTE: this uses amino explicitly, don't migrate it! - s.Require().NoError(s.cfg.LegacyAmino.UnmarshalJSON(res, &s.stdTxRes)) - s.Require().Equal(uint32(0), s.stdTxRes.Code) + s.Require().NoError(s.cfg.LegacyAmino.UnmarshalJSON(res, &s.stdtxRes)) + s.Require().Equal(uint32(0), s.stdtxRes.Code) s.Require().NoError(s.network.WaitForNextBlock()) } @@ -154,10 +155,10 @@ func (s *IntegrationTestSuite) testQueryTx(txHeight int64, txHash, txRecipient s 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] + var searchtxResult sdk.SearchTxsResult + s.Require().NoError(val0.ClientCtx.LegacyAmino.UnmarshalJSON(txJSON, &searchtxResult)) + s.Require().Len(searchtxResult.Txs, 1) + return searchtxResult.Txs[0] }, }, { @@ -166,10 +167,10 @@ func (s *IntegrationTestSuite) testQueryTx(txHeight int64, txHash, txRecipient s 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] + var searchtxResult sdk.SearchTxsResult + s.Require().NoError(val0.ClientCtx.LegacyAmino.UnmarshalJSON(txJSON, &searchtxResult)) + s.Require().Len(searchtxResult.Txs, 1) + return searchtxResult.Txs[0] }, }, } @@ -204,9 +205,9 @@ func (s *IntegrationTestSuite) TestQueryTxWithStdTx() { // 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 - s.Require().NotEmpty(s.stdTxRes.TxHash) + s.Require().NotEmpty(s.stdtxRes.TxHash) - s.testQueryTx(s.stdTxRes.Height, s.stdTxRes.TxHash, val0.Address.String()) + s.testQueryTx(s.stdtxRes.Height, s.stdtxRes.TxHash, val0.Address.String()) } func (s *IntegrationTestSuite) TestQueryTxWithServiceMessage() { @@ -276,13 +277,13 @@ func (s *IntegrationTestSuite) TestMultipleSyncBroadcastTxRequests() { if tc.shouldErr { var sigVerifyFailureCode uint32 = 4 s.Require().Equal(sigVerifyFailureCode, txRes.Code, - "Testcase '%s': Expected signature verification failure {Code: %d} from TxResponse. "+ + "Testcase '%s': Expected signature verification failure {Code: %d} from txResponse. "+ "Found {Code: %d, RawLog: '%v'}", tc.desc, sigVerifyFailureCode, txRes.Code, txRes.RawLog, ) } else { s.Require().Equal(uint32(0), txRes.Code, - "Testcase '%s': TxResponse errored unexpectedly. Err: {Code: %d, RawLog: '%v'}", + "Testcase '%s': txResponse errored unexpectedly. Err: {Code: %d, RawLog: '%v'}", tc.desc, txRes.Code, txRes.RawLog, ) } @@ -341,36 +342,18 @@ 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{ - "121", // dummy port-id - "21212121212", // 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), - } - - // created a dummy txn for IBC, eventually it fails. 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. - out, err := clitestutil.ExecTestCLICmd(val.ClientCtx, ibccli.NewChannelCloseInitCmd(), args) - s.Require().NoError(err) - - var txRes sdk.TxResponse - s.Require().NoError(val.ClientCtx.JSONMarshaler.UnmarshalJSON(out.Bytes(), &txRes)) - s.Require().NotEqual(0, txRes.Code) // It fails, see comment above. - - s.Require().NoError(s.network.WaitForNextBlock()) - errMsg := "this transaction was created with the new SIGN_MODE_DIRECT signing method, and therefore cannot be displayed" + " via legacy REST handlers. Please either use CLI, gRPC, gRPC-gateway, or directly query the Tendermint RPC" + " endpoint to query this transaction. The new REST endpoint (via gRPC-gateway) is " - // Test that legacy endpoint return the above error message on IBC txs. + // Test that legacy endpoint return the above error message on IBC txs. testCases := []struct { desc string url string @@ -401,13 +384,13 @@ func (s *IntegrationTestSuite) TestLegacyRestErrMessages() { grpcJSON, err := rest.GetRequest(fmt.Sprintf("%s/cosmos/tx/v1beta1/tx/%s", val.APIAddress, txRes.TxHash)) s.Require().NoError(err) - var getTxRes txtypes.GetTxResponse - s.Require().NoError(val.ClientCtx.JSONMarshaler.UnmarshalJSON(grpcJSON, &getTxRes)) - s.Require().Equal(getTxRes.Tx.Body.Memo, "foobar") + var gettxRes txtypes.GetTxResponse + s.Require().NoError(val.ClientCtx.JSONMarshaler.UnmarshalJSON(grpcJSON, &gettxRes)) + s.Require().Equal(gettxRes.Tx.Body.Memo, "foobar") // 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())) @@ -430,6 +413,77 @@ func (s *IntegrationTestSuite) TestLegacyRestErrMessages() { 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 + "21212121212", // 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) + }) + } + + s.Require().False(true) +} + func TestIntegrationTestSuite(t *testing.T) { suite.Run(t, new(IntegrationTestSuite)) } From 8da19f8d0edc93f00a0d0de7f0b35a0ce5232a9e Mon Sep 17 00:00:00 2001 From: Amaury Martiny Date: Mon, 23 Nov 2020 11:36:13 +0100 Subject: [PATCH 06/13] Remove logs --- x/auth/client/rest/rest_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/x/auth/client/rest/rest_test.go b/x/auth/client/rest/rest_test.go index 7ff9b881d75..88273fee4f7 100644 --- a/x/auth/client/rest/rest_test.go +++ b/x/auth/client/rest/rest_test.go @@ -480,8 +480,6 @@ func (s *IntegrationTestSuite) TestLegacyRestErrMessages() { s.testQueryIBCTx(txRes, tc.cmd, tc.args) }) } - - s.Require().False(true) } func TestIntegrationTestSuite(t *testing.T) { From 9f9d5a700aaef03acaa200683fde34fe0c231ba1 Mon Sep 17 00:00:00 2001 From: Amaury Martiny Date: Mon, 23 Nov 2020 11:55:38 +0100 Subject: [PATCH 07/13] break --- x/auth/client/rest/query.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x/auth/client/rest/query.go b/x/auth/client/rest/query.go index 5aa4ee16d6c..7d2cce7d4fb 100644 --- a/x/auth/client/rest/query.go +++ b/x/auth/client/rest/query.go @@ -217,6 +217,8 @@ func checkSignModeError(ctx client.Context, resp interface{}, grpcEndPoint strin for _, knownError := range knownErrors { if strings.Contains(err.Error(), knownError) { isKnownError = true + + break } } From e06b4dffbe0de348e4a677e24a81e78a921dfce0 Mon Sep 17 00:00:00 2001 From: Amaury Martiny Date: Mon, 23 Nov 2020 15:52:59 +0100 Subject: [PATCH 08/13] Remove known errors --- x/auth/client/rest/query.go | 27 +++++---------------------- 1 file changed, 5 insertions(+), 22 deletions(-) diff --git a/x/auth/client/rest/query.go b/x/auth/client/rest/query.go index 7d2cce7d4fb..ec2618e92fc 100644 --- a/x/auth/client/rest/query.go +++ b/x/auth/client/rest/query.go @@ -17,14 +17,6 @@ import ( genutilrest "github.com/cosmos/cosmos-sdk/x/genutil/client/rest" ) -// knownErrors are errors that happen on unmarshalling new proto-only txs using -// amino. Also see https://github.com/cosmos/cosmos-sdk/issues/7639. -var knownErrors = []string{ - "unregistered interface", - "unregistered concrete type", - "wrong SignMode. Expected SIGN_MODE_LEGACY_AMINO_JSON, got SIGN_MODE_DIRECT", -} - // query accountREST Handler func QueryAccountRequestHandlerFn(storeName string, clientCtx client.Context) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { @@ -213,22 +205,13 @@ func checkSignModeError(ctx client.Context, resp interface{}, grpcEndPoint strin _, err := marshaler.MarshalJSON(resp) if err != nil { - isKnownError := false - for _, knownError := range knownErrors { - if strings.Contains(err.Error(), knownError) { - isKnownError = true - break - } - } + // If there's an unmarshalling error, we assume that it's because we're + // using amino to unmarshal a proto-only tx. + return fmt.Errorf("this transaction was created with the new SIGN_MODE_DIRECT signing method, and therefore cannot be displayed"+ + " via legacy REST handlers. Please either use CLI, gRPC, gRPC-gateway, or directly query the Tendermint RPC"+ + " endpoint to query this transaction. The new REST endpoint (via gRPC-gateway) is %s", grpcEndPoint) - if isKnownError { - return fmt.Errorf("this transaction was created with the new SIGN_MODE_DIRECT signing method, and therefore cannot be displayed"+ - " via legacy REST handlers. Please either use CLI, gRPC, gRPC-gateway, or directly query the Tendermint RPC"+ - " endpoint to query this transaction. The new REST endpoint (via gRPC-gateway) is %s", grpcEndPoint) - } - - return err } return nil From 84a98fc1d548ded0a571c2f1ade7cec1d13725d6 Mon Sep 17 00:00:00 2001 From: Amaury Martiny Date: Tue, 24 Nov 2020 15:45:48 +0100 Subject: [PATCH 09/13] Update error message --- x/auth/client/rest/query.go | 8 ++++---- x/auth/client/rest/rest_test.go | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/x/auth/client/rest/query.go b/x/auth/client/rest/query.go index aebc5352108..979fd5ceed5 100644 --- a/x/auth/client/rest/query.go +++ b/x/auth/client/rest/query.go @@ -209,10 +209,10 @@ func checkSignModeError(ctx client.Context, resp interface{}, grpcEndPoint strin // If there's an unmarshalling error, we assume that it's because we're // using amino to unmarshal a proto-only tx. - return fmt.Errorf("this transaction was created with the new SIGN_MODE_DIRECT signing method, and therefore cannot be displayed"+ - " via legacy REST handlers. Please either use CLI, gRPC, gRPC-gateway, or directly query the Tendermint RPC"+ - " 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 fmt.Errorf("this transaction cannot be displayed via legacy REST endpoints, because it does not support"+ + " Amino serialization. Please either use CLI, gRPC, gRPC-gateway, or directly query the Tendermint RPC"+ + " 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) } diff --git a/x/auth/client/rest/rest_test.go b/x/auth/client/rest/rest_test.go index 88273fee4f7..da564eb3eb4 100644 --- a/x/auth/client/rest/rest_test.go +++ b/x/auth/client/rest/rest_test.go @@ -349,8 +349,8 @@ func (s *IntegrationTestSuite) broadcastReq(stdTx legacytx.StdTx, mode string) ( func (s *IntegrationTestSuite) testQueryIBCTx(txRes sdk.TxResponse, cmd *cobra.Command, args []string) { val := s.network.Validators[0] - errMsg := "this transaction was created with the new SIGN_MODE_DIRECT signing method, and therefore cannot be displayed" + - " via legacy REST handlers. Please either use CLI, gRPC, gRPC-gateway, or directly query the Tendermint RPC" + + errMsg := "this transaction cannot be displayed via legacy REST endpoints, because it does not support" + + " Amino serialization. Please either use CLI, gRPC, gRPC-gateway, or directly query the Tendermint RPC" + " endpoint to query this transaction. The new REST endpoint (via gRPC-gateway) is " // Test that legacy endpoint return the above error message on IBC txs. From dbeb2c3b958c7d5b1d3fbd50f41c5e078ea737b9 Mon Sep 17 00:00:00 2001 From: Amaury Martiny Date: Tue, 24 Nov 2020 15:48:16 +0100 Subject: [PATCH 10/13] Better comments --- x/auth/client/rest/query.go | 6 +++--- x/auth/client/rest/rest_test.go | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/x/auth/client/rest/query.go b/x/auth/client/rest/query.go index 979fd5ceed5..4887d08b27e 100644 --- a/x/auth/client/rest/query.go +++ b/x/auth/client/rest/query.go @@ -18,7 +18,7 @@ import ( genutilrest "github.com/cosmos/cosmos-sdk/x/genutil/client/rest" ) -// query accountREST Handler +// QueryAccountRequestHandlerFn is the query accountREST Handler. func QueryAccountRequestHandlerFn(storeName string, clientCtx client.Context) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) @@ -198,7 +198,7 @@ func packStdTxResponse(w http.ResponseWriter, clientCtx client.Context, txRes *s return nil } -// checkSignModeError checks if there are errors with marshalling proto-only +// 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 @@ -208,7 +208,7 @@ func checkSignModeError(ctx client.Context, resp interface{}, grpcEndPoint strin if err != nil { // If there's an unmarshalling error, we assume that it's because we're - // using amino to unmarshal a proto-only tx. + // using amino to unmarshal a non-amino tx. return fmt.Errorf("this transaction cannot be displayed via legacy REST endpoints, because it does not support"+ " Amino serialization. Please either use CLI, gRPC, gRPC-gateway, or directly query the Tendermint RPC"+ " endpoint to query this transaction. The new REST endpoint (via gRPC-gateway) is %s. Please also see the"+ diff --git a/x/auth/client/rest/rest_test.go b/x/auth/client/rest/rest_test.go index da564eb3eb4..bf3fc9cd68d 100644 --- a/x/auth/client/rest/rest_test.go +++ b/x/auth/client/rest/rest_test.go @@ -36,7 +36,7 @@ type IntegrationTestSuite struct { network *network.Network stdTx legacytx.StdTx - stdtxRes sdk.TxResponse + stdTxRes sdk.TxResponse } func (s *IntegrationTestSuite) SetupSuite() { @@ -61,8 +61,8 @@ func (s *IntegrationTestSuite) SetupSuite() { s.Require().NoError(err) // NOTE: this uses amino explicitly, don't migrate it! - s.Require().NoError(s.cfg.LegacyAmino.UnmarshalJSON(res, &s.stdtxRes)) - s.Require().Equal(uint32(0), s.stdtxRes.Code) + s.Require().NoError(s.cfg.LegacyAmino.UnmarshalJSON(res, &s.stdTxRes)) + s.Require().Equal(uint32(0), s.stdTxRes.Code) s.Require().NoError(s.network.WaitForNextBlock()) } @@ -205,9 +205,9 @@ func (s *IntegrationTestSuite) TestQueryTxWithStdTx() { // 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 - s.Require().NotEmpty(s.stdtxRes.TxHash) + s.Require().NotEmpty(s.stdTxRes.TxHash) - s.testQueryTx(s.stdtxRes.Height, s.stdtxRes.TxHash, val0.Address.String()) + s.testQueryTx(s.stdTxRes.Height, s.stdTxRes.TxHash, val0.Address.String()) } func (s *IntegrationTestSuite) TestQueryTxWithServiceMessage() { From 41acfce36c4d9f69bad74879fb7a8668098fd384 Mon Sep 17 00:00:00 2001 From: Amaury Martiny Date: Tue, 24 Nov 2020 15:50:21 +0100 Subject: [PATCH 11/13] Revert variable renaming --- x/auth/client/rest/rest_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/x/auth/client/rest/rest_test.go b/x/auth/client/rest/rest_test.go index bf3fc9cd68d..5dac80f69d8 100644 --- a/x/auth/client/rest/rest_test.go +++ b/x/auth/client/rest/rest_test.go @@ -277,13 +277,13 @@ func (s *IntegrationTestSuite) TestMultipleSyncBroadcastTxRequests() { if tc.shouldErr { var sigVerifyFailureCode uint32 = 4 s.Require().Equal(sigVerifyFailureCode, txRes.Code, - "Testcase '%s': Expected signature verification failure {Code: %d} from txResponse. "+ + "Testcase '%s': Expected signature verification failure {Code: %d} from TxResponse. "+ "Found {Code: %d, RawLog: '%v'}", tc.desc, sigVerifyFailureCode, txRes.Code, txRes.RawLog, ) } else { s.Require().Equal(uint32(0), txRes.Code, - "Testcase '%s': txResponse errored unexpectedly. Err: {Code: %d, RawLog: '%v'}", + "Testcase '%s': TxResponse errored unexpectedly. Err: {Code: %d, RawLog: '%v'}", tc.desc, txRes.Code, txRes.RawLog, ) } @@ -384,9 +384,9 @@ func (s *IntegrationTestSuite) testQueryIBCTx(txRes sdk.TxResponse, cmd *cobra.C grpcJSON, err := rest.GetRequest(fmt.Sprintf("%s/cosmos/tx/v1beta1/tx/%s", val.APIAddress, txRes.TxHash)) s.Require().NoError(err) - var gettxRes txtypes.GetTxResponse - s.Require().NoError(val.ClientCtx.JSONMarshaler.UnmarshalJSON(grpcJSON, &gettxRes)) - s.Require().Equal(gettxRes.Tx.Body.Memo, "foobar") + var getTxRes txtypes.GetTxResponse + s.Require().NoError(val.ClientCtx.JSONMarshaler.UnmarshalJSON(grpcJSON, &getTxRes)) + s.Require().Equal(getTxRes.Tx.Body.Memo, "foobar") // generate broadcast only txn. args = append(args, fmt.Sprintf("--%s=true", flags.FlagGenerateOnly)) From d5d39db426d190bf05d67329db6ae866648c5e2a Mon Sep 17 00:00:00 2001 From: Amaury Martiny Date: Tue, 24 Nov 2020 15:55:13 +0100 Subject: [PATCH 12/13] Fix lint --- x/auth/client/rest/query.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/auth/client/rest/query.go b/x/auth/client/rest/query.go index 4887d08b27e..f9abae6b71f 100644 --- a/x/auth/client/rest/query.go +++ b/x/auth/client/rest/query.go @@ -212,7 +212,7 @@ func checkSignModeError(ctx client.Context, resp interface{}, grpcEndPoint strin return fmt.Errorf("this transaction cannot be displayed via legacy REST endpoints, because it does not support"+ " Amino serialization. Please either use CLI, gRPC, gRPC-gateway, or directly query the Tendermint RPC"+ " 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) + "REST endpoints migration guide at %s for more info", grpcEndPoint, clientrest.DeprecationURL) } From 2445fd020cb21e9e32aec0ec0102a08c5cf9dec8 Mon Sep 17 00:00:00 2001 From: Amaury Martiny Date: Wed, 25 Nov 2020 12:52:26 +0100 Subject: [PATCH 13/13] Add mention of gRPC as TODO --- docs/migrations/rest.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/migrations/rest.md b/docs/migrations/rest.md index 4de2946faaf..d418587d086 100644 --- a/docs/migrations/rest.md +++ b/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)1. | | `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)1. | | `GET /txs` | Query tx by events | Endpoint will error when trying to output transactions that don't support Amino serialization (e.g. IBC txs)1. | | `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).