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

Support custom extension options when building tx #2573

Merged
merged 12 commits into from
Aug 24, 2022

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Aug 17, 2022

Description

Closes: #2566

Support custom extension options to be able to specify max_priority_price in ethermint dynamic fee tx.


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
Copy link
Member

romac commented Aug 17, 2022

If I understand correctly, this would be used as follows in the config file:

[[chains]]
id = '...'

# ...

extension_options = [
  { type = "ethermint_dynamic_fee", value = "10" }
]

Right?

For our understanding, can you please elaborate on what this means for Ethermint? It's not super clear in the issue what are the semantics of that extension.

@yihuang
Copy link
Contributor Author

yihuang commented Aug 17, 2022

If I understand correctly, this would be used as follows in the config file:

[[chains]]

id = '...'



# ...



extension_options = [

  { type = "ethermint_dynamic_fee", value = "10" }

]

Right?

For our understanding, can you please elaborate on what this means for Ethermint? It's not super clear in the issue what are the semantics of that extension.

Yes, there's some contexts here: #2350 (comment), it's needed to fix the reorder issue after we enable the tx priority and feemarket.

@yihuang
Copy link
Contributor Author

yihuang commented Aug 18, 2022

We did an integration test with it: crypto-org-chain/cronos#652

@romac romac marked this pull request as draft August 18, 2022 15:13
@yihuang
Copy link
Contributor Author

yihuang commented Aug 22, 2022

@romac what's your opinion on this? this is necessary for all ethermint chains, including cronos and evmos.

@romac
Copy link
Member

romac commented Aug 22, 2022

I am in favor of this change, thank you very much for suggesting it. Unfortunately, it won't make it into v1.0 (which will go very soon) but we will merge and release it into Hermes v1.1.

@romac romac added this to the v1.1 milestone Aug 22, 2022
@seanchen1991 seanchen1991 marked this pull request as ready for review August 22, 2022 15:39
@romac romac merged commit 13669ca into informalsystems:master Aug 24, 2022
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
Closes: informalsystems#2566

* Support custom extension options when building tx

* Cargo.lock is outdated

* optimize serde encoding

* fix clippy and test

* Add changelog entry

* Fix clippy warnings

* Fix compilation

* Fix clippy warnings

Co-authored-by: Sean Chen <seanchen11235@gmail.com>
Co-authored-by: Romain Ruetschi <romain@informal.systems>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support specify more flexible gas price on feemarket chains
3 participants