From ceba51a414f339dd3bfc0e45e41f1fb005e635a7 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Tue, 11 Sep 2018 12:27:20 +0100 Subject: [PATCH 1/6] Change --gas=0 semantic and introduce --gas=simulate Make --gas flag accept a conventional "simulate" string value in addition to integers. Passing --gas=simulate would trigger the tx simulation and set the gas according to the gas estimate returned by the simulation. Any other integer value passed to --gas would be interpreted as-is and and set as gas wanted value. Closes: #2300 --- PENDING.md | 5 ++- client/context/context.go | 10 ----- client/flags.go | 62 +++++++++++++++++++++++++--- client/lcd/lcd_test.go | 24 ++++++----- client/utils/utils.go | 28 ++++++------- cmd/gaia/cli_test/cli_test.go | 8 +++- docs/light/api.md | 4 +- docs/sdk/clients.md | 2 +- x/auth/client/txbuilder/txbuilder.go | 6 ++- x/bank/client/rest/sendtx.go | 30 ++++++++------ x/gov/client/rest/util.go | 31 ++++++++------ x/ibc/client/rest/transfer.go | 23 +++++++---- x/slashing/client/rest/tx.go | 12 +++--- x/stake/client/rest/tx.go | 34 ++++++++------- 14 files changed, 176 insertions(+), 103 deletions(-) diff --git a/PENDING.md b/PENDING.md index b82f34f2e6d1..aec65f3d563b 100644 --- a/PENDING.md +++ b/PENDING.md @@ -61,8 +61,9 @@ FEATURES * [gov][cli] #2062 added `--proposal` flag to `submit-proposal` that allows a JSON file containing a proposal to be passed in * [\#2040](https://github.com/cosmos/cosmos-sdk/issues/2040) Add `--bech` to `gaiacli keys show` and respective REST endpoint to provide desired Bech32 prefix encoding - * [cli] [\#2047](https://github.com/cosmos/cosmos-sdk/issues/2047) Setting the --gas flag value to 0 triggers a simulation of the tx before the actual execution. The gas estimate obtained via the simulation will be used as gas limit in the actual execution. - * [cli] [\#2047](https://github.com/cosmos/cosmos-sdk/issues/2047) The --gas-adjustment flag can be used to adjust the estimate obtained via the simulation triggered by --gas=0. + * [cli] [\#2047](https://github.com/cosmos/cosmos-sdk/issues/2047) [\#2306](https://github.com/cosmos/cosmos-sdk/pull/2306) Passing --gas=simulate triggers a simulation of the tx before the actual execution. + The gas estimate obtained via the simulation will be used as gas limit in the actual execution. + * [cli] [\#2047](https://github.com/cosmos/cosmos-sdk/issues/2047) The --gas-adjustment flag can be used to adjust the estimate obtained via the simulation triggered by --gas=simulate. * [cli] [\#2110](https://github.com/cosmos/cosmos-sdk/issues/2110) Add --dry-run flag to perform a simulation of a transaction without broadcasting it. The --gas flag is ignored as gas would be automatically estimated. * [cli] [\#2204](https://github.com/cosmos/cosmos-sdk/issues/2204) Support generating and broadcasting messages with multiple signatures via command line: * [\#966](https://github.com/cosmos/cosmos-sdk/issues/966) Add --generate-only flag to build an unsigned transaction and write it to STDOUT. diff --git a/client/context/context.go b/client/context/context.go index a4d4afd3b347..3e785a28e2ff 100644 --- a/client/context/context.go +++ b/client/context/context.go @@ -27,8 +27,6 @@ type CLIContext struct { Client rpcclient.Client Logger io.Writer Height int64 - Gas int64 - GasAdjustment float64 NodeURI string FromAddressName string AccountStore string @@ -58,8 +56,6 @@ func NewCLIContext() CLIContext { AccountStore: ctxAccStoreName, FromAddressName: viper.GetString(client.FlagFrom), Height: viper.GetInt64(client.FlagHeight), - Gas: viper.GetInt64(client.FlagGas), - GasAdjustment: viper.GetFloat64(client.FlagGasAdjustment), TrustNode: viper.GetBool(client.FlagTrustNode), UseLedger: viper.GetBool(client.FlagUseLedger), Async: viper.GetBool(client.FlagAsync), @@ -164,9 +160,3 @@ func (ctx CLIContext) WithCertifier(certifier tmlite.Certifier) CLIContext { ctx.Certifier = certifier return ctx } - -// WithGasAdjustment returns a copy of the context with an updated GasAdjustment flag. -func (ctx CLIContext) WithGasAdjustment(adjustment float64) CLIContext { - ctx.GasAdjustment = adjustment - return ctx -} diff --git a/client/flags.go b/client/flags.go index 97fce42a5885..4b2a72053382 100644 --- a/client/flags.go +++ b/client/flags.go @@ -1,14 +1,20 @@ package client -import "github.com/spf13/cobra" +import ( + "fmt" + "strconv" + + "github.com/spf13/cobra" +) // nolint const ( // DefaultGasAdjustment is applied to gas estimates to avoid tx // execution failures due to state changes that might // occur between the tx simulation and the actual run. - DefaultGasAdjustment = 1.0 - DefaultGasLimit = 200000 + DefaultGasAdjustment = 1.0 + DefaultGasLimit = 200000 + GasFlagSimulateString = "simulate" FlagUseLedger = "ledger" FlagChainID = "chain-id" @@ -32,7 +38,10 @@ const ( // LineBreak can be included in a command list to provide a blank line // to help with readability -var LineBreak = &cobra.Command{Run: func(*cobra.Command, []string) {}} +var ( + LineBreak = &cobra.Command{Run: func(*cobra.Command, []string) {}} + GasFlagVar = GasSetting{Gas: DefaultGasLimit} +) // GetCommands adds common flags to query commands func GetCommands(cmds ...*cobra.Command) []*cobra.Command { @@ -58,7 +67,6 @@ func PostCommands(cmds ...*cobra.Command) []*cobra.Command { c.Flags().String(FlagChainID, "", "Chain ID of tendermint node") c.Flags().String(FlagNode, "tcp://localhost:26657", ": to tendermint rpc interface for this chain") c.Flags().Bool(FlagUseLedger, false, "Use a connected Ledger device") - c.Flags().Int64(FlagGas, DefaultGasLimit, "gas limit to set per-transaction; set to 0 to calculate required gas automatically") c.Flags().Float64(FlagGasAdjustment, DefaultGasAdjustment, "adjustment factor to be multiplied against the estimate returned by the tx simulation; if the gas limit is set manually this flag is ignored ") c.Flags().Bool(FlagAsync, false, "broadcast transactions asynchronously") c.Flags().Bool(FlagJson, false, "return output in json format") @@ -66,6 +74,50 @@ func PostCommands(cmds ...*cobra.Command) []*cobra.Command { c.Flags().Bool(FlagTrustNode, true, "Don't verify proofs for query responses") c.Flags().Bool(FlagDryRun, false, "ignore the --gas flag and perform a simulation of a transaction, but don't broadcast it") c.Flags().Bool(FlagGenerateOnly, false, "build an unsigned transaction and write it to STDOUT") + // --gas can accept integers and "simulate" + c.Flags().Var(&GasFlagVar, "gas", fmt.Sprintf( + "gas limit to set per-transaction; set to %q to calculate required gas automatically (default %d)", GasFlagSimulateString, DefaultGasLimit)) } return cmds } + +// Gas flag parsing functions + +// GasSetting encapsulates the possible values passed through the --gas flag. +type GasSetting struct { + Simulate bool + Gas int64 +} + +// Type returns the flag's value type. +func (v *GasSetting) Type() string { return "string" } + +// Set parses and sets the value of the --gas flag. +func (v *GasSetting) Set(s string) (err error) { + v.Simulate, v.Gas, err = ReadGasFlag(s) + return +} + +func (v *GasSetting) String() string { + if v.Simulate { + return GasFlagSimulateString + } + return strconv.FormatInt(v.Gas, 10) +} + +// ParseGasFlag parses the value of the --gas flag. +func ReadGasFlag(s string) (simulate bool, gas int64, err error) { + switch s { + case "": + gas = DefaultGasLimit + case GasFlagSimulateString: + simulate = true + default: + gas, err = strconv.ParseInt(s, 10, 64) + if err != nil { + err = fmt.Errorf("it must be either integer or %q", GasFlagSimulateString) + return + } + } + return +} diff --git a/client/lcd/lcd_test.go b/client/lcd/lcd_test.go index 977c03cf7227..61c6aad06dec 100644 --- a/client/lcd/lcd_test.go +++ b/client/lcd/lcd_test.go @@ -267,21 +267,25 @@ func TestCoinSend(t *testing.T) { require.Equal(t, int64(1), mycoins.Amount.Int64()) // test failure with too little gas - res, body, _ = doSendWithGas(t, port, seed, name, password, addr, 100, 0, "") + res, body, _ = doSendWithGas(t, port, seed, name, password, addr, "100", 0, "") + require.Equal(t, http.StatusInternalServerError, res.StatusCode, body) + + // test failure with too negative gas + res, body, _ = doSendWithGas(t, port, seed, name, password, addr, "-200", 0, "") require.Equal(t, http.StatusInternalServerError, res.StatusCode, body) // test failure with wrong adjustment - res, body, _ = doSendWithGas(t, port, seed, name, password, addr, 0, 0.1, "") + res, body, _ = doSendWithGas(t, port, seed, name, password, addr, "simulate", 0.1, "") require.Equal(t, http.StatusInternalServerError, res.StatusCode, body) // run simulation and test success with estimated gas - res, body, _ = doSendWithGas(t, port, seed, name, password, addr, 0, 0, "?simulate=true") + res, body, _ = doSendWithGas(t, port, seed, name, password, addr, "", 0, "?simulate=true") require.Equal(t, http.StatusOK, res.StatusCode, body) var responseBody struct { GasEstimate int64 `json:"gas_estimate"` } require.Nil(t, json.Unmarshal([]byte(body), &responseBody)) - res, body, _ = doSendWithGas(t, port, seed, name, password, addr, responseBody.GasEstimate, 0, "") + res, body, _ = doSendWithGas(t, port, seed, name, password, addr, fmt.Sprintf("%v", responseBody.GasEstimate), 0, "") require.Equal(t, http.StatusOK, res.StatusCode, body) } @@ -322,7 +326,7 @@ func TestCoinSendGenerateSignAndBroadcast(t *testing.T) { acc := getAccount(t, port, addr) // generate TX - res, body, _ := doSendWithGas(t, port, seed, name, password, addr, 0, 0, "?generate_only=true") + res, body, _ := doSendWithGas(t, port, seed, name, password, addr, "simulate", 0, "?generate_only=true") require.Equal(t, http.StatusOK, res.StatusCode, body) var msg auth.StdTx require.Nil(t, cdc.UnmarshalJSON([]byte(body), &msg)) @@ -792,7 +796,7 @@ func getAccount(t *testing.T, port string, addr sdk.AccAddress) auth.Account { return acc } -func doSendWithGas(t *testing.T, port, seed, name, password string, addr sdk.AccAddress, gas int64, gasAdjustment float64, queryStr string) (res *http.Response, body string, receiveAddr sdk.AccAddress) { +func doSendWithGas(t *testing.T, port, seed, name, password string, addr sdk.AccAddress, gas string, gasAdjustment float64, queryStr string) (res *http.Response, body string, receiveAddr sdk.AccAddress) { // create receive address kb := client.MockKeyBase() @@ -811,14 +815,14 @@ func doSendWithGas(t *testing.T, port, seed, name, password string, addr sdk.Acc } gasStr := "" - if gas > 0 { + if len(gas) != 0 { gasStr = fmt.Sprintf(` - "gas":"%v", + "gas":%q, `, gas) } gasAdjustmentStr := "" if gasAdjustment > 0 { - gasStr = fmt.Sprintf(` + gasAdjustmentStr = fmt.Sprintf(` "gas_adjustment":"%v", `, gasAdjustment) } @@ -837,7 +841,7 @@ func doSendWithGas(t *testing.T, port, seed, name, password string, addr sdk.Acc } func doSend(t *testing.T, port, seed, name, password string, addr sdk.AccAddress) (receiveAddr sdk.AccAddress, resultTx ctypes.ResultBroadcastTxCommit) { - res, body, receiveAddr := doSendWithGas(t, port, seed, name, password, addr, 0, 0, "") + res, body, receiveAddr := doSendWithGas(t, port, seed, name, password, addr, "", 0, "") require.Equal(t, http.StatusOK, res.StatusCode, body) err := cdc.UnmarshalJSON([]byte(body), &resultTx) diff --git a/client/utils/utils.go b/client/utils/utils.go index 54d9dd584c5f..52472ba1ebba 100644 --- a/client/utils/utils.go +++ b/client/utils/utils.go @@ -24,8 +24,8 @@ func SendTx(txBldr authtxb.TxBuilder, cliCtx context.CLIContext, msgs []sdk.Msg) if err != nil { return err } - autogas := cliCtx.DryRun || (cliCtx.Gas == 0) - if autogas { + + if txBldr.SimulateGas || cliCtx.DryRun { txBldr, err = EnrichCtxWithGas(txBldr, cliCtx, cliCtx.FromAddressName, msgs) if err != nil { return err @@ -50,20 +50,10 @@ func SendTx(txBldr authtxb.TxBuilder, cliCtx context.CLIContext, msgs []sdk.Msg) return cliCtx.EnsureBroadcastTx(txBytes) } -// SimulateMsgs simulates the transaction and returns the gas estimate and the adjusted value. -func SimulateMsgs(txBldr authtxb.TxBuilder, cliCtx context.CLIContext, name string, msgs []sdk.Msg, gas int64) (estimated, adjusted int64, err error) { - txBytes, err := txBldr.WithGas(gas).BuildWithPubKey(name, msgs) - if err != nil { - return - } - estimated, adjusted, err = CalculateGas(cliCtx.Query, cliCtx.Codec, txBytes, cliCtx.GasAdjustment) - return -} - // EnrichCtxWithGas calculates the gas estimate that would be consumed by the // transaction and set the transaction's respective value accordingly. func EnrichCtxWithGas(txBldr authtxb.TxBuilder, cliCtx context.CLIContext, name string, msgs []sdk.Msg) (authtxb.TxBuilder, error) { - _, adjusted, err := SimulateMsgs(txBldr, cliCtx, name, msgs, 0) + _, adjusted, err := simulateMsgs(txBldr, cliCtx, name, msgs) if err != nil { return txBldr, err } @@ -143,6 +133,16 @@ func SignStdTx(txBldr authtxb.TxBuilder, cliCtx context.CLIContext, name string, return txBldr.SignStdTx(name, passphrase, stdTx, appendSig) } +// SimulateMsgs simulates the transaction and returns the gas estimate and the adjusted value. +func simulateMsgs(txBldr authtxb.TxBuilder, cliCtx context.CLIContext, name string, msgs []sdk.Msg) (estimated, adjusted int64, err error) { + txBytes, err := txBldr.BuildWithPubKey(name, msgs) + if err != nil { + return + } + estimated, adjusted, err = CalculateGas(cliCtx.Query, cliCtx.Codec, txBytes, txBldr.GasAdjustment) + return +} + func adjustGasEstimate(estimate int64, adjustment float64) int64 { return int64(adjustment * float64(estimate)) } @@ -194,7 +194,7 @@ func buildUnsignedStdTx(txBldr authtxb.TxBuilder, cliCtx context.CLIContext, msg if err != nil { return } - if txBldr.Gas == 0 { + if txBldr.SimulateGas { txBldr, err = EnrichCtxWithGas(txBldr, cliCtx, cliCtx.FromAddressName, msgs) if err != nil { return diff --git a/cmd/gaia/cli_test/cli_test.go b/cmd/gaia/cli_test/cli_test.go index 032482e7bcd3..c67f73cbc1a8 100644 --- a/cmd/gaia/cli_test/cli_test.go +++ b/cmd/gaia/cli_test/cli_test.go @@ -112,8 +112,12 @@ func TestGaiaCLIGasAuto(t *testing.T) { fooAcc = executeGetAccount(t, fmt.Sprintf("gaiacli account %s %v", fooAddr, flags)) require.Equal(t, int64(50), fooAcc.GetCoins().AmountOf("steak").Int64()) + // Test failure with negative gas + success = executeWrite(t, fmt.Sprintf("gaiacli send %v --gas=-100 --amount=10steak --to=%s --from=foo", flags, barAddr), app.DefaultKeyPass) + require.False(t, success) + // Enable auto gas - success, stdout, _ := executeWriteRetStdStreams(t, fmt.Sprintf("gaiacli send %v --json --gas=0 --amount=10steak --to=%s --from=foo", flags, barAddr), app.DefaultKeyPass) + success, stdout, _ := executeWriteRetStdStreams(t, fmt.Sprintf("gaiacli send %v --json --gas=simulate --amount=10steak --to=%s --from=foo", flags, barAddr), app.DefaultKeyPass) require.True(t, success) // check that gas wanted == gas used cdc := app.MakeCodec() @@ -381,7 +385,7 @@ func TestGaiaCLISendGenerateSignAndBroadcast(t *testing.T) { // Test generate sendTx, estimate gas success, stdout, stderr = executeWriteRetStdStreams(t, fmt.Sprintf( - "gaiacli send %v --amount=10steak --to=%s --from=foo --gas=0 --generate-only", + "gaiacli send %v --amount=10steak --to=%s --from=foo --gas=simulate --generate-only", flags, barAddr), []string{}...) require.True(t, success) require.NotEmpty(t, stderr) diff --git a/docs/light/api.md b/docs/light/api.md index 6c7f9de177de..7fbf9fbe19f5 100644 --- a/docs/light/api.md +++ b/docs/light/api.md @@ -763,7 +763,7 @@ The GovernanceAPI exposes all functionality needed for casting votes on plain te "chain_id": "string", "account_number": 0, "sequence": 0, - "gas": 0 + "gas": "simulate" }, "depositer": "string", "amount": 0, @@ -866,7 +866,7 @@ The GovernanceAPI exposes all functionality needed for casting votes on plain te "chain_id": "string", "account_number": 0, "sequence": 0, - "gas": 0 + "gas": "simulate" }, // A cosmos address "voter": "string", diff --git a/docs/sdk/clients.md b/docs/sdk/clients.md index 4d02d3c90850..5b7d3ca8669c 100644 --- a/docs/sdk/clients.md +++ b/docs/sdk/clients.md @@ -111,7 +111,7 @@ The `--amount` flag accepts the format `--amount=`. ::: tip Note You may want to cap the maximum gas that can be consumed by the transaction via the `--gas` flag. -If set to 0, the gas limit will be automatically estimated. +If you pass `--gas=simulate`, the gas limit will be automatically estimated. Gas estimate might be inaccurate as state changes could occur in between the end of the simulation and the actual execution of a transaction, thus an adjustment is applied on top of the original estimate in order to ensure the transaction is broadcasted successfully. The adjustment can be controlled via the `--gas-adjustment` flag, whose default value is 1.0. ::: diff --git a/x/auth/client/txbuilder/txbuilder.go b/x/auth/client/txbuilder/txbuilder.go index 026eabeceffc..da5b769a3dbf 100644 --- a/x/auth/client/txbuilder/txbuilder.go +++ b/x/auth/client/txbuilder/txbuilder.go @@ -17,6 +17,8 @@ type TxBuilder struct { AccountNumber int64 Sequence int64 Gas int64 + GasAdjustment float64 + SimulateGas bool ChainID string Memo string Fee string @@ -36,9 +38,11 @@ func NewTxBuilderFromCLI() TxBuilder { return TxBuilder{ ChainID: chainID, - Gas: viper.GetInt64(client.FlagGas), AccountNumber: viper.GetInt64(client.FlagAccountNumber), + Gas: client.GasFlagVar.Gas, + GasAdjustment: viper.GetFloat64(client.FlagGasAdjustment), Sequence: viper.GetInt64(client.FlagSequence), + SimulateGas: client.GasFlagVar.Simulate, Fee: viper.GetString(client.FlagFee), Memo: viper.GetString(client.FlagMemo), } diff --git a/x/bank/client/rest/sendtx.go b/x/bank/client/rest/sendtx.go index 82df24642f36..02a66f2b9095 100644 --- a/x/bank/client/rest/sendtx.go +++ b/x/bank/client/rest/sendtx.go @@ -32,7 +32,7 @@ type sendBody struct { ChainID string `json:"chain_id"` AccountNumber int64 `json:"account_number"` Sequence int64 `json:"sequence"` - Gas int64 `json:"gas"` + Gas string `json:"gas"` GasAdjustment string `json:"gas_adjustment"` } @@ -81,31 +81,37 @@ func SendRequestHandlerFn(cdc *wire.Codec, kb keys.Keybase, cliCtx context.CLICo return } - txBldr := authtxb.TxBuilder{ - Codec: cdc, - Gas: m.Gas, - ChainID: m.ChainID, - AccountNumber: m.AccountNumber, - Sequence: m.Sequence, + simulateGas, gas, err := cliclient.ReadGasFlag(m.Gas) + if err != nil { + utils.WriteErrorResponse(w, http.StatusBadRequest, err.Error()) + return } adjustment, ok := utils.ParseFloat64OrReturnBadRequest(w, m.GasAdjustment, cliclient.DefaultGasAdjustment) if !ok { return } - cliCtx = cliCtx.WithGasAdjustment(adjustment) + txBldr := authtxb.TxBuilder{ + Codec: cdc, + Gas: gas, + GasAdjustment: adjustment, + SimulateGas: simulateGas, + ChainID: m.ChainID, + AccountNumber: m.AccountNumber, + Sequence: m.Sequence, + } - if utils.HasDryRunArg(r) || m.Gas == 0 { - newCtx, err := utils.EnrichCtxWithGas(txBldr, cliCtx, m.LocalAccountName, []sdk.Msg{msg}) + if utils.HasDryRunArg(r) || txBldr.SimulateGas { + newBldr, err := utils.EnrichCtxWithGas(txBldr, cliCtx, m.LocalAccountName, []sdk.Msg{msg}) if err != nil { utils.WriteErrorResponse(w, http.StatusInternalServerError, err.Error()) return } if utils.HasDryRunArg(r) { - utils.WriteSimulationResponse(w, txBldr.Gas) + utils.WriteSimulationResponse(w, newBldr.Gas) return } - txBldr = newCtx + txBldr = newBldr } if utils.HasGenerateOnlyArg(r) { diff --git a/x/gov/client/rest/util.go b/x/gov/client/rest/util.go index f9987c03088d..b152cb20b285 100644 --- a/x/gov/client/rest/util.go +++ b/x/gov/client/rest/util.go @@ -20,7 +20,7 @@ type baseReq struct { ChainID string `json:"chain_id"` AccountNumber int64 `json:"account_number"` Sequence int64 `json:"sequence"` - Gas int64 `json:"gas"` + Gas string `json:"gas"` GasAdjustment string `json:"gas_adjustment"` } @@ -69,32 +69,37 @@ func (req baseReq) baseReqValidate(w http.ResponseWriter) bool { // TODO: Build this function out into a more generic base-request // (probably should live in client/lcd). func signAndBuild(w http.ResponseWriter, r *http.Request, cliCtx context.CLIContext, baseReq baseReq, msg sdk.Msg, cdc *wire.Codec) { - var err error - txBldr := authtxb.TxBuilder{ - Codec: cdc, - AccountNumber: baseReq.AccountNumber, - Sequence: baseReq.Sequence, - ChainID: baseReq.ChainID, - Gas: baseReq.Gas, + simulateGas, gas, err := client.ReadGasFlag(baseReq.Gas) + if err != nil { + utils.WriteErrorResponse(w, http.StatusBadRequest, err.Error()) + return } adjustment, ok := utils.ParseFloat64OrReturnBadRequest(w, baseReq.GasAdjustment, client.DefaultGasAdjustment) if !ok { return } - cliCtx = cliCtx.WithGasAdjustment(adjustment) + txBldr := authtxb.TxBuilder{ + Codec: cdc, + Gas: gas, + GasAdjustment: adjustment, + SimulateGas: simulateGas, + ChainID: baseReq.ChainID, + AccountNumber: baseReq.AccountNumber, + Sequence: baseReq.Sequence, + } - if utils.HasDryRunArg(r) || baseReq.Gas == 0 { - newCtx, err := utils.EnrichCtxWithGas(txBldr, cliCtx, baseReq.Name, []sdk.Msg{msg}) + if utils.HasDryRunArg(r) || txBldr.SimulateGas { + newBldr, err := utils.EnrichCtxWithGas(txBldr, cliCtx, baseReq.Name, []sdk.Msg{msg}) if err != nil { utils.WriteErrorResponse(w, http.StatusInternalServerError, err.Error()) return } if utils.HasDryRunArg(r) { - utils.WriteSimulationResponse(w, txBldr.Gas) + utils.WriteSimulationResponse(w, newBldr.Gas) return } - txBldr = newCtx + txBldr = newBldr } if utils.HasGenerateOnlyArg(r) { diff --git a/x/ibc/client/rest/transfer.go b/x/ibc/client/rest/transfer.go index 110efe601ecb..5a7b632e833c 100644 --- a/x/ibc/client/rest/transfer.go +++ b/x/ibc/client/rest/transfer.go @@ -29,7 +29,7 @@ type transferBody struct { SrcChainID string `json:"src_chain_id"` AccountNumber int64 `json:"account_number"` Sequence int64 `json:"sequence"` - Gas int64 `json:"gas"` + Gas string `json:"gas"` GasAdjustment string `json:"gas_adjustment"` } @@ -71,21 +71,26 @@ func TransferRequestHandlerFn(cdc *wire.Codec, kb keys.Keybase, cliCtx context.C packet := ibc.NewIBCPacket(sdk.AccAddress(info.GetPubKey().Address()), to, m.Amount, m.SrcChainID, destChainID) msg := ibc.IBCTransferMsg{packet} + simulateGas, gas, err := client.ReadGasFlag(m.Gas) + if err != nil { + utils.WriteErrorResponse(w, http.StatusBadRequest, err.Error()) + return + } + adjustment, ok := utils.ParseFloat64OrReturnBadRequest(w, m.GasAdjustment, client.DefaultGasAdjustment) + if !ok { + return + } txBldr := authtxb.TxBuilder{ Codec: cdc, + Gas: gas, + GasAdjustment: adjustment, + SimulateGas: simulateGas, ChainID: m.SrcChainID, AccountNumber: m.AccountNumber, Sequence: m.Sequence, - Gas: m.Gas, - } - - adjustment, ok := utils.ParseFloat64OrReturnBadRequest(w, m.GasAdjustment, client.DefaultGasAdjustment) - if !ok { - return } - cliCtx = cliCtx.WithGasAdjustment(adjustment) - if utils.HasDryRunArg(r) || m.Gas == 0 { + if utils.HasDryRunArg(r) || txBldr.SimulateGas { newCtx, err := utils.EnrichCtxWithGas(txBldr, cliCtx, m.LocalAccountName, []sdk.Msg{msg}) if err != nil { utils.WriteErrorResponse(w, http.StatusInternalServerError, err.Error()) diff --git a/x/slashing/client/rest/tx.go b/x/slashing/client/rest/tx.go index 1d4fdefa1d2c..969cdcce092a 100644 --- a/x/slashing/client/rest/tx.go +++ b/x/slashing/client/rest/tx.go @@ -70,22 +70,20 @@ func unjailRequestHandlerFn(cdc *wire.Codec, kb keys.Keybase, cliCtx context.CLI return } + adjustment, ok := utils.ParseFloat64OrReturnBadRequest(w, m.GasAdjustment, client.DefaultGasAdjustment) + if !ok { + return + } txBldr := authtxb.TxBuilder{ Codec: cdc, ChainID: m.ChainID, AccountNumber: m.AccountNumber, Sequence: m.Sequence, Gas: m.Gas, + GasAdjustment: adjustment, } msg := slashing.NewMsgUnjail(valAddr) - - adjustment, ok := utils.ParseFloat64OrReturnBadRequest(w, m.GasAdjustment, client.DefaultGasAdjustment) - if !ok { - return - } - cliCtx = cliCtx.WithGasAdjustment(adjustment) - if utils.HasDryRunArg(r) || m.Gas == 0 { newCtx, err := utils.EnrichCtxWithGas(txBldr, cliCtx, m.LocalAccountName, []sdk.Msg{msg}) if err != nil { diff --git a/x/stake/client/rest/tx.go b/x/stake/client/rest/tx.go index d84a8daea977..fda92f85aadb 100644 --- a/x/stake/client/rest/tx.go +++ b/x/stake/client/rest/tx.go @@ -60,7 +60,7 @@ type EditDelegationsBody struct { ChainID string `json:"chain_id"` AccountNumber int64 `json:"account_number"` Sequence int64 `json:"sequence"` - Gas int64 `json:"gas"` + Gas string `json:"gas"` GasAdjustment string `json:"gas_adjustment"` Delegations []msgDelegationsInput `json:"delegations"` BeginUnbondings []msgBeginUnbondingInput `json:"begin_unbondings"` @@ -263,10 +263,21 @@ func delegationsRequestHandlerFn(cdc *wire.Codec, kb keys.Keybase, cliCtx contex i++ } + simulateGas, gas, err := client.ReadGasFlag(m.Gas) + if err != nil { + utils.WriteErrorResponse(w, http.StatusBadRequest, err.Error()) + return + } + adjustment, ok := utils.ParseFloat64OrReturnBadRequest(w, m.GasAdjustment, client.DefaultGasAdjustment) + if !ok { + return + } txBldr := authtxb.TxBuilder{ - Codec: cdc, - ChainID: m.ChainID, - Gas: m.Gas, + Codec: cdc, + Gas: gas, + GasAdjustment: adjustment, + SimulateGas: simulateGas, + ChainID: m.ChainID, } // sign messages @@ -275,26 +286,19 @@ func delegationsRequestHandlerFn(cdc *wire.Codec, kb keys.Keybase, cliCtx contex // increment sequence for each message txBldr = txBldr.WithAccountNumber(m.AccountNumber) txBldr = txBldr.WithSequence(m.Sequence) - m.Sequence++ - adjustment, ok := utils.ParseFloat64OrReturnBadRequest(w, m.GasAdjustment, client.DefaultGasAdjustment) - if !ok { - return - } - cliCtx = cliCtx.WithGasAdjustment(adjustment) - - if utils.HasDryRunArg(r) || m.Gas == 0 { - newCtx, err := utils.EnrichCtxWithGas(txBldr, cliCtx, m.LocalAccountName, []sdk.Msg{msg}) + if utils.HasDryRunArg(r) || txBldr.SimulateGas { + newBldr, err := utils.EnrichCtxWithGas(txBldr, cliCtx, m.LocalAccountName, []sdk.Msg{msg}) if err != nil { utils.WriteErrorResponse(w, http.StatusInternalServerError, err.Error()) return } if utils.HasDryRunArg(r) { - utils.WriteSimulationResponse(w, txBldr.Gas) + utils.WriteSimulationResponse(w, newBldr.Gas) return } - txBldr = newCtx + txBldr = newBldr } if utils.HasGenerateOnlyArg(r) { From 40cd51e17a0f13b351a2de09f6eeddace4048205 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Tue, 11 Sep 2018 15:26:15 +0100 Subject: [PATCH 2/6] Add test cases with gas=0 --- client/lcd/lcd_test.go | 6 +++++- cmd/gaia/cli_test/cli_test.go | 4 ++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/client/lcd/lcd_test.go b/client/lcd/lcd_test.go index 61c6aad06dec..b3c27bfbab7a 100644 --- a/client/lcd/lcd_test.go +++ b/client/lcd/lcd_test.go @@ -270,10 +270,14 @@ func TestCoinSend(t *testing.T) { res, body, _ = doSendWithGas(t, port, seed, name, password, addr, "100", 0, "") require.Equal(t, http.StatusInternalServerError, res.StatusCode, body) - // test failure with too negative gas + // test failure with negative gas res, body, _ = doSendWithGas(t, port, seed, name, password, addr, "-200", 0, "") require.Equal(t, http.StatusInternalServerError, res.StatusCode, body) + // test failure with 0 gas + res, body, _ = doSendWithGas(t, port, seed, name, password, addr, "0", 0, "") + require.Equal(t, http.StatusInternalServerError, res.StatusCode, body) + // test failure with wrong adjustment res, body, _ = doSendWithGas(t, port, seed, name, password, addr, "simulate", 0.1, "") require.Equal(t, http.StatusInternalServerError, res.StatusCode, body) diff --git a/cmd/gaia/cli_test/cli_test.go b/cmd/gaia/cli_test/cli_test.go index c67f73cbc1a8..43f35925e31b 100644 --- a/cmd/gaia/cli_test/cli_test.go +++ b/cmd/gaia/cli_test/cli_test.go @@ -116,6 +116,10 @@ func TestGaiaCLIGasAuto(t *testing.T) { success = executeWrite(t, fmt.Sprintf("gaiacli send %v --gas=-100 --amount=10steak --to=%s --from=foo", flags, barAddr), app.DefaultKeyPass) require.False(t, success) + // Test failure with 0 gas + success = executeWrite(t, fmt.Sprintf("gaiacli send %v --gas=0 --amount=10steak --to=%s --from=foo", flags, barAddr), app.DefaultKeyPass) + require.False(t, success) + // Enable auto gas success, stdout, _ := executeWriteRetStdStreams(t, fmt.Sprintf("gaiacli send %v --json --gas=simulate --amount=10steak --to=%s --from=foo", flags, barAddr), app.DefaultKeyPass) require.True(t, success) From 4222b9d037868c8d7cc03d9e2aee58c3bb51bf11 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Tue, 11 Sep 2018 17:15:48 +0100 Subject: [PATCH 3/6] ACK suggestion from @alexanderbez --- client/flags.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/flags.go b/client/flags.go index 4b2a72053382..eb7e276f0ab8 100644 --- a/client/flags.go +++ b/client/flags.go @@ -115,7 +115,7 @@ func ReadGasFlag(s string) (simulate bool, gas int64, err error) { default: gas, err = strconv.ParseInt(s, 10, 64) if err != nil { - err = fmt.Errorf("it must be either integer or %q", GasFlagSimulateString) + err = fmt.Errorf("gas must be either integer or %q", GasFlagSimulateString) return } } From 7f15866e2f6756f4e2385f9b962cb483b540a96d Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Tue, 11 Sep 2018 17:17:17 +0100 Subject: [PATCH 4/6] s/GasFlagSimulateString/GasFlagSimulate/ --- client/flags.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/client/flags.go b/client/flags.go index eb7e276f0ab8..c98898029114 100644 --- a/client/flags.go +++ b/client/flags.go @@ -12,9 +12,9 @@ const ( // DefaultGasAdjustment is applied to gas estimates to avoid tx // execution failures due to state changes that might // occur between the tx simulation and the actual run. - DefaultGasAdjustment = 1.0 - DefaultGasLimit = 200000 - GasFlagSimulateString = "simulate" + DefaultGasAdjustment = 1.0 + DefaultGasLimit = 200000 + GasFlagSimulate = "simulate" FlagUseLedger = "ledger" FlagChainID = "chain-id" @@ -76,7 +76,7 @@ func PostCommands(cmds ...*cobra.Command) []*cobra.Command { c.Flags().Bool(FlagGenerateOnly, false, "build an unsigned transaction and write it to STDOUT") // --gas can accept integers and "simulate" c.Flags().Var(&GasFlagVar, "gas", fmt.Sprintf( - "gas limit to set per-transaction; set to %q to calculate required gas automatically (default %d)", GasFlagSimulateString, DefaultGasLimit)) + "gas limit to set per-transaction; set to %q to calculate required gas automatically (default %d)", GasFlagSimulate, DefaultGasLimit)) } return cmds } @@ -100,7 +100,7 @@ func (v *GasSetting) Set(s string) (err error) { func (v *GasSetting) String() string { if v.Simulate { - return GasFlagSimulateString + return GasFlagSimulate } return strconv.FormatInt(v.Gas, 10) } @@ -110,12 +110,12 @@ func ReadGasFlag(s string) (simulate bool, gas int64, err error) { switch s { case "": gas = DefaultGasLimit - case GasFlagSimulateString: + case GasFlagSimulate: simulate = true default: gas, err = strconv.ParseInt(s, 10, 64) if err != nil { - err = fmt.Errorf("gas must be either integer or %q", GasFlagSimulateString) + err = fmt.Errorf("gas must be either integer or %q", GasFlagSimulate) return } } From 461261a37ea58743f800c77b40addf6abe1f64fd Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Tue, 11 Sep 2018 18:30:48 +0100 Subject: [PATCH 5/6] Drop TODO comment on Gas type --- x/auth/client/txbuilder/txbuilder.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/auth/client/txbuilder/txbuilder.go b/x/auth/client/txbuilder/txbuilder.go index da5b769a3dbf..c3a4c30e0623 100644 --- a/x/auth/client/txbuilder/txbuilder.go +++ b/x/auth/client/txbuilder/txbuilder.go @@ -16,7 +16,7 @@ type TxBuilder struct { Codec *wire.Codec AccountNumber int64 Sequence int64 - Gas int64 + Gas int64 // TODO: should this turn into uint64? requires further discussion GasAdjustment float64 SimulateGas bool ChainID string From cb0526177adf5af14d6ca39ccc93b0a0cd899b7c Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Tue, 11 Sep 2018 19:33:16 +0100 Subject: [PATCH 6/6] Enrich TODO with ref --- x/auth/client/txbuilder/txbuilder.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/auth/client/txbuilder/txbuilder.go b/x/auth/client/txbuilder/txbuilder.go index c3a4c30e0623..6daa75e1205c 100644 --- a/x/auth/client/txbuilder/txbuilder.go +++ b/x/auth/client/txbuilder/txbuilder.go @@ -16,7 +16,7 @@ type TxBuilder struct { Codec *wire.Codec AccountNumber int64 Sequence int64 - Gas int64 // TODO: should this turn into uint64? requires further discussion + Gas int64 // TODO: should this turn into uint64? requires further discussion - see #2173 GasAdjustment float64 SimulateGas bool ChainID string