Skip to content

Commit

Permalink
Audit through legacy endpoints to find breaking changes (#8037)
Browse files Browse the repository at this point in the history 
* Add error msg on staking

* Add tests for legacy staking and gov

* Add test for encode

* Fix broadcast too

* Add comments

* update changelog

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
  • Loading branch information
@amaury1093 @mergify
amaury1093 and mergify[bot] authored Dec 3, 2020
1 parent 379c2ad commit 1b00c01
Show file tree
Hide file tree
Showing 11 changed files with 300 additions and 30 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,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
Expand Down
15 changes: 9 additions & 6 deletions docs/migrations/rest.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
13 changes: 11 additions & 2 deletions x/auth/client/rest/broadcast.go
View file
Original file line number Diff line number Diff line change
@@ -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"
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion x/auth/client/rest/decode.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())

Expand Down
19 changes: 15 additions & 4 deletions x/auth/client/rest/encode.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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.
Expand All @@ -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
Expand Down
8 changes: 4 additions & 4 deletions x/auth/client/rest/query.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())

Expand Down Expand Up @@ -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/txs/{txhash}")
err = checkAminoMarshalError(clientCtx, output, "/cosmos/tx/v1beta1/txs/{txhash}")
if err != nil {
rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())

Expand Down Expand Up @@ -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

Expand Down
78 changes: 69 additions & 9 deletions x/auth/client/rest/rest_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{}},
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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")
Expand All @@ -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]
Expand Down Expand Up @@ -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,
}
Expand Down Expand Up @@ -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.
Expand Down
8 changes: 4 additions & 4 deletions x/gov/client/rest/grpc_query_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
},
{
Expand All @@ -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,
},
}
Expand Down
Loading

0 comments on commit 1b00c01

Please sign in to comment.