Audit through legacy endpoints to find breaking changes

CHANGELOG.md

@@ -53,6 +53,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Client Breaking
 
+* (crypto) [\#7419](https://github.com/cosmos/cosmos-sdk/pull/7419) The SDK doesn't use Tendermint's `crypto.PubKey` interface anymore, and uses instead it's own `PubKey` interface, defined in `crypto/types`. Replace all instances of `crypto.PubKey` by `cryptotypes.Pubkey`.
 * (x/staking) [\#7419](https://github.com/cosmos/cosmos-sdk/pull/7419) The `TmConsPubKey` method on ValidatorI has been removed and replaced instead by `ConsPubKey` (which returns a SDK `cryptotypes.PubKey`) and `TmConsPublicKey` (which returns a Tendermint proto PublicKey).
 
 ### Improvements

docs/migrations/rest.md

@@ -14,12 +14,15 @@ Some important information concerning all legacy REST endpoints:
 
 ## Breaking Changes in 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}`. |
+| Legacy REST Endpoint                                                     | Description                                 | Breaking Change                                                                                                                                                                                                                                                                                                                                                                                                                                                                        |
+| ------------------------------------------------------------------------ | ------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| `POST /txs`                                                              | Broadcast tx                                | Endpoint will error when trying to broadcast transactions that don't support Amino serialization (e.g. IBC txs)<sup>1</sup>.                                                                                                                                                                                                                                                                                                                                                           |
+| `POST /txs/encode`, `POST /txs/decode`                                   | Encode/decode Amino txs from JSON to binary | Endpoint will error when trying to encode/decode 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 /gov/proposals/{id}/votes`, `GET /gov/proposals/{id}/votes/{voter}` | Gov endpoints for querying votes            | All gov endpoints which return votes return int32 in the `option` field instead of string: `1=VOTE_OPTION_YES, 2=VOTE_OPTION_ABSTAIN, 3=VOTE_OPTION_NO, 4=VOTE_OPTION_NO_WITH_VETO`.                                                                                                                                                                                                                                                                                                   |
+| `GET /staking/*`                                                         | Staking query endpoints                     | All staking endpoints which return validators have two breaking changes. First, the validator's `consensus_pubkey` field returns an Amino-encoded struct representing an `Any` instead of a bech32-encoded string representing the pubkey. The `value` field of the `Any` is the pubkey's raw key as base64-encoded bytes. Second, the validator's `status` field now returns an int32 instead of string: `1=BOND_STATUS_UNBONDED`, `2=BOND_STATUS_UNBONDING`, `3=BOND_STATUS_BONDED`. |
+| `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}`.                                                                                                                                                                                                                                             |
 
 <sup>1</sup>: Transactions that don't support Amino serialization are the ones that contain one or more `Msg`s that are not registered with the Amino codec. Currently in the SDK, only IBC `Msg`s fall into this case.
 

x/auth/client/rest/broadcast.go

@@ -1,9 +1,11 @@
 package rest
 
 import (
+	"fmt"
 	"io/ioutil"
 	"net/http"
 
+	clientrest "github.com/cosmos/cosmos-sdk/client/rest"
 	"github.com/cosmos/cosmos-sdk/client/tx"
 
 	"github.com/cosmos/cosmos-sdk/client"
@@ -30,8 +32,15 @@ func BroadcastTxRequest(clientCtx client.Context) http.HandlerFunc {
 		}
 
 		// NOTE: amino is used intentionally here, don't migrate it!
-		if err := clientCtx.LegacyAmino.UnmarshalJSON(body, &req); rest.CheckBadRequestError(w, err) {
-			return
+		err = clientCtx.LegacyAmino.UnmarshalJSON(body, &req)
+		if err != nil {
+			err := fmt.Errorf("this transaction cannot be broadcasted 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 broadcast this transaction. The new REST endpoint (via gRPC-gateway) is POST /cosmos/tx/v1beta1/txs."+
+				" Please also see the REST endpoints migration guide at %s for more info", clientrest.DeprecationURL)
+			if rest.CheckBadRequestError(w, err) {
+				return
+			}
 		}
 
 		txBytes, err := tx.ConvertAndEncodeStdTx(clientCtx.TxConfig, req.Tx)

x/auth/client/rest/decode.go

@@ -55,7 +55,7 @@ func DecodeTxRequestHandlerFn(clientCtx client.Context) http.HandlerFunc {
 
 		response := DecodeResp(stdTx)
 
-		err = checkSignModeError(clientCtx, response, "/cosmos/tx/v1beta1/txs/decode")
+		err = checkAminoMarshalError(clientCtx, response, "/cosmos/tx/v1beta1/txs/decode")
 		if err != nil {
 			rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
 

x/auth/client/rest/encode.go

@@ -2,12 +2,13 @@ package rest
 
 import (
 	"encoding/base64"
+	"fmt"
 	"io/ioutil"
 	"net/http"
 
-	"github.com/cosmos/cosmos-sdk/client/tx"
-
 	"github.com/cosmos/cosmos-sdk/client"
+	clientrest "github.com/cosmos/cosmos-sdk/client/rest"
+	"github.com/cosmos/cosmos-sdk/client/tx"
 	"github.com/cosmos/cosmos-sdk/types/rest"
 	"github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx"
 )
@@ -17,6 +18,12 @@ type EncodeResp struct {
 	Tx string `json:"tx" yaml:"tx"`
 }
 
+// ErrEncodeDecode is the error to show when encoding/decoding txs that are not
+// amino-serializable (e.g. IBC txs).
+var ErrEncodeDecode error = fmt.Errorf("this endpoint does not support txs that are not serializable"+
+	" via Amino, such as txs that contain IBC `Msg`s. For more info, please refer to our"+
+	" REST migration guide at %s", clientrest.DeprecationURL)
+
 // EncodeTxRequestHandlerFn returns the encode tx REST handler. In particular,
 // it takes a json-formatted transaction, encodes it to the Amino wire protocol,
 // and responds with base64-encoded bytes.
@@ -31,8 +38,12 @@ func EncodeTxRequestHandlerFn(clientCtx client.Context) http.HandlerFunc {
 
 		// NOTE: amino is used intentionally here, don't migrate it
 		err = clientCtx.LegacyAmino.UnmarshalJSON(body, &req)
-		if rest.CheckBadRequestError(w, err) {
-			return
+		// If there's an unmarshalling error, we assume that it's because we're
+		// using amino to unmarshal a non-amino tx.
+		if err != nil {
+			if rest.CheckBadRequestError(w, ErrEncodeDecode) {
+				return
+			}
 		}
 
 		// re-encode it in the chain's native binary format

x/auth/client/rest/query.go

@@ -108,7 +108,7 @@ func QueryTxsRequestHandlerFn(clientCtx client.Context) http.HandlerFunc {
 			packStdTxResponse(w, clientCtx, txRes)
 		}
 
-		err = checkSignModeError(clientCtx, searchResult, "/cosmos/tx/v1beta1/txs")
+		err = checkAminoMarshalError(clientCtx, searchResult, "/cosmos/tx/v1beta1/txs")
 		if err != nil {
 			rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
 
@@ -151,7 +151,7 @@ func QueryTxRequestHandlerFn(clientCtx client.Context) http.HandlerFunc {
 			rest.WriteErrorResponse(w, http.StatusNotFound, fmt.Sprintf("no transaction found with hash %s", hashHexStr))
 		}
 
-		err = checkSignModeError(clientCtx, output, "/cosmos/tx/v1beta1/tx/{txhash}")
+		err = checkAminoMarshalError(clientCtx, output, "/cosmos/tx/v1beta1/tx/{txhash}")
 		if err != nil {
 			rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
 
@@ -198,9 +198,9 @@ func packStdTxResponse(w http.ResponseWriter, clientCtx client.Context, txRes *s
 	return nil
 }
 
-// checkSignModeError checks if there are errors with marshalling non-amino
+// checkAminoMarshalError checks if there are errors with marshalling non-amino
 // txs with amino.
-func checkSignModeError(ctx client.Context, resp interface{}, grpcEndPoint string) error {
+func checkAminoMarshalError(ctx client.Context, resp interface{}, grpcEndPoint string) error {
 	// LegacyAmino used intentionally here to handle the SignMode errors
 	marshaler := ctx.LegacyAmino
 

x/auth/client/rest/rest_test.go

@@ -21,7 +21,7 @@ import (
 	"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"
-	rest2 "github.com/cosmos/cosmos-sdk/x/auth/client/rest"
+	authrest "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"
@@ -72,7 +72,7 @@ func (s *IntegrationTestSuite) TearDownSuite() {
 	s.network.Cleanup()
 }
 
-func mkTx() legacytx.StdTx {
+func mkStdTx() legacytx.StdTx {
 	// NOTE: this uses StdTx explicitly, don't migrate it!
 	return legacytx.StdTx{
 		Msgs: []sdk.Msg{&types.MsgSend{}},
@@ -84,10 +84,49 @@ func mkTx() legacytx.StdTx {
 	}
 }
 
+// Create an IBC tx that's encoded as amino-JSON. Since we can't amino-marshal
+// a tx with "cosmos-sdk/MsgTransfer" using the SDK, we just hardcode the tx
+// here. But external clients might, see https://github.com/cosmos/cosmos-sdk/issues/8022.
+func mkIBCStdTx() []byte {
+	ibcTx := `{
+		"account_number": "68",
+		"chain_id": "stargate-4",
+		"fee": {
+		  "amount": [
+			{
+			  "amount": "3500",
+			  "denom": "umuon"
+			}
+		  ],
+		  "gas": "350000"
+		},
+		"memo": "",
+		"msg": [
+		  {
+			"type": "cosmos-sdk/MsgTransfer",
+			"value": {
+			  "receiver": "cosmos1q9wtnlwdjrhwtcjmt2uq77jrgx7z3usrq2yz7z",
+			  "sender": "cosmos1q9wtnlwdjrhwtcjmt2uq77jrgx7z3usrq2yz7z",
+			  "source_channel": "THEslipperCHANNEL",
+			  "source_port": "transfer",
+			  "token": {
+				"amount": "1000000",
+				"denom": "umuon"
+			  }
+			}
+		  }
+		],
+		"sequence": "24"
+	  }`
+	req := fmt.Sprintf(`{"tx":%s,"mode":"async"}`, ibcTx)
+
+	return []byte(req)
+}
+
 func (s *IntegrationTestSuite) TestEncodeDecode() {
 	var require = s.Require()
 	val := s.network.Validators[0]
-	stdTx := mkTx()
+	stdTx := mkStdTx()
 
 	// NOTE: this uses amino explicitly, don't migrate it!
 	cdc := val.ClientCtx.LegacyAmino
@@ -98,11 +137,11 @@ func (s *IntegrationTestSuite) TestEncodeDecode() {
 	res, err := rest.PostRequest(fmt.Sprintf("%s/txs/encode", val.APIAddress), "application/json", bz)
 	require.NoError(err)
 
-	var encodeResp rest2.EncodeResp
+	var encodeResp authrest.EncodeResp
 	err = cdc.UnmarshalJSON(res, &encodeResp)
 	require.NoError(err)
 
-	bz, err = cdc.MarshalJSON(rest2.DecodeReq{Tx: encodeResp.Tx})
+	bz, err = cdc.MarshalJSON(authrest.DecodeReq{Tx: encodeResp.Tx})
 	require.NoError(err)
 
 	res, err = rest.PostRequest(fmt.Sprintf("%s/txs/decode", val.APIAddress), "application/json", bz)
@@ -111,14 +150,24 @@ func (s *IntegrationTestSuite) TestEncodeDecode() {
 	var respWithHeight rest.ResponseWithHeight
 	err = cdc.UnmarshalJSON(res, &respWithHeight)
 	require.NoError(err)
-	var decodeResp rest2.DecodeResp
+	var decodeResp authrest.DecodeResp
 	err = cdc.UnmarshalJSON(respWithHeight.Result, &decodeResp)
 	require.NoError(err)
 	require.Equal(stdTx, legacytx.StdTx(decodeResp))
 }
 
+func (s *IntegrationTestSuite) TestEncodeIBCTx() {
+	val := s.network.Validators[0]
+
+	req := mkIBCStdTx()
+	res, err := rest.PostRequest(fmt.Sprintf("%s/txs/encode", val.APIAddress), "application/json", []byte(req))
+	s.Require().NoError(err)
+
+	s.Require().Contains(string(res), authrest.ErrEncodeDecode.Error())
+}
+
 func (s *IntegrationTestSuite) TestBroadcastTxRequest() {
-	stdTx := mkTx()
+	stdTx := mkStdTx()
 
 	// we just test with async mode because this tx will fail - all we care about is that it got encoded and broadcast correctly
 	res, err := s.broadcastReq(stdTx, "async")
@@ -130,6 +179,17 @@ func (s *IntegrationTestSuite) TestBroadcastTxRequest() {
 	s.Require().NotEmpty(txRes.TxHash)
 }
 
+func (s *IntegrationTestSuite) TestBroadcastIBCTxRequest() {
+	val := s.network.Validators[0]
+
+	req := mkIBCStdTx()
+	res, err := rest.PostRequest(fmt.Sprintf("%s/txs", val.APIAddress), "application/json", []byte(req))
+	s.Require().NoError(err)
+
+	// Make sure the error message is correct.
+	s.Require().Contains(string(res), "this transaction cannot be broadcasted via legacy REST endpoints")
+}
+
 // 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]
@@ -332,7 +392,7 @@ func (s *IntegrationTestSuite) broadcastReq(stdTx legacytx.StdTx, mode string) (
 
 	// NOTE: this uses amino explicitly, don't migrate it!
 	cdc := val.ClientCtx.LegacyAmino
-	req := rest2.BroadcastReq{
+	req := authrest.BroadcastReq{
 		Tx:   stdTx,
 		Mode: mode,
 	}
@@ -401,7 +461,7 @@ func (s *IntegrationTestSuite) testQueryIBCTx(txRes sdk.TxResponse, cmd *cobra.C
 	out, err = clitestutil.ExecTestCLICmd(val.ClientCtx, authcli.GetEncodeCommand(), []string{txFileName})
 	s.Require().NoError(err)
 
-	bz, err := val.ClientCtx.LegacyAmino.MarshalJSON(rest2.DecodeReq{Tx: string(out.Bytes())})
+	bz, err := val.ClientCtx.LegacyAmino.MarshalJSON(authrest.DecodeReq{Tx: string(out.Bytes())})
 	s.Require().NoError(err)
 
 	// try to decode the txn using legacy rest, it fails.

x/gov/client/rest/grpc_query_test.go

@@ -150,7 +150,7 @@ func (s *IntegrationTestSuite) TestGetProposalsGRPC() {
 func (s *IntegrationTestSuite) TestGetProposalVoteGRPC() {
 	val := s.network.Validators[0]
 
-	voterAddressBase64 := val.Address.String()
+	voterAddressBech32 := val.Address.String()
 
 	testCases := []struct {
 		name   string
@@ -159,12 +159,12 @@ func (s *IntegrationTestSuite) TestGetProposalVoteGRPC() {
 	}{
 		{
 			"empty proposal",
-			fmt.Sprintf("%s/cosmos/gov/v1beta1/proposals/%s/votes/%s", val.APIAddress, "", voterAddressBase64),
+			fmt.Sprintf("%s/cosmos/gov/v1beta1/proposals/%s/votes/%s", val.APIAddress, "", voterAddressBech32),
 			true,
 		},
 		{
 			"get non existing proposal",
-			fmt.Sprintf("%s/cosmos/gov/v1beta1/proposals/%s/votes/%s", val.APIAddress, "10", voterAddressBase64),
+			fmt.Sprintf("%s/cosmos/gov/v1beta1/proposals/%s/votes/%s", val.APIAddress, "10", voterAddressBech32),
 			true,
 		},
 		{
@@ -174,7 +174,7 @@ func (s *IntegrationTestSuite) TestGetProposalVoteGRPC() {
 		},
 		{
 			"get proposal with id",
-			fmt.Sprintf("%s/cosmos/gov/v1beta1/proposals/%s/votes/%s", val.APIAddress, "1", voterAddressBase64),
+			fmt.Sprintf("%s/cosmos/gov/v1beta1/proposals/%s/votes/%s", val.APIAddress, "1", voterAddressBech32),
 			false,
 		},
 	}