Audit through legacy endpoints to find breaking changes (#8037)

* 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>
This commit is contained in:
Amaury
2020-12-03 23:33:32 +00:00
committed by GitHub
co-authored by mergify[bot]
parent 379c2ad630
commit 1b00c01b70
11 changed files with 300 additions and 30 deletions
+1
View File
@@ -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
+9 -6
View File
@@ -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.
+11 -2
View File
@@ -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)
+1 -1
View File
@@ -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())
+15 -4
View File
@@ -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
+4 -4
View File
@@ -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/txs/{txhash}")
err = checkAminoMarshalError(clientCtx, output, "/cosmos/tx/v1beta1/txs/{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
+69 -9
View File
@@ -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.
+4 -4
View File
@@ -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,
},
}
+110
View File
@@ -0,0 +1,110 @@
// +build norace
package rest_test
import (
"fmt"
"github.com/cosmos/cosmos-sdk/types/rest"
"github.com/cosmos/cosmos-sdk/x/gov/types"
)
func (s *IntegrationTestSuite) TestLegacyGetVote() {
val := s.network.Validators[0]
voterAddressBech32 := val.Address.String()
testCases := []struct {
name string
url string
expErr bool
expErrMsg string
}{
{
"get non existing proposal",
fmt.Sprintf("%s/gov/proposals/%s/votes/%s", val.APIAddress, "10", voterAddressBech32),
true, "proposalID 10 does not exist",
},
{
"get proposal with wrong voter address",
fmt.Sprintf("%s/gov/proposals/%s/votes/%s", val.APIAddress, "1", "wrongVoterAddress"),
true, "decoding bech32 failed: string not all lowercase or all uppercase",
},
{
"get proposal with id",
fmt.Sprintf("%s/gov/proposals/%s/votes/%s", val.APIAddress, "1", voterAddressBech32),
false, "",
},
}
for _, tc := range testCases {
tc := tc
s.Run(tc.name, func() {
respJSON, err := rest.GetRequest(tc.url)
s.Require().NoError(err)
if tc.expErr {
var errResp rest.ErrorResponse
s.Require().NoError(val.ClientCtx.LegacyAmino.UnmarshalJSON(respJSON, &errResp))
s.Require().Equal(errResp.Error, tc.expErrMsg)
} else {
var resp = rest.ResponseWithHeight{}
err = val.ClientCtx.LegacyAmino.UnmarshalJSON(respJSON, &resp)
s.Require().NoError(err)
// Check result is not empty.
var vote types.Vote
s.Require().NoError(val.ClientCtx.LegacyAmino.UnmarshalJSON(resp.Result, &vote))
s.Require().Equal(val.Address.String(), vote.Voter)
// Note that option is now an int.
s.Require().Equal(types.VoteOption(1), vote.Option)
}
})
}
}
func (s *IntegrationTestSuite) TestLegacyGetVotes() {
val := s.network.Validators[0]
testCases := []struct {
name string
url string
expErr bool
expErrMsg string
}{
{
"votes with empty proposal id",
fmt.Sprintf("%s/gov/proposals/%s/votes", val.APIAddress, ""),
true, "'votes' is not a valid uint64",
},
{
"get votes with valid id",
fmt.Sprintf("%s/gov/proposals/%s/votes", val.APIAddress, "1"),
false, "",
},
}
for _, tc := range testCases {
tc := tc
s.Run(tc.name, func() {
respJSON, err := rest.GetRequest(tc.url)
s.Require().NoError(err)
if tc.expErr {
var errResp rest.ErrorResponse
s.Require().NoError(val.ClientCtx.LegacyAmino.UnmarshalJSON(respJSON, &errResp))
s.Require().Equal(errResp.Error, tc.expErrMsg)
} else {
var resp = rest.ResponseWithHeight{}
err = val.ClientCtx.LegacyAmino.UnmarshalJSON(respJSON, &resp)
s.Require().NoError(err)
// Check result is not empty.
var votes []types.Vote
s.Require().NoError(val.ClientCtx.LegacyAmino.UnmarshalJSON(resp.Result, &votes))
s.Require().Greater(len(votes), 0)
}
})
}
}
+14
View File
@@ -9,6 +9,7 @@ import (
"github.com/gorilla/mux"
"github.com/cosmos/cosmos-sdk/client"
clientrest "github.com/cosmos/cosmos-sdk/client/rest"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/rest"
"github.com/cosmos/cosmos-sdk/x/staking/types"
@@ -278,6 +279,19 @@ func validatorsHandlerFn(clientCtx client.Context) http.HandlerFunc {
}
status := r.FormValue("status")
// These are query params that were available in =<0.39. We show a nice
// error message for this breaking change.
if status == "bonded" || status == "unbonding" || status == "unbonded" {
err := fmt.Errorf("cosmos sdk v0.40 introduces a breaking change on this endpoint:"+
" instead of querying using `?status=%s`, please use `status=BOND_STATUS_%s`. For more"+
" info, please see our REST endpoint migration guide at %s", status, strings.ToUpper(status), clientrest.DeprecationURL)
if rest.CheckBadRequestError(w, err) {
return
}
}
if status == "" {
status = types.BondStatusBonded
}
+62
View File
@@ -0,0 +1,62 @@
// +build norace
package rest_test
import (
"fmt"
"github.com/cosmos/cosmos-sdk/types/rest"
"github.com/cosmos/cosmos-sdk/x/staking/types"
)
func (s *IntegrationTestSuite) TestLegacyGetValidators() {
val := s.network.Validators[0]
baseURL := val.APIAddress
testCases := []struct {
name string
url string
expErr bool
expErrMsg string
}{
{
"old status should show error message",
fmt.Sprintf("%s/staking/validators?status=bonded", baseURL),
true, "cosmos sdk v0.40 introduces a breaking change on this endpoint: instead of" +
" querying using `?status=bonded`, please use `status=BOND_STATUS_BONDED`. For more" +
" info, please see our REST endpoint migration guide at https://docs.cosmos.network/master/migrations/rest.html",
},
{
"new status should work",
fmt.Sprintf("%s/staking/validators?status=BOND_STATUS_BONDED", baseURL),
false, "",
},
}
for _, tc := range testCases {
tc := tc
s.Run(tc.name, func() {
respJSON, err := rest.GetRequest(tc.url)
s.Require().NoError(err)
if tc.expErr {
var errResp rest.ErrorResponse
s.Require().NoError(val.ClientCtx.LegacyAmino.UnmarshalJSON(respJSON, &errResp))
s.Require().Equal(errResp.Error, tc.expErrMsg)
} else {
var resp = rest.ResponseWithHeight{}
err = val.ClientCtx.LegacyAmino.UnmarshalJSON(respJSON, &resp)
s.Require().NoError(err)
// Check result is not empty.
var validators []types.Validator
s.Require().NoError(val.ClientCtx.LegacyAmino.UnmarshalJSON(resp.Result, &validators))
s.Require().Greater(len(validators), 0)
// While we're at it, also check that the consensus_pubkey is
// an Any, and not bech32 anymore.
s.Require().Contains(string(resp.Result), "\"consensus_pubkey\": {\n \"type\": \"tendermint/PubKeyEd25519\",")
}
})
}
}