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.

Sign up for GitHub

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

Jump to bottom

Add a check for min_gas_price in the health check process #2873

Merged
merged 39 commits into from
Dec 9, 2022
Merged

Add a check for min_gas_price in the health check process #2873

merged 39 commits into from
Dec 9, 2022

Conversation

seanchen1991
Copy link
Contributor

@seanchen1991 seanchen1991 commented Nov 17, 2022 •
edited
Loading

Closes: #2776

Description

Adds a step to the health check process that verifies that the gas_price parameter in Hermes is >= the on-chain min_gas_price parameter.

PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

romac
romac reviewed Nov 17, 2022
View reviewed changes
crates/relayer/src/chain/cosmos.rs Outdated Show resolved Hide resolved
ci/no-std-check/Cargo.toml Outdated Show resolved Hide resolved
adizere
adizere previously requested changes Nov 29, 2022
View reviewed changes
crates/relayer/src/chain/cosmos.rs Outdated Show resolved Hide resolved
crates/relayer/src/chain/cosmos.rs Outdated Show resolved Hide resolved
crates/relayer/src/chain/cosmos.rs Outdated Show resolved Hide resolved
crates/relayer/src/error.rs Outdated Show resolved Hide resolved
crates/relayer/src/config.rs Outdated Show resolved Hide resolved
@adizere
Copy link
Member

adizere commented Nov 29, 2022 •
edited by seanchen1991
Loading

Outstanding todos:

  • parsing of denoms should be able to handle the case of multiple/list min-gas-prices, eg: minimum-gas-price = 0.25token1;0.0001token2
  • make sure we cover the case when minimum-gas-price = ""
  • add more unit tests for GasPrice::try_from
  • handle the case when minimum-gas-price = X and gas_price in Hermes is defined as Y with X != Y, and in that case we should likely emit a warning that it's a possible misconfiguration

seanchen1991 and others added 7 commits November 29, 2022 15:47
@seanchen1991 @adizere
Verify min gas price tests (#2911)
0db501c 
* one test for gas price try_from string

* Rewrote try_from String for GasPrice

* Refactored query_config_params to return Some(), ensuring compatibility with old networks.

Co-authored-by: Adi Seredinschi <a@seredinschi.net>
@seanchen1991
Address PR feedback
803590e
@seanchen1991
Address PR feedback
e23553f
@seanchen1991
Merge branch 'master' into verify-min-gas-price
1195f22
@seanchen1991
Add method for parsing multiple gas prices as well as unit tests
c749a90
@seanchen1991
Merge branch 'verify-min-gas-price' of https://github.com/informalsys…
9805f0a 
…tems/hermes into verify-min-gas-price
@seanchen1991
Improve GasPrice comparison logic in health check function
13919ea
@seanchen1991 seanchen1991 marked this pull request as ready for review December 1, 2022 18:09
@seanchen1991
Copy link
Contributor Author

@adizere Do these changes look good to you?

romac
romac requested changes Dec 6, 2022
View reviewed changes
Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

Great work! I have a few nitpicks but looks great otherwise 🙌

crates/relayer/src/chain/cosmos.rs Outdated Show resolved Hide resolved
crates/relayer/src/chain/cosmos.rs Outdated Show resolved Hide resolved
crates/relayer/src/chain/cosmos.rs Outdated Show resolved Hide resolved
crates/relayer/src/config.rs Outdated Show resolved Hide resolved
crates/relayer/src/config.rs Outdated Show resolved Hide resolved
crates/relayer/src/chain/cosmos.rs Outdated Show resolved Hide resolved
crates/relayer/src/config.rs Show resolved Hide resolved
crates/relayer/src/config.rs Show resolved Hide resolved
@romac romac self-requested a review December 8, 2022 16:20
@romac romac dismissed adizere’s stale review December 9, 2022 12:10

Concerns were addressed

@romac romac merged commit bf19b5e into master Dec 9, 2022
@romac romac deleted the verify-min-gas-price branch December 9, 2022 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adizere adizere adizere left review comments

@romac romac romac approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a min_gas_price verification step in the health check process
3 participants
@seanchen1991 @adizere @romac