feat!: add gas-related parameters to client config

[larry0x]

Description

Closes: #12722

---

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

• [x] included the correct type prefix in the PR title
• [x] added ! to the type prefix if API or client breaking change
• [x] targeted the correct branch (see PR Targeting)
• [x] provided a link to the relevant issue or specification
• [x] followed the guidelines for building modules
• [x] included the necessary unit and integration tests
• [x] added a changelog entry to CHANGELOG.md
• [x] included comments for documenting Go code
• [x] updated the relevant documentation or specification
• [x] reviewed "Files changed" and left comments if necessary
• [ ] confirmed all CI checks have passed

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]

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

[alexanderbez]

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]

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]

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]

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]

@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]

@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]

make rosetta-data, yup!

[larry0x]

make rosetta-data, yup!

actually, make rosetta-data doesn't work: https://github.com/cosmos/cosmos-sdk/issues/12772

here's what i know:

• my branch is forked from main at the commit https://github.com/cosmos/cosmos-sdk/commit/5decd22cd3a9259bbae151e1e283ab756c4323c7 which was 3 days ago. with this base, the make command works
• once i merge the latest main (https://github.com/cosmos/cosmos-sdk/commit/2bec9d2021918650d3938c3ab242f84289daef80) 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

[codecov[bot]]

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%)	:arrow_down:
client/config/toml.go	55.55% <ø> (ø)
client/context.go	53.64% <0.00%> (-2.64%)	:arrow_down:
client/tx/factory.go	23.27% <0.00%> (-2.33%)	:arrow_down:
client/config/config.go	50.64% <40.00%> (-5.12%)	:arrow_down:
client/gas.go	70.58% <70.58%> (ø)
crypto/keys/internal/ecdsa/privkey.go	81.13% <0.00%> (-1.89%)	:arrow_down:
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

[tac0turtle]

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

[larry0x]

@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[bot]]

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]

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]

@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]

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