From fe36f9c414e6b5eec369f566644f76a8ffd89394 Mon Sep 17 00:00:00 2001 From: Amaury Martiny Date: Thu, 29 Oct 2020 14:50:40 +0100 Subject: [PATCH 1/5] Add test --- x/auth/client/rest/decode.go | 19 ++++++++--------- x/auth/client/rest/query.go | 33 ++++++++++++++++++++++++------ x/auth/client/rest/rest_test.go | 36 ++++++++++++++++++++++++++++++++- 3 files changed, 72 insertions(+), 16 deletions(-) diff --git a/x/auth/client/rest/decode.go b/x/auth/client/rest/decode.go index ca72487a80a8..a3aa5ec750b0 100644 --- a/x/auth/client/rest/decode.go +++ b/x/auth/client/rest/decode.go @@ -6,12 +6,12 @@ import ( "io/ioutil" "net/http" - "github.com/cosmos/cosmos-sdk/x/auth/signing" - "github.com/cosmos/cosmos-sdk/client" clienttx "github.com/cosmos/cosmos-sdk/client/tx" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/types/rest" "github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx" + "github.com/cosmos/cosmos-sdk/x/auth/signing" ) type ( @@ -47,8 +47,9 @@ func DecodeTxRequestHandlerFn(clientCtx client.Context) http.HandlerFunc { return } - stdTx, ok := convertToStdTx(w, clientCtx, txBytes) - if !ok { + stdTx, err := convertToStdTx(w, clientCtx, txBytes) + if err != nil { + // Error is already returned by convertToStdTx. return } @@ -61,22 +62,22 @@ func DecodeTxRequestHandlerFn(clientCtx client.Context) http.HandlerFunc { // convertToStdTx converts tx proto binary bytes retrieved from Tendermint into // a StdTx. Returns the StdTx, as well as a flag denoting if the function // successfully converted or not. -func convertToStdTx(w http.ResponseWriter, clientCtx client.Context, txBytes []byte) (legacytx.StdTx, bool) { +func convertToStdTx(w http.ResponseWriter, clientCtx client.Context, txBytes []byte) (legacytx.StdTx, error) { txI, err := clientCtx.TxConfig.TxDecoder()(txBytes) if rest.CheckBadRequestError(w, err) { - return legacytx.StdTx{}, false + return legacytx.StdTx{}, err } tx, ok := txI.(signing.Tx) if !ok { rest.WriteErrorResponse(w, http.StatusBadRequest, fmt.Sprintf("%+v is not backwards compatible with %T", tx, legacytx.StdTx{})) - return legacytx.StdTx{}, false + return legacytx.StdTx{}, sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "expected %T, got %T", (signing.Tx)(nil), txI) } stdTx, err := clienttx.ConvertTxToStdTx(clientCtx.LegacyAmino, tx) if rest.CheckBadRequestError(w, err) { - return legacytx.StdTx{}, false + return legacytx.StdTx{}, err } - return stdTx, true + return stdTx, nil } diff --git a/x/auth/client/rest/query.go b/x/auth/client/rest/query.go index c40fc5530d77..23afef323c2e 100644 --- a/x/auth/client/rest/query.go +++ b/x/auth/client/rest/query.go @@ -9,6 +9,7 @@ import ( "github.com/gorilla/mux" "github.com/cosmos/cosmos-sdk/client" + codectypes "github.com/cosmos/cosmos-sdk/codec/types" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/rest" authclient "github.com/cosmos/cosmos-sdk/x/auth/client" @@ -102,6 +103,10 @@ func QueryTxsRequestHandlerFn(clientCtx client.Context) http.HandlerFunc { return } + for _, txRes := range searchResult.Txs { + packStdTxResponse(w, clientCtx, txRes) + } + rest.PostProcessResponseBare(w, clientCtx, searchResult) } } @@ -128,11 +133,9 @@ func QueryTxRequestHandlerFn(clientCtx client.Context) http.HandlerFunc { return } - // We just unmarshalled from Tendermint, we take the proto Tx's raw - // bytes, and convert them into a StdTx to be displayed. - txBytes := output.Tx.Value - stdTx, ok := convertToStdTx(w, clientCtx, txBytes) - if !ok { + err = packStdTxResponse(w, clientCtx, output) + if err != nil { + // Error is already returned by packStdTxResponse. return } @@ -140,7 +143,7 @@ func QueryTxRequestHandlerFn(clientCtx client.Context) http.HandlerFunc { rest.WriteErrorResponse(w, http.StatusNotFound, fmt.Sprintf("no transaction found with hash %s", hashHexStr)) } - rest.PostProcessResponseBare(w, clientCtx, stdTx) + rest.PostProcessResponseBare(w, clientCtx, output) } } @@ -161,3 +164,21 @@ func queryParamsHandler(clientCtx client.Context) http.HandlerFunc { rest.PostProcessResponse(w, clientCtx, res) } } + +// packStdTxResponse takes a sdk.TxResponse, converts the Tx into a StdTx, and +// packs the StdTx again into the sdk.TxResponse Any. Amino then takes care of +// seamlessly JSON-outputting the Any. +func packStdTxResponse(w http.ResponseWriter, clientCtx client.Context, txRes *sdk.TxResponse) error { + // We just unmarshalled from Tendermint, we take the proto Tx's raw + // bytes, and convert them into a StdTx to be displayed. + txBytes := txRes.Tx.Value + stdTx, err := convertToStdTx(w, clientCtx, txBytes) + if err != nil { + return err + } + + // Pack the amino stdTx into the TxResponse's Any. + txRes.Tx = codectypes.UnsafePackAny(stdTx) + + return nil +} diff --git a/x/auth/client/rest/rest_test.go b/x/auth/client/rest/rest_test.go index 1c8d4924266c..23e5030e15ea 100644 --- a/x/auth/client/rest/rest_test.go +++ b/x/auth/client/rest/rest_test.go @@ -119,7 +119,7 @@ func (s *IntegrationTestSuite) TestQueryTxByHash() { s.network.WaitForNextBlock() - // We now fetch the tx by has on the `/tx/{hash}` route. + // We now fetch the tx by hash on the `/tx/{hash}` route. txJSON, err := rest.GetRequest(fmt.Sprintf("%s/txs/%s", val0.APIAddress, txRes.TxHash)) s.Require().NoError(err) @@ -128,6 +128,40 @@ func (s *IntegrationTestSuite) TestQueryTxByHash() { s.Require().True(strings.Contains(string(txJSON), stdTx.Memo)) } +func (s *IntegrationTestSuite) TestQueryTxByHeight() { + val0 := s.network.Validators[0] + + // Create and broadcast a tx. + stdTx := s.createTestStdTx(val0, 1) // Validator's sequence starts at 1. + res, err := s.broadcastReq(stdTx, "block") + s.Require().NoError(err) + var txRes sdk.TxResponse + // NOTE: this uses amino explicitly, don't migrate it! + s.Require().NoError(s.cfg.LegacyAmino.UnmarshalJSON(res, &txRes)) + // we just check for a non-empty height here, as we'll need to for querying. + s.Require().NotEmpty(txRes.Height) + + s.network.WaitForNextBlock() + + // We now fetch the tx on `/txs` route, filtering by `tx.height` + txJSON, err := rest.GetRequest(fmt.Sprintf("%s/txs?limit=100&page=1&tx.height=%d", val0.APIAddress, txRes.Height)) + s.Require().NoError(err) + + // txJSON should contain the whole tx, we just make sure that our custom + // memo is there. + s.Require().True(strings.Contains(string(txJSON), stdTx.Memo)) + + // We now fetch the tx on `/txs` route, filtering by `height` + txJSON, err = rest.GetRequest(fmt.Sprintf("%s/txs?height=%d", val0.APIAddress, txRes.Height)) + s.Require().NoError(err) + + fmt.Println(string(txJSON)) // TODO This one is empty. + + // txJSON should contain the whole tx, we just make sure that our custom + // memo is there. + s.Require().True(strings.Contains(string(txJSON), stdTx.Memo)) +} + func (s *IntegrationTestSuite) TestMultipleSyncBroadcastTxRequests() { // First test transaction from validator should have sequence=1 (non-genesis tx) testCases := []struct { From 6431221381605c176fb07fb06238b9fc7edea826 Mon Sep 17 00:00:00 2001 From: Amaury Martiny Date: Thu, 29 Oct 2020 15:37:15 +0100 Subject: [PATCH 2/5] Fix potential flakiness in sequence --- x/auth/client/rest/rest_test.go | 60 +++++++++++++++------------------ 1 file changed, 28 insertions(+), 32 deletions(-) diff --git a/x/auth/client/rest/rest_test.go b/x/auth/client/rest/rest_test.go index 23e5030e15ea..3dc9c4d6dbf9 100644 --- a/x/auth/client/rest/rest_test.go +++ b/x/auth/client/rest/rest_test.go @@ -4,8 +4,6 @@ import ( "fmt" "testing" - "strings" - "github.com/stretchr/testify/suite" "github.com/cosmos/cosmos-sdk/client/tx" @@ -26,6 +24,9 @@ type IntegrationTestSuite struct { cfg network.Config network *network.Network + + stdTx legacytx.StdTx + stdTxRes sdk.TxResponse } func (s *IntegrationTestSuite) SetupSuite() { @@ -39,6 +40,17 @@ func (s *IntegrationTestSuite) SetupSuite() { _, err := s.network.WaitForHeight(1) s.Require().NoError(err) + + // Broadcast a StdTx used for tests. + s.stdTx = s.createTestStdTx(s.network.Validators[0], 1) + res, err := s.broadcastReq(s.stdTx, "block") + 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.network.WaitForNextBlock()) } func (s *IntegrationTestSuite) TearDownSuite() { @@ -107,63 +119,48 @@ func (s *IntegrationTestSuite) TestBroadcastTxRequest() { func (s *IntegrationTestSuite) TestQueryTxByHash() { val0 := s.network.Validators[0] - // Create and broadcast a tx. - stdTx := s.createTestStdTx(val0, 1) // Validator's sequence starts at 1. - res, err := s.broadcastReq(stdTx, "block") - s.Require().NoError(err) - var txRes sdk.TxResponse - // NOTE: this uses amino explicitly, don't migrate it! - s.Require().NoError(s.cfg.LegacyAmino.UnmarshalJSON(res, &txRes)) + // 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(txRes.TxHash) - - s.network.WaitForNextBlock() + 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, txRes.TxHash)) + txJSON, err := rest.GetRequest(fmt.Sprintf("%s/txs/%s", val0.APIAddress, s.stdTxRes.TxHash)) s.Require().NoError(err) // txJSON should contain the whole tx, we just make sure that our custom // memo is there. - s.Require().True(strings.Contains(string(txJSON), stdTx.Memo)) + s.Require().Contains(string(txJSON), s.stdTx.Memo) } func (s *IntegrationTestSuite) TestQueryTxByHeight() { val0 := s.network.Validators[0] - // Create and broadcast a tx. - stdTx := s.createTestStdTx(val0, 1) // Validator's sequence starts at 1. - res, err := s.broadcastReq(stdTx, "block") - s.Require().NoError(err) - var txRes sdk.TxResponse - // NOTE: this uses amino explicitly, don't migrate it! - s.Require().NoError(s.cfg.LegacyAmino.UnmarshalJSON(res, &txRes)) + // 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(txRes.Height) - - s.network.WaitForNextBlock() + 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=100&page=1&tx.height=%d", val0.APIAddress, txRes.Height)) + txJSON, err := rest.GetRequest(fmt.Sprintf("%s/txs?limit=100&page=1&tx.height=%d", val0.APIAddress, s.stdTxRes.Height)) s.Require().NoError(err) // txJSON should contain the whole tx, we just make sure that our custom // memo is there. - s.Require().True(strings.Contains(string(txJSON), stdTx.Memo)) + s.Require().Contains(string(txJSON), s.stdTx.Memo) // We now fetch the tx on `/txs` route, filtering by `height` - txJSON, err = rest.GetRequest(fmt.Sprintf("%s/txs?height=%d", val0.APIAddress, txRes.Height)) + txJSON, err = rest.GetRequest(fmt.Sprintf("%s/txs?height=%d", val0.APIAddress, s.stdTxRes.Height)) s.Require().NoError(err) fmt.Println(string(txJSON)) // TODO This one is empty. // txJSON should contain the whole tx, we just make sure that our custom // memo is there. - s.Require().True(strings.Contains(string(txJSON), stdTx.Memo)) + s.Require().Contains(string(txJSON), s.stdTx.Memo) } func (s *IntegrationTestSuite) TestMultipleSyncBroadcastTxRequests() { - // First test transaction from validator should have sequence=1 (non-genesis tx) + // We already sent one tx in SetupSuite with sequence=1 (non-genesis tx). + // Therefore, we're starting this test with sequence=2. testCases := []struct { desc string sequence uint64 @@ -171,12 +168,12 @@ func (s *IntegrationTestSuite) TestMultipleSyncBroadcastTxRequests() { }{ { "First tx (correct sequence)", - 1, + 2, false, }, { "Second tx (correct sequence)", - 2, + 3, false, }, { @@ -187,7 +184,6 @@ func (s *IntegrationTestSuite) TestMultipleSyncBroadcastTxRequests() { } for _, tc := range testCases { s.Run(fmt.Sprintf("Case %s", tc.desc), func() { - // broadcast test with sync mode, as we want to run CheckTx to verify account sequence is correct stdTx := s.createTestStdTx(s.network.Validators[0], tc.sequence) res, err := s.broadcastReq(stdTx, "sync") From 0a7cb8f25283c0331ba51155022fb09b73d24174 Mon Sep 17 00:00:00 2001 From: Amaury Martiny Date: Thu, 29 Oct 2020 16:59:46 +0100 Subject: [PATCH 3/5] Remove test with ?height --- x/auth/client/rest/rest_test.go | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/x/auth/client/rest/rest_test.go b/x/auth/client/rest/rest_test.go index 3dc9c4d6dbf9..c7aa1e4110e8 100644 --- a/x/auth/client/rest/rest_test.go +++ b/x/auth/client/rest/rest_test.go @@ -140,22 +140,12 @@ func (s *IntegrationTestSuite) TestQueryTxByHeight() { 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=100&page=1&tx.height=%d", val0.APIAddress, s.stdTxRes.Height)) + txJSON, err := rest.GetRequest(fmt.Sprintf("%s/txs?limit=10&page=1&tx.height=%d", val0.APIAddress, s.stdTxRes.Height)) s.Require().NoError(err) // txJSON should contain the whole tx, we just make sure that our custom // memo is there. s.Require().Contains(string(txJSON), s.stdTx.Memo) - - // We now fetch the tx on `/txs` route, filtering by `height` - txJSON, err = rest.GetRequest(fmt.Sprintf("%s/txs?height=%d", val0.APIAddress, s.stdTxRes.Height)) - s.Require().NoError(err) - - fmt.Println(string(txJSON)) // TODO This one is empty. - - // txJSON should contain the whole tx, we just make sure that our custom - // memo is there. - s.Require().Contains(string(txJSON), s.stdTx.Memo) } func (s *IntegrationTestSuite) TestMultipleSyncBroadcastTxRequests() { From b979eb7aa8929b49992a442e13bc431a71e93a0c Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Fri, 30 Oct 2020 12:36:53 -0400 Subject: [PATCH 4/5] Fix test --- x/auth/client/rest/rest_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/x/auth/client/rest/rest_test.go b/x/auth/client/rest/rest_test.go index 3673d6ffdb91..86085d573b72 100644 --- a/x/auth/client/rest/rest_test.go +++ b/x/auth/client/rest/rest_test.go @@ -184,12 +184,13 @@ func (s *IntegrationTestSuite) TestQueryTxByHashWithServiceMessage() { txJSON, err := rest.GetRequest(fmt.Sprintf("%s/txs/%s", val.APIAddress, txRes.TxHash)) s.Require().NoError(err) - var result legacytx.StdTx - s.Require().NoError(val.ClientCtx.LegacyAmino.UnmarshalJSON(txJSON, &result)) - s.Require().NotNil(result) - msgs := result.GetMsgs() + 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) + _, ok = msgs[0].(*types.MsgSend) s.Require().True(ok) } From d0c688ad2ac05b1b6bdea24ca305a21eced87631 Mon Sep 17 00:00:00 2001 From: Amaury Martiny Date: Mon, 2 Nov 2020 12:36:33 +0100 Subject: [PATCH 5/5] Fix tests --- x/auth/client/rest/rest_test.go | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/x/auth/client/rest/rest_test.go b/x/auth/client/rest/rest_test.go index 86085d573b72..86216dd53b7c 100644 --- a/x/auth/client/rest/rest_test.go +++ b/x/auth/client/rest/rest_test.go @@ -50,7 +50,7 @@ func (s *IntegrationTestSuite) SetupSuite() { s.Require().NoError(err) // Broadcast a StdTx used for tests. - s.stdTx = s.createTestStdTx(s.network.Validators[0], 1) + s.stdTx = s.createTestStdTx(s.network.Validators[0], 0, 1) res, err := s.broadcastReq(s.stdTx, "block") s.Require().NoError(err) @@ -159,15 +159,16 @@ func (s *IntegrationTestSuite) TestQueryTxByHeight() { func (s *IntegrationTestSuite) TestQueryTxByHashWithServiceMessage() { val := s.network.Validators[0] - account, err := val.ClientCtx.Keyring.Key("newAccount") - s.Require().NoError(err) - sendTokens := sdk.NewInt64Coin(s.cfg.BondDenom, 10) + // Right after this line, we're sending a tx. Might need to wait a block + // to refresh sequences. + s.Require().NoError(s.network.WaitForNextBlock()) + out, err := bankcli.ServiceMsgSendExec( val.ClientCtx, val.Address, - account.GetAddress(), + val.Address, sdk.NewCoins(sendTokens), fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), @@ -178,6 +179,7 @@ func (s *IntegrationTestSuite) TestQueryTxByHashWithServiceMessage() { s.Require().NoError(err) var txRes sdk.TxResponse s.Require().NoError(val.ClientCtx.JSONMarshaler.UnmarshalJSON(out.Bytes(), &txRes)) + s.Require().Equal(uint32(0), txRes.Code) s.Require().NoError(s.network.WaitForNextBlock()) @@ -195,8 +197,7 @@ func (s *IntegrationTestSuite) TestQueryTxByHashWithServiceMessage() { } func (s *IntegrationTestSuite) TestMultipleSyncBroadcastTxRequests() { - // We already sent one tx in SetupSuite with sequence=1 (non-genesis tx). - // Therefore, we're starting this test with sequence=2. + // First test transaction from validator should have sequence=1 (non-genesis tx) testCases := []struct { desc string sequence uint64 @@ -204,12 +205,12 @@ func (s *IntegrationTestSuite) TestMultipleSyncBroadcastTxRequests() { }{ { "First tx (correct sequence)", - 2, + 1, false, }, { "Second tx (correct sequence)", - 3, + 2, false, }, { @@ -221,7 +222,7 @@ func (s *IntegrationTestSuite) TestMultipleSyncBroadcastTxRequests() { for _, tc := range testCases { s.Run(fmt.Sprintf("Case %s", tc.desc), func() { // broadcast test with sync mode, as we want to run CheckTx to verify account sequence is correct - stdTx := s.createTestStdTx(s.network.Validators[0], tc.sequence) + stdTx := s.createTestStdTx(s.network.Validators[1], 1, tc.sequence) res, err := s.broadcastReq(stdTx, "sync") s.Require().NoError(err) @@ -246,7 +247,7 @@ func (s *IntegrationTestSuite) TestMultipleSyncBroadcastTxRequests() { } } -func (s *IntegrationTestSuite) createTestStdTx(val *network.Validator, sequence uint64) legacytx.StdTx { +func (s *IntegrationTestSuite) createTestStdTx(val *network.Validator, accNum, sequence uint64) legacytx.StdTx { txConfig := legacytx.StdTxConfig{Cdc: s.cfg.LegacyAmino} msg := &types.MsgSend{ @@ -270,6 +271,7 @@ func (s *IntegrationTestSuite) createTestStdTx(val *network.Validator, sequence WithKeybase(val.ClientCtx.Keyring). WithTxConfig(txConfig). WithSignMode(signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON). + WithAccountNumber(accNum). WithSequence(sequence) // sign Tx (offline mode so we can manually set sequence number)