Prefer sending tx_bytes to Simulate gRPC endpoint (#8926)

* First run

* Remove dead code

* Make test pass

* Proto gen

* Fix lint

* Add changelog

* Fix tests

* Fix test

* Update x/auth/tx/service.go

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>

* Remove protoTxProvider

* Add grpc-gateway test

* Add comment

* move to api breaking

* lesser diff

* remove conflict

* empty commit to rerun CI

* empty commit to rerun CI

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>
Co-authored-by: Alessio Treglia <alessio@tendermint.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

CHANGELOG.md

@@ -65,6 +65,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (x/upgrade) [\#8743](https://github.com/cosmos/cosmos-sdk/pull/8743) `UpgradeHandler` includes a new argument `VersionMap` which helps facilitate in-place migrations.
 * (x/auth) [\#8129](https://github.com/cosmos/cosmos-sdk/pull/8828) Updated `SigVerifiableTx.GetPubKeys` method signature to return error.
 * [\#8682](https://github.com/cosmos/cosmos-sdk/pull/8682) `ante.NewAnteHandler` updated to receive all positional params as `ante.HandlerOptions` struct. If required fields aren't set, throws error accordingly.
+* (client) [\#8926](https://github.com/cosmos/cosmos-sdk/pull/8926) `client/tx.PrepareFactory` has been converted to a private function, as it's only used internally.
+* (auth/tx) [\#8926](https://github.com/cosmos/cosmos-sdk/pull/8926) The `ProtoTxProvider` interface used as a workaround for transaction simulation has been removed.
 
 ### State Machine Breaking
 
@@ -110,6 +112,10 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 This release fixes security vulnerability identified in the simapp.
 
+### Deprecated
+
+* (grpc) [\#8926](https://github.com/cosmos/cosmos-sdk/pull/8926) The `tx` field in `SimulateRequest` has been deprecated, prefer to pass `tx_bytes` instead.
+
 ## [v0.42.0](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.42.0) - 2021-03-08
 
 **IMPORTANT**: This release contains an important security fix for all non Cosmos Hub chains running Stargate version of the Cosmos SDK (>0.40). Non-hub chains should not be using any version of the SDK in the v0.40.x or v0.41.x release series. See [#8461](https://github.com/cosmos/cosmos-sdk/pull/8461) for more details.

client/tx/tx.go

@@ -2,11 +2,13 @@ package tx
 
 import (
 	"bufio"
+	"context"
 	"errors"
 	"fmt"
 	"net/http"
 	"os"
 
+	gogogrpc "github.com/gogo/protobuf/grpc"
 	"github.com/spf13/pflag"
 
 	"github.com/cosmos/cosmos-sdk/client"
@@ -20,7 +22,6 @@ import (
 	"github.com/cosmos/cosmos-sdk/types/tx"
 	"github.com/cosmos/cosmos-sdk/types/tx/signing"
 	authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing"
-	authtx "github.com/cosmos/cosmos-sdk/x/auth/tx"
 )
 
 // GenerateOrBroadcastTxCLI will either generate and print and unsigned transaction
@@ -50,7 +51,7 @@ func GenerateTx(clientCtx client.Context, txf Factory, msgs ...sdk.Msg) error {
 			return errors.New("cannot estimate gas in offline mode")
 		}
 
-		_, adjusted, err := CalculateGas(clientCtx.QueryWithData, txf, msgs...)
+		_, adjusted, err := CalculateGas(clientCtx, txf, msgs...)
 		if err != nil {
 			return err
 		}
@@ -76,13 +77,13 @@ func GenerateTx(clientCtx client.Context, txf Factory, msgs ...sdk.Msg) error {
 // given set of messages. It will also simulate gas requirements if necessary.
 // It will return an error upon failure.
 func BroadcastTx(clientCtx client.Context, txf Factory, msgs ...sdk.Msg) error {
-	txf, err := PrepareFactory(clientCtx, txf)
+	txf, err := prepareFactory(clientCtx, txf)
 	if err != nil {
 		return err
 	}
 
 	if txf.SimulateAndExecute() || clientCtx.Simulate {
-		_, adjusted, err := CalculateGas(clientCtx.QueryWithData, txf, msgs...)
+		_, adjusted, err := CalculateGas(clientCtx, txf, msgs...)
 		if err != nil {
 			return err
 		}
@@ -142,8 +143,9 @@ func BroadcastTx(clientCtx client.Context, txf Factory, msgs ...sdk.Msg) error {
 // BaseReq. Upon any error, the error will be written to the http.ResponseWriter.
 // Note that this function returns the legacy StdTx Amino JSON format for compatibility
 // with legacy clients.
+// Deprecated: We are removing Amino soon.
 func WriteGeneratedTxResponse(
-	ctx client.Context, w http.ResponseWriter, br rest.BaseReq, msgs ...sdk.Msg,
+	clientCtx client.Context, w http.ResponseWriter, br rest.BaseReq, msgs ...sdk.Msg,
 ) {
 	gasAdj, ok := rest.ParseFloat64OrReturnBadRequest(w, br.GasAdjustment, flags.DefaultGasAdjustment)
 	if !ok {
@@ -163,7 +165,7 @@ func WriteGeneratedTxResponse(
 		WithMemo(br.Memo).
 		WithChainID(br.ChainID).
 		WithSimulateAndExecute(br.Simulate).
-		WithTxConfig(ctx.TxConfig).
+		WithTxConfig(clientCtx.TxConfig).
 		WithTimeoutHeight(br.TimeoutHeight)
 
 	if br.Simulate || gasSetting.Simulate {
@@ -172,15 +174,15 @@ func WriteGeneratedTxResponse(
 			return
 		}
 
-		_, adjusted, err := CalculateGas(ctx.QueryWithData, txf, msgs...)
+		_, adjusted, err := CalculateGas(clientCtx, txf, msgs...)
 		if rest.CheckInternalServerError(w, err) {
 			return
 		}
 
 		txf = txf.WithGas(adjusted)
 
 		if br.Simulate {
-			rest.WriteSimulationResponse(w, ctx.LegacyAmino, txf.Gas())
+			rest.WriteSimulationResponse(w, clientCtx.LegacyAmino, txf.Gas())
 			return
 		}
 	}
@@ -190,12 +192,12 @@ func WriteGeneratedTxResponse(
 		return
 	}
 
-	stdTx, err := ConvertTxToStdTx(ctx.LegacyAmino, tx.GetTx())
+	stdTx, err := ConvertTxToStdTx(clientCtx.LegacyAmino, tx.GetTx())
 	if rest.CheckInternalServerError(w, err) {
 		return
 	}
 
-	output, err := ctx.LegacyAmino.MarshalJSON(stdTx)
+	output, err := clientCtx.LegacyAmino.MarshalJSON(stdTx)
 	if rest.CheckInternalServerError(w, err) {
 		return
 	}
@@ -268,46 +270,35 @@ func BuildSimTx(txf Factory, msgs ...sdk.Msg) ([]byte, error) {
 		return nil, err
 	}
 
-	protoProvider, ok := txb.(authtx.ProtoTxProvider)
-	if !ok {
-		return nil, fmt.Errorf("cannot simulate amino tx")
-	}
-	simReq := tx.SimulateRequest{Tx: protoProvider.GetProtoTx()}
-
-	return simReq.Marshal()
+	return txf.txConfig.TxEncoder()(txb.GetTx())
 }
 
 // CalculateGas simulates the execution of a transaction and returns the
 // simulation response obtained by the query and the adjusted gas amount.
 func CalculateGas(
-	queryFunc func(string, []byte) ([]byte, int64, error), txf Factory, msgs ...sdk.Msg,
-) (tx.SimulateResponse, uint64, error) {
+	clientCtx gogogrpc.ClientConn, txf Factory, msgs ...sdk.Msg,
+) (*tx.SimulateResponse, uint64, error) {
 	txBytes, err := BuildSimTx(txf, msgs...)
 	if err != nil {
-		return tx.SimulateResponse{}, 0, err
+		return nil, 0, err
 	}
 
-	// TODO This should use the generated tx service Client.
-	// https://github.com/cosmos/cosmos-sdk/issues/7726
-	bz, _, err := queryFunc("/cosmos.tx.v1beta1.Service/Simulate", txBytes)
+	txSvcClient := tx.NewServiceClient(clientCtx)
+	simRes, err := txSvcClient.Simulate(context.Background(), &tx.SimulateRequest{
+		TxBytes: txBytes,
+	})
 	if err != nil {
-		return tx.SimulateResponse{}, 0, err
-	}
-
-	var simRes tx.SimulateResponse
-
-	if err := simRes.Unmarshal(bz); err != nil {
-		return tx.SimulateResponse{}, 0, err
+		return nil, 0, err
 	}
 
 	return simRes, uint64(txf.GasAdjustment() * float64(simRes.GasInfo.GasUsed)), nil
 }
 
-// PrepareFactory ensures the account defined by ctx.GetFromAddress() exists and
+// prepareFactory ensures the account defined by ctx.GetFromAddress() exists and
 // if the account number and/or the account sequence number are zero (not set),
 // they will be queried for and set on the provided Factory. A new Factory with
 // the updated fields will be returned.
-func PrepareFactory(clientCtx client.Context, txf Factory) (Factory, error) {
+func prepareFactory(clientCtx client.Context, txf Factory) (Factory, error) {
 	from := clientCtx.GetFromAddress()
 
 	if err := txf.accountRetriever.EnsureExists(clientCtx, from); err != nil {

client/tx/tx_test.go

@@ -1,10 +1,12 @@
 package tx_test
 
 import (
-	"errors"
+	gocontext "context"
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"google.golang.org/grpc"
 
 	"github.com/cosmos/cosmos-sdk/client"
 	"github.com/cosmos/cosmos-sdk/client/tx"
@@ -24,30 +26,34 @@ func NewTestTxConfig() client.TxConfig {
 	return cfg.TxConfig
 }
 
-func TestCalculateGas(t *testing.T) {
-	makeQueryFunc := func(gasUsed uint64, wantErr bool) func(string, []byte) ([]byte, int64, error) {
-		return func(string, []byte) ([]byte, int64, error) {
-			if wantErr {
-				return nil, 0, errors.New("query failed")
-			}
-			simRes := &txtypes.SimulateResponse{
-				GasInfo: &sdk.GasInfo{GasUsed: gasUsed, GasWanted: gasUsed},
-				Result:  &sdk.Result{Data: []byte("tx data"), Log: "log"},
-			}
+// mockContext is a mock client.Context to return abitrary simulation response, used to
+// unit test CalculateGas.
+type mockContext struct {
+	gasUsed uint64
+	wantErr bool
+}
 
-			bz, err := simRes.Marshal()
-			if err != nil {
-				return nil, 0, err
-			}
+func (m mockContext) Invoke(grpcCtx gocontext.Context, method string, req, reply interface{}, opts ...grpc.CallOption) (err error) {
+	if m.wantErr {
+		return fmt.Errorf("mock err")
+	}
 
-			return bz, 0, nil
-		}
+	*(reply.(*txtypes.SimulateResponse)) = txtypes.SimulateResponse{
+		GasInfo: &sdk.GasInfo{GasUsed: m.gasUsed, GasWanted: m.gasUsed},
+		Result:  &sdk.Result{Data: []byte("tx data"), Log: "log"},
 	}
 
+	return nil
+}
+func (mockContext) NewStream(gocontext.Context, *grpc.StreamDesc, string, ...grpc.CallOption) (grpc.ClientStream, error) {
+	panic("not implemented")
+}
+
+func TestCalculateGas(t *testing.T) {
 	type args struct {
-		queryFuncGasUsed uint64
-		queryFuncWantErr bool
-		adjustment       float64
+		mockGasUsed uint64
+		mockWantErr bool
+		adjustment  float64
 	}
 
 	testCases := []struct {
@@ -70,16 +76,19 @@ func TestCalculateGas(t *testing.T) {
 			WithTxConfig(txCfg).WithSignMode(txCfg.SignModeHandler().DefaultMode())
 
 		t.Run(stc.name, func(t *testing.T) {
-			queryFunc := makeQueryFunc(stc.args.queryFuncGasUsed, stc.args.queryFuncWantErr)
-			simRes, gotAdjusted, err := tx.CalculateGas(queryFunc, txf.WithGasAdjustment(stc.args.adjustment))
+			mockClientCtx := mockContext{
+				gasUsed: tc.args.mockGasUsed,
+				wantErr: tc.args.mockWantErr,
+			}
+			simRes, gotAdjusted, err := tx.CalculateGas(mockClientCtx, txf.WithGasAdjustment(stc.args.adjustment))
 			if stc.expPass {
 				require.NoError(t, err)
 				require.Equal(t, simRes.GasInfo.GasUsed, stc.wantEstimate)
 				require.Equal(t, gotAdjusted, stc.wantAdjusted)
 				require.NotNil(t, simRes.Result)
 			} else {
 				require.Error(t, err)
-				require.Nil(t, simRes.Result)
+				require.Nil(t, simRes)
 			}
 		})
 	}

docs/core/proto-docs.md

@@ -7257,7 +7257,8 @@ RPC method.
 
 | Field | Type | Label | Description |
 | ----- | ---- | ----- | ----------- |
-| `tx` | [Tx](#cosmos.tx.v1beta1.Tx) |  | tx is the transaction to simulate. |
+| `tx` | [Tx](#cosmos.tx.v1beta1.Tx) |  | **Deprecated.** tx is the transaction to simulate. Deprecated. Send raw tx bytes instead. |
+| `tx_bytes` | [bytes](#bytes) |  | tx_bytes is the raw transaction. |
 
 
 

docs/run-node/txs.md

@@ -305,15 +305,13 @@ func simulateTx() error {
     // Simulate the tx via gRPC. We create a new client for the Protobuf Tx
     // service.
     txClient := tx.NewServiceClient(grpcConn)
-    // We then call the BroadcastTx method on this client.
-    protoTx := txBuilderToProtoTx(txBuilder)
-    if err != nil {
-        return err
-    }
+    txBytes := /* Fill in with your signed transaction bytes. */
+
+    // We then call the Simulate method on this client.
     grpcRes, err := txClient.Simulate(
         context.Background(),
         &tx.SimulateRequest{
-            Tx: protoTx,
+            TxBytes: txBytes,
         },
     )
     if err != nil {
@@ -324,16 +322,6 @@ func simulateTx() error {
 
     return nil
 }
-
-// txBuilderToProtoTx converts a txBuilder into a proto tx.Tx.
-func txBuilderToProtoTx(txBuilder client.TxBuilder) (*tx.Tx, error) { // nolint
-	protoProvider, ok := txBuilder.(authtx.ProtoTxProvider)
-	if !ok {
-		return nil, fmt.Errorf("expected proto tx builder, got %T", txBuilder)
-	}
-
-	return protoProvider.GetProtoTx(), nil
-}
 ```
 
 ## Using REST

proto/cosmos/tx/v1beta1/service.proto

@@ -101,7 +101,10 @@ message BroadcastTxResponse {
 // RPC method.
 message SimulateRequest {
   // tx is the transaction to simulate.
-  cosmos.tx.v1beta1.Tx tx = 1;
+  // Deprecated. Send raw tx bytes instead.
+  cosmos.tx.v1beta1.Tx tx = 1 [deprecated=true];
+  // tx_bytes is the raw transaction.
+  bytes tx_bytes = 2;
 }
 
 // SimulateResponse is the response type for the

simapp/simd/cmd/root.go

@@ -25,7 +25,6 @@ import (
 	"github.com/cosmos/cosmos-sdk/snapshots"
 	"github.com/cosmos/cosmos-sdk/store"
 	sdk "github.com/cosmos/cosmos-sdk/types"
-	authclient "github.com/cosmos/cosmos-sdk/x/auth/client"
 	authcmd "github.com/cosmos/cosmos-sdk/x/auth/client/cli"
 	"github.com/cosmos/cosmos-sdk/x/auth/types"
 	vestingcli "github.com/cosmos/cosmos-sdk/x/auth/vesting/client/cli"
@@ -66,8 +65,6 @@ func NewRootCmd() (*cobra.Command, params.EncodingConfig) {
 }
 
 func initRootCmd(rootCmd *cobra.Command, encodingConfig params.EncodingConfig) {
-	authclient.Codec = encodingConfig.Marshaler
-
 	rootCmd.AddCommand(
 		genutilcli.InitCmd(simapp.ModuleBasics, simapp.DefaultNodeHome),
 		genutilcli.CollectGenTxsCmd(banktypes.GenesisBalancesIterator{}, simapp.DefaultNodeHome),