Skip to content
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: Non-zero Default Fees #9371

Merged
merged 31 commits into from
Jun 25, 2021
Merged

feat: Non-zero Default Fees #9371

merged 31 commits into from
Jun 25, 2021

Conversation

cyberbono3
Copy link
Contributor

@cyberbono3 cyberbono3 commented May 20, 2021

Description

closes: #9106


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@cyberbono3 cyberbono3 requested a review from amaury1093 May 20, 2021 17:14
@codecov
Copy link

codecov bot commented May 20, 2021

Codecov Report

Merging #9371 (d77b083) into master (bb61e28) will increase coverage by 0.00%.
The diff coverage is 37.50%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9371   +/-   ##
=======================================
  Coverage   60.66%   60.66%           
=======================================
  Files         588      588           
  Lines       37266    37273    +7     
=======================================
+ Hits        22607    22612    +5     
  Misses      12715    12715           
- Partials     1944     1946    +2     
Impacted Files Coverage Δ
server/config/config.go 40.35% <0.00%> (-1.10%) ⬇️
testutil/network/util.go 69.72% <0.00%> (-1.31%) ⬇️
types/errors/errors.go 80.32% <ø> (ø)
simapp/simd/cmd/root.go 67.33% <100.00%> (+0.44%) ⬆️
server/logger.go 33.33% <0.00%> (+33.33%) ⬆️

@cyberbono3 cyberbono3 force-pushed the cyberbono3/non-zero-default-fees branch from c5b8a52 to c85f811 Compare May 26, 2021 10:12
@cyberbono3
Copy link
Contributor Author

cyberbono3 commented May 26, 2021

I tested 2 cases:

  1. Set MinGasPrices = "" in app.toml
./build/simd start
Error: MinGasPrices must not be empty
  1. Set MinGasPrices to "0" in app.toml
./build/simd start
1:14PM INF starting ABCI with Tendermint
panic: invalid minimum gas prices: invalid decimal coin expression: 0

Questions:

  1. Should I test cases to check precedence rules with envVars, flags? I guess, Viper handles it automatically.
  2. Since empty value for MinGasPrices in app.toml is not allowed, what should be the default value for MinGasPrices in this case?

server/start.go Outdated Show resolved Hide resolved
server/util.go Outdated Show resolved Hide resolved
@amaury1093
Copy link
Contributor

Should I test cases to check precedence rules with envVars, flags? I guess, Viper handles it automatically.

it would be useful to still test it.

Since empty value for MinGasPrices in app.toml is not allowed, what should be the default value for MinGasPrices in this case?

There shouldn't by any default value, the app should exit.

server/config/config.go Outdated Show resolved Hide resolved
server/config/config.go Outdated Show resolved Hide resolved
server/config/config.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added the C:CLI label May 28, 2021
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, but we should add a test

server/util.go Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the C:CLI label May 28, 2021
@cyberbono3
Copy link
Contributor Author

Error: No release type found in pull request title "Non-zero Default Fees ". Add a prefix to indicate what kind of release this pull request corresponds to (see https://www.conventionalcommits.org/). what kind of prefix should I add here?

@tac0turtle tac0turtle changed the title Non-zero Default Fees fix: Non-zero Default Fees Jun 4, 2021
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this R4R

@cyberbono3 cyberbono3 marked this pull request as ready for review June 6, 2021 10:21
@cyberbono3 cyberbono3 requested a review from aaronc as a code owner June 6, 2021 10:21
@cyberbono3
Copy link
Contributor Author

is this R4R

yes

@cyberbono3 cyberbono3 force-pushed the cyberbono3/non-zero-default-fees branch from 4ded361 to 1c6ea9c Compare June 9, 2021 17:05
@amaury1093
Copy link
Contributor

@cyberbono3 could you also fix the test?

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. 2 not blocking small things (would be good to have it in this PR):

  • update error to wrap from errors package (as in the comment below)
  • update setup guide (docs)

server/config/config.go Outdated Show resolved Hide resolved
@cyberbono3
Copy link
Contributor Author

Looks good. 2 not blocking small things (would be good to have it in this PR):

  • update error to wrap from errors package (as in the comment below)
  • update setup guide (docs)

what setup guide do you mean here?

@cyberbono3
Copy link
Contributor Author

@cyberbono3 could you also fix the test?

I fixed that.

@amaury1093
Copy link
Contributor

amaury1093 commented Jun 24, 2021

@cyberbono3 did you start debugging why test-rosetta and liveness-test fail?

Edit:

  • to fix liveness-test, we should probably add --minimum-gas-prices=something:
cosmossdk/simd-env testnet --v 4 -o . --starting-ip-address 192.168.10.2 --keyring-backend=test --minimum-gas-prices=0stake ; fi

@cyberbono3
Copy link
Contributor Author

@cyberbono3 did you start debugging why test-rosetta and liveness-test fail?

Edit:

  • to fix liveness-test, we should probably add --minimum-gas-prices=something:
cosmossdk/simd-env testnet --v 4 -o . --starting-ip-address 192.168.10.2 --keyring-backend=test --minimum-gas-prices=0stake ; fi

I ran this command cosmossdk/simd-env testnet --v 4 -o . --starting-ip-address 192.168.10.2 --keyring-backend=test --minimum-gas-prices=0stake

Can you give me some context how to fix rosetta-test and liveness, please?

test_rosetta_1  | waiting for rosetta instance to be up
test_rosetta_1  | Terminated
rosetta_test_rosetta_1 exited with code 143

Should I look for this error code code 143 in the codebase?

@amaury1093
Copy link
Contributor

amaury1093 commented Jun 24, 2021

Can you give me some context how to fix rosetta-test and liveness, please?

For liveness, as I pointed out above, try maybe adding cosmossdk/simd-env testnet --v 4 -o . --starting-ip-address 192.168.10.2 --keyring-backend=test --minimum-gas-prices=0stake ; fi in the Makefile in localnet-start

For rosetta, not 100% sure where we should define min-gas-prices. Are there more logs?

@amaury1093
Copy link
Contributor

amaury1093 commented Jun 24, 2021

Looking at the osmosis launch, they explicitly said leave it empty. Is there a way the app developer could override this functionality in order to make a better UX if need be?

Maybe we should put this as public:

defaultMinGasPrices = ""

So that app devs can do serverconfig.DefaultMinGasPrices = "whatever" (e.g. even "0denom") and that'll be set by default in all nodes' app.toml. wdyt @marbar3778 ? (although it's a new global var)

edit: #9371 (comment) seems like a better solution, as it doesn't introduce a global var

@robert-zaremba
Copy link
Collaborator

what setup guide do you mean here?

There is a guide on how to run a node - let's add a line there that the min gas price must be correctly set (with an example).

@robert-zaremba
Copy link
Collaborator

Can you give me some context how to fix rosetta-test and liveness, please?

Maybe check with @noandrea about the Rosetta test. Not sure why it fails when we add the min fee check.

Comment on lines +101 to +116
// Optionally allow the chain developer to overwrite the SDK's default
// server config.
srvCfg := serverconfig.DefaultConfig()
// The SDK's default minimum gas price is set to "" (empty value) inside
// app.toml. If left empty by validators, the node will halt on startup.
// However, the chain developer can set a default app.toml value for their
// validators here.
//
// In summary:
// - if you leave srvCfg.MinGasPrices = "", all validators MUST tweak their
// own app.toml config,
// - if you set srvCfg.MinGasPrices non-empty, validators CAN tweak their
// own app.toml to override, or use this default value.
//
// In simapp, we set the min gas prices to 0.
srvCfg.MinGasPrices = "0stake"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a chat with @aaronc last evening about this, we were looking around Viper.SetDefault, but this morning I noticed this new initAppConfig function introduced by @anilcse in #9550. And it seems to me like the perfect place for chain developers to set a default min gas price.

cc. @marbar3778

Copy link
Member

@tac0turtle tac0turtle Jun 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is awesome!! We should add a note in the docs, that @anilcse added, that this is possible.

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval pending an addition to docs.

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK.

Manual test:

  • set srvCfg.MinGasPrices = "10stake" in cmd/root.go, node runs correctly, all txs need a --fee
  • set srvCfg.MinGasPrices = "", got error: Error: set min gas price in app.toml or flag or env variable: error in app.toml when running the node
    • node runs correctly if I edit app.toml manually

@github-actions github-actions bot added the C:Rosetta Issues and PR related to Rosetta label Jun 25, 2021
@amaury1093
Copy link
Contributor

tests pass, docs added, I believe all reviews were addressed... putting automerge on!

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Jun 25, 2021
@mergify mergify bot merged commit 3fd376b into master Jun 25, 2021
@mergify mergify bot deleted the cyberbono3/non-zero-default-fees branch June 25, 2021 10:41
mergify bot pushed a commit that referenced this pull request Jun 25, 2021
<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺
v                               ✰  Thanks for creating a PR! ✰
v    Before smashing the submit button please review the checkboxes.
v    If a checkbox is n/a - please still include it but + a little note why
☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >  -->

## Description

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review.
-->

closes: #9106

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [ ] Wrote unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [ ] Re-reviewed `Files changed` in the Github PR explorer
- [ ] Review `Codecov Report` in the comment section below once CI passes

(cherry picked from commit 3fd376b)

# Conflicts:
#	CHANGELOG.md
#	contrib/rosetta/node/data.tar.gz
#	docs/core/cli.md
#	server/util_test.go
#	simapp/simd/cmd/root.go
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺
v                               ✰  Thanks for creating a PR! ✰
v    Before smashing the submit button please review the checkboxes.
v    If a checkbox is n/a - please still include it but + a little note why
☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >  -->

## Description

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review.
-->

closes: cosmos#9106

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [ ] Wrote unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [ ] Re-reviewed `Files changed` in the Github PR explorer
- [ ] Review `Codecov Report` in the comment section below once CI passes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:Rosetta Issues and PR related to Rosetta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-zero Default Fees
8 participants