feat!: add gas-related parameters to client config #12723

larry0x · 2022-07-26T05:56:08Z

Description

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

fedekunze · 2022-07-26T06:38:28Z

nice! I think adding the log level and log format would be great too

alexanderbez · 2022-07-26T23:41:11Z

See my comment in the main issue. I think we should be extra careful about this. If we're gonna go with this approach, we should at least figure out a means to print a warning/diagnostic message saying, "Reading X value from confg.toml" or something similar.

larry0x · 2022-07-27T00:08:22Z

nice! I think adding the log level and log format would be great too

it turns out that log level/format are parameters for Tendermint (the "server"). the params in client.toml are only for the client, so it doesn't make sense to put log level/format there

larry0x · 2022-07-27T00:42:38Z

See my comment in the main issue. I think we should be extra careful about this. If we're gonna go with this approach, we should at least figure out a means to print a warning/diagnostic message saying, "Reading X value from confg.toml" or something similar.

i see that currently the client prints gas estimation result to stderr (https://github.com/cosmos/cosmos-sdk/blob/v0.46.0/client/tx/tx.go#L78)

is it ok with you, if i similarly print a warning message to stderr if any of the gas flag is not specified? i.e. like this:

// client/tx/factory.go/NewFactoryCli
gasAdj, err := flagSet.GetFloat64(flags.FlagGasAdjustment)
if err != nil {
	gasAdj = clientCtx.GasAdjustment
	// print out warning 👇👇👇
	_, _ = fmt.Fprintf(os.Stderr, "gas adjustment is not specified! using default value from client.toml: %f\n", gasAdj)
}

var gasSetting client.GasSetting
gasStr, err := flagSet.GetString(flags.FlagGas)
if err != nil {
	gasSetting = clientCtx.GasSetting
	// print out warning 👇👇👇
	_, _ = fmt.Fprintf(os.Stderr, "gas setting is not specified! using default value from client.toml: %f\n", gasSetting)
} else {
	gasSetting, _ = client.ParseGasSetting(gasStr)
}

var gasPrices sdk.DecCoins
gasPricesStr, _ := flagSet.GetString(flags.FlagGasPrices)
if err != nil {
	gasPrices = clientCtx.GasPrices
	// print out warning 👇👇👇
	_, _ = fmt.Fprintf(os.Stderr, "gas prices is not specified! using default value from client.toml: %f\n", gasPrices)
} else {
	gasPrices, _ = sdk.ParseDecCoins(gasPricesStr)
}

larry0x · 2022-07-27T00:45:41Z

on a separate topic: this PR breaks a test related to rosetta: https://github.com/cosmos/cosmos-sdk/runs/7514388710

i have very limited knowledge on what rosetta is and how it works. the test is also dockerized making it harder to debug. can i get some help in fixing this?

alexanderbez · 2022-07-27T01:23:06Z

@larry0x yes, that sounds reasonable to me!

Re; rosetta failure, it seems like it can't read the client.toml -- perhaps it's a bug in the implementation and nothing to do with rosetta?

larry0x · 2022-07-28T12:21:55Z

@larry0x yes, that sounds reasonable to me!

Re; rosetta failure, it seems like it can't read the client.toml -- perhaps it's a bug in the implementation and nothing to do with rosetta?

turns out the reason is that the test data needs to be updated (contents in this tarball: https://github.com/cosmos/cosmos-sdk/blob/main/contrib/rosetta/rosetta-ci/data.tar.gz)

alexanderbez · 2022-07-28T13:08:19Z

make rosetta-data, yup!

larry0x · 2022-07-28T23:57:34Z

make rosetta-data, yup!

actually, make rosetta-data doesn't work: #12772

here's what i know:

my branch is forked from main at the commit 5decd22 which was 3 days ago. with this base, the make command works
once i merge the latest main (2bec9d2) into my branch, the command breaks
therefore it must have broken by one of the commits between these 3 days, but i'm unsure which one

alexanderbez · 2022-07-29T01:46:56Z

CHANGELOG.md

@@ -93,7 +93,9 @@ Ref: https://keepachangelog.com/en/1.0.0/

 ### CLI Breaking Changes

-* /
+* (client) [#12723](https://github.com/cosmos/cosmos-sdk/pull/12723) The client config now includes default values for several gas-related parameters, which previously needed to be provided by `--gas`, `--gas-adjustment`, and `--gas-prices` flags.


Curious, why do you consider this CLI breaking and not a feature or improvement?

because users need to add these three lines to their .simapp/config/client.toml:

gas = "200000" gas-adjustment = "1" gas-prices = ""

either by running simd init again or by manually editing the file, of the tx command won't work.

this is because when creating the ClientCtx the client will attempt to read these three params from client.toml, and if not found it will throw an error.

Can you add exactly that in the upgrading.md ? Under a Configuration section or something?

Can you add exactly that in the upgrading.md ? Under a Configuration section or something?

done: 7c099f8

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

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.

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 in client.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

} else if f.chainID == "" {

return nil, fmt.Errorf("chain ID required but not specified")

}

the gas parameters aren't different from this. you need to either configure them in client.toml, or specify with CLI flags.

@alexanderbez ok here's how i think we can make this work. currently, clientCtx is created by parsing client.toml:

// cosmos-sdk/client/config/config.go func ReadFromClientConfig(ctx client.Context) (client.Context, err) { // parse gas setting gasSetting, err := client.ParseGasSetting(conf.Gas) if err != nil { return ctx, fmt.Errorf("couldn't get gas setting from client config: %v", err) } ctx = ctx.WithGasSetting(gasSetting) // parse gas adjustment gasAdj, err := strconv.ParseFloat(conf.GasAdjustment, 64) if err != nil { gasAdj, _ = strconv.ParseFloat(gasAdjustment, 64) } ctx = ctx.WithGasAdjustment(gasAdj) // parse gas prices gasPrices, err := sdk.ParseDecCoins(conf.GasPrices) if err != nil { return ctx, fmt.Errorf("couldn't get gas prices from client config: %v", err) } ctx = ctx.WithGasPrices(gasPrices) // parse other client.toml fields, e.g. chain id, rpc node, sign mode, etc. // ... return ctx }

take gas-adjustment as an example: assume it is NOT present in client.toml (e.g. if client.toml was generated by an old version of SimApp). in this case conf.GasAdjustment will be an empty string "", which causes strconv.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 their client.toml files. sounds good?

In general the order of precedence is:

env var > file > flag > default

let me research how other config params are handled. i'll keep this PR as draft for now.

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 their client.toml files. sounds good?

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.

alexanderbez · 2022-07-29T01:49:36Z

client/flags/flags.go

@@ -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 ")


This should still be 1.0 IMO

the client will now use the value in client.toml as the default, instead of what's defined here. setting this to 0 means there is no default for the flag.

in other words, if i have it as 1.0 here, the CLI help text will say (default: 1), while in fact the default value is taken from client.toml and may not be 1

Yes, so again, client.toml should not be defining any defaults. The file should be completely opt-in. So "" is treated as an OK value which means it would read the default value from the CLI flags.

client/tx/factory.go

julienrbrt

utACK

codecov · 2022-07-29T20:46:48Z

Codecov Report

Merging #12723 (7493f83) into main (dc95e33) will increase coverage by 0.02%.
The diff coverage is 18.64%.

@@            Coverage Diff             @@
##             main   #12723      +/-   ##
==========================================
+ Coverage   56.29%   56.31%   +0.02%     
==========================================
  Files         639      643       +4     
  Lines       54457    54582     +125     
==========================================
+ Hits        30657    30740      +83     
- Misses      21356    21386      +30     
- Partials     2444     2456      +12

Impacted Files	Coverage Δ
client/config/cmd.go	`37.50% <0.00%> (-6.62%)`	⬇️
client/config/toml.go	`55.55% <ø> (ø)`
client/context.go	`53.64% <0.00%> (-2.64%)`	⬇️
client/tx/factory.go	`23.27% <0.00%> (-2.33%)`	⬇️
client/config/config.go	`50.64% <40.00%> (-5.12%)`	⬇️
client/gas.go	`70.58% <70.58%> (ø)`
crypto/keys/internal/ecdsa/privkey.go	`81.13% <0.00%> (-1.89%)`	⬇️
client/flags/flags.go
tx/textual/valuerenderer/bytes.go	`23.07% <0.00%> (ø)`
tx/textual/valuerenderer/int.go	`65.21% <0.00%> (ø)`
... and 5 more

…cosmos-sdk into larry/gas-client-config

tac0turtle · 2022-08-29T19:22:11Z

@larry0x is there anything you need from us to complete this?

larry0x · 2022-08-29T19:25:32Z

@larry0x is there anything you need from us to complete this?

i need to think about what @alexanderbez suggested previously (about the precedence between env var, config file, and flag) and make adjustments. sorry, have been too busy recently to work on this.

github-actions · 2022-10-14T00:09:53Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

gjermundgaraba · 2022-10-14T19:17:10Z

Reviving this.

I talked with @larry0x and I will take a stab at continuing this work.

I am currently looking into the precedence between env, config and flag to make sure it works as intended.

Not sure how to practically continue this particular PR though, if anyone has any ideas (I can make a PR into larry's branch, he can merge it (if he thinks it is good enough :)) which would keep history and everything clean here. Let me know what you want)

alexanderbez · 2022-10-14T19:53:34Z

@bjaanes it might be best to start this PR from scratch honestly and it should be pretty straight forward. We just need to add these fields to the client configuration and read them (this structure already exists). Then it's simply a matter of setting precedence, which we already already do I believe (e.g. chain-id, which you can use as an example for what to do for this PR).

tac0turtle · 2022-10-21T14:26:46Z

gonna close this for now, we can add this to our next sprint as something to work on

add gas-related parameters to client config; pending tests

befb240

github-actions bot added the C:CLI label Jul 26, 2022

larry0x mentioned this pull request Jul 26, 2022

add gas-related parameters to client config #12722

Closed

print warnings if gas parameters are not specified by CLI

c08b594

update rosetta test data

53accfb

github-actions bot added the C:Rosetta Issues and PR related to Rosetta label Jul 28, 2022

larry0x added 2 commits July 29, 2022 01:01

Merge branch 'main' into larry/gas-client-config

6eaa783

update changelog

5c245bd

larry0x marked this pull request as ready for review July 29, 2022 00:16

larry0x requested a review from a team as a code owner July 29, 2022 00:16

alexanderbez reviewed Jul 29, 2022

View reviewed changes

larry0x commented Jul 29, 2022

View reviewed changes

client/tx/factory.go Outdated Show resolved Hide resolved

client/tx/factory.go Show resolved Hide resolved

larry0x added 5 commits July 29, 2022 03:53

handle errors when parsing CLI input; add comments

5566bb3

Delete debug_container.dot

182587d

fix comments

5f59f6e

update UPGRADING.md

7c099f8

Merge branch 'main' into larry/gas-client-config

9092757

julienrbrt approved these changes Jul 29, 2022

View reviewed changes

larry0x marked this pull request as draft July 30, 2022 02:36

fix handling of CLI flags

eca07a7

larry0x added 2 commits July 30, 2022 04:31

make the gas parameters in client.toml optional

75faaf5

Merge branch 'larry/gas-client-config' of https://github.com/larry0x/…

7493f83

…cosmos-sdk into larry/gas-client-config

github-actions bot added the stale label Oct 14, 2022

github-actions bot removed the stale label Oct 15, 2022

tac0turtle closed this Oct 21, 2022

larry0x mentioned this pull request Jan 17, 2023

Reducing flags in junod commands CosmosContracts/juno#518

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: add gas-related parameters to client config #12723

feat!: add gas-related parameters to client config #12723

larry0x commented Jul 26, 2022 •

edited

Loading

fedekunze commented Jul 26, 2022

alexanderbez commented Jul 26, 2022

larry0x commented Jul 27, 2022 •

edited

Loading

larry0x commented Jul 27, 2022 •

edited

Loading

larry0x commented Jul 27, 2022

alexanderbez commented Jul 27, 2022

larry0x commented Jul 28, 2022

alexanderbez commented Jul 28, 2022

larry0x commented Jul 28, 2022 •

edited

Loading

alexanderbez Jul 29, 2022

larry0x Jul 29, 2022 •

edited

Loading

julienrbrt Jul 29, 2022

larry0x Jul 29, 2022

alexanderbez Jul 30, 2022 •

edited

Loading

larry0x Jul 30, 2022

larry0x Jul 30, 2022

larry0x Jul 30, 2022

alexanderbez Aug 1, 2022

alexanderbez Jul 29, 2022

larry0x Jul 29, 2022

larry0x Jul 29, 2022 •

edited

Loading

alexanderbez Aug 1, 2022

julienrbrt left a comment •

edited

Loading

codecov bot commented Jul 29, 2022 •

edited

Loading

tac0turtle commented Aug 29, 2022

larry0x commented Aug 29, 2022

github-actions bot commented Oct 14, 2022

gjermundgaraba commented Oct 14, 2022

alexanderbez commented Oct 14, 2022

tac0turtle commented Oct 21, 2022

	} else if f.chainID == "" {
	return nil, fmt.Errorf("chain ID required but not specified")
	}

feat!: add gas-related parameters to client config #12723

feat!: add gas-related parameters to client config #12723

Conversation

larry0x commented Jul 26, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

fedekunze commented Jul 26, 2022

alexanderbez commented Jul 26, 2022

larry0x commented Jul 27, 2022 • edited Loading

larry0x commented Jul 27, 2022 • edited Loading

larry0x commented Jul 27, 2022

alexanderbez commented Jul 27, 2022

larry0x commented Jul 28, 2022

alexanderbez commented Jul 28, 2022

larry0x commented Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

larry0x Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Jul 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larry0x Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienrbrt left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jul 29, 2022 • edited Loading

Codecov Report

tac0turtle commented Aug 29, 2022

larry0x commented Aug 29, 2022

github-actions bot commented Oct 14, 2022

gjermundgaraba commented Oct 14, 2022

alexanderbez commented Oct 14, 2022

tac0turtle commented Oct 21, 2022

larry0x commented Jul 26, 2022 •

edited

Loading

larry0x commented Jul 27, 2022 •

edited

Loading

larry0x commented Jul 27, 2022 •

edited

Loading

larry0x commented Jul 28, 2022 •

edited

Loading

larry0x Jul 29, 2022 •

edited

Loading

alexanderbez Jul 30, 2022 •

edited

Loading

larry0x Jul 29, 2022 •

edited

Loading

julienrbrt left a comment •

edited

Loading

codecov bot commented Jul 29, 2022 •

edited

Loading