-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: add gas-related parameters to client config #12723
Changes from 10 commits
befb240
c08b594
53accfb
6eaa783
5c245bd
5566bb3
182587d
5f59f6e
7c099f8
9092757
eca07a7
75faaf5
7493f83
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,6 @@ package flags | |
|
||
import ( | ||
"fmt" | ||
"strconv" | ||
|
||
"github.com/spf13/cobra" | ||
tmcli "github.com/tendermint/tendermint/libs/cli" | ||
|
@@ -112,7 +111,7 @@ func AddTxFlagsToCmd(cmd *cobra.Command) { | |
cmd.Flags().String(FlagGasPrices, "", "Gas prices in decimal format to determine the transaction fee (e.g. 0.1uatom)") | ||
cmd.Flags().String(FlagNode, "tcp://localhost:26657", "<host>:<port> to tendermint rpc interface for this chain") | ||
cmd.Flags().Bool(FlagUseLedger, false, "Use a connected Ledger device") | ||
cmd.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 ") | ||
cmd.Flags().Float64(FlagGasAdjustment, 0, "adjustment factor to be multiplied against the estimate returned by the tx simulation; if the gas limit is set manually this flag is ignored ") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should still be 1.0 IMO There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the client will now use the value in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in other words, if i have it as 1.0 here, the CLI help text will say There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, so again, |
||
cmd.Flags().StringP(FlagBroadcastMode, "b", BroadcastSync, "Transaction broadcasting mode (sync|async|block)") | ||
cmd.Flags().Bool(FlagDryRun, false, "ignore the --gas flag and perform a simulation of a transaction, but don't broadcast it (when enabled, the local Keybase is not accessible)") | ||
cmd.Flags().Bool(FlagGenerateOnly, false, "Build an unsigned transaction and write it to STDOUT (when enabled, the local Keybase only accessed when providing a key name)") | ||
|
@@ -127,7 +126,7 @@ func AddTxFlagsToCmd(cmd *cobra.Command) { | |
cmd.Flags().Bool(FlagAux, false, "Generate aux signer data instead of sending a tx") | ||
|
||
// --gas can accept integers and "auto" | ||
cmd.Flags().String(FlagGas, "", fmt.Sprintf("gas limit to set per-transaction; set to %q to calculate sufficient gas automatically (default %d)", GasFlagAuto, DefaultGasLimit)) | ||
cmd.Flags().String(FlagGas, "", fmt.Sprintf("gas limit to set per-transaction; set to %q to calculate sufficient gas automatically", GasFlagAuto)) | ||
} | ||
|
||
// AddPaginationFlagsToCmd adds common pagination flags to cmd | ||
|
@@ -139,39 +138,3 @@ func AddPaginationFlagsToCmd(cmd *cobra.Command, query string) { | |
cmd.Flags().Bool(FlagCountTotal, false, fmt.Sprintf("count total number of records in %s to query for", query)) | ||
cmd.Flags().Bool(FlagReverse, false, "results are sorted in descending order") | ||
} | ||
|
||
// GasSetting encapsulates the possible values passed through the --gas flag. | ||
type GasSetting struct { | ||
Simulate bool | ||
Gas uint64 | ||
} | ||
|
||
func (v *GasSetting) String() string { | ||
if v.Simulate { | ||
return GasFlagAuto | ||
} | ||
|
||
return strconv.FormatUint(v.Gas, 10) | ||
} | ||
|
||
// ParseGasSetting parses a string gas value. The value may either be 'auto', | ||
// which indicates a transaction should be executed in simulate mode to | ||
// automatically find a sufficient gas value, or a string integer. It returns an | ||
// error if a string integer is provided which cannot be parsed. | ||
func ParseGasSetting(gasStr string) (GasSetting, error) { | ||
switch gasStr { | ||
case "": | ||
return GasSetting{false, DefaultGasLimit}, nil | ||
|
||
case GasFlagAuto: | ||
return GasSetting{true, 0}, nil | ||
|
||
default: | ||
gas, err := strconv.ParseUint(gasStr, 10, 64) | ||
if err != nil { | ||
return GasSetting{}, fmt.Errorf("gas must be either integer or %s", GasFlagAuto) | ||
} | ||
|
||
return GasSetting{false, gas}, nil | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
package client | ||
|
||
import ( | ||
"fmt" | ||
"strconv" | ||
|
||
"github.com/cosmos/cosmos-sdk/client/flags" | ||
) | ||
|
||
// GasSetting encapsulates the possible values passed through the --gas flag. | ||
type GasSetting struct { | ||
Simulate bool | ||
Gas uint64 | ||
} | ||
|
||
func (v *GasSetting) String() string { | ||
if v.Simulate { | ||
return flags.GasFlagAuto | ||
} | ||
|
||
return strconv.FormatUint(v.Gas, 10) | ||
} | ||
|
||
// DefaultGasSetting returns the default gas setting | ||
func DefaultGasSetting() GasSetting { | ||
return GasSetting{false, flags.DefaultGasLimit} | ||
} | ||
|
||
// ParseGasSetting parses a string gas value. The value may either be 'auto', | ||
// which indicates a transaction should be executed in simulate mode to | ||
// automatically find a sufficient gas value, or a string integer. It returns an | ||
// error if a string integer is provided which cannot be parsed. | ||
func ParseGasSetting(gasStr string) (GasSetting, error) { | ||
switch gasStr { | ||
case "": | ||
return DefaultGasSetting(), nil | ||
|
||
case flags.GasFlagAuto: | ||
return GasSetting{true, 0}, nil | ||
|
||
default: | ||
gas, err := strconv.ParseUint(gasStr, 10, 64) | ||
if err != nil { | ||
return GasSetting{}, fmt.Errorf("gas must be either integer or %s", flags.GasFlagAuto) | ||
} | ||
|
||
return GasSetting{false, gas}, nil | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, why do you consider this CLI breaking and not a feature or improvement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because users need to add these three lines to their
.simapp/config/client.toml
:either by running
simd init
again or by manually editing the file, of thetx
command won't work.this is because when creating the
ClientCtx
the client will attempt to read these three params fromclient.toml
, and if not found it will throw an error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add exactly that in the upgrading.md ? Under a Configuration section or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done: 7c099f8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noooo, I don't think this is wise. We should not be requiring any fields in
client.toml
whatsoever. I'm strongly opposed to this.client.toml
should be completely opt-in and read only when it exists.AFAIU, the way
client.toml
works is that if a supported field exists in that file, it takes precedence over a default value and a flag value. But in no circumstance should a field ever be required to exist in that file.In general the order of precedence is:
env var > file > flag > default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is in fact already the case for other fields currently in
client.toml
. the chain id for example, if you neither have it configured inclient.toml
nor specify it by the CLI flag, then the client won't be able to sign txs:cosmos-sdk/client/tx/factory.go
Lines 251 to 253 in dc95e33
the gas parameters aren't different from this. you need to either configure them in
client.toml
, or specify with CLI flags.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexanderbez ok here's how i think we can make this work. currently,
clientCtx
is created by parsingclient.toml
:take
gas-adjustment
as an example: assume it is NOT present inclient.toml
(e.g. ifclient.toml
was generated by an old version of SimApp). in this caseconf.GasAdjustment
will be an empty string""
, which causesstrconv.ParseFloat
to fail.this is the reason that the three gas parameters are mandatory in
client.toml
.however we can make them optional simply by, instead of returning
err
here, we use some default values. this way, users won't need to update theirclient.toml
files. sounds good?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me research how other config params are handled. i'll keep this PR as draft for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in general this is the idea. I would do whatever we do for all the values we support in
client.toml
. I haven't looked at how that file is parsed and handled, but I imagine it only parses when non-empty.