Update dynamic gas fee to work with FeeMarket #4004
Conversation
.changelog/unreleased/improvements/ibc-relayer/4000-update-dynamic-gas-fee.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Romain Ruetschi <romain@informal.systems> Signed-off-by: Luca Joss <43531661+ljoss17@users.noreply.github.com>
| let dynamic_gas_price = query_eip_base_fee(rpc_address) | ||
| let url = if chain_id.name().contains("osmosis") { | ||
| format!( | ||
| "{}abci_query?path=\"/osmosis.txfees.v1beta1.Query/GetEipBaseFee\"", |
There was a problem hiding this comment.
is it possible to have the query paths in the chain config?
There was a problem hiding this comment.
We discussed this with Luca today and felt like we'd rather avoid introducing one more knob only for osmosis, and that special casing would be enough until they move to Skip's fee market module (if ever).
Also, we can't just add this in the config and issue the query blindly, because we have to know whether we are doing an osmosis or fee market query in order to deserialize the response.
We thought about having a config like fee_market_module with values like Osmosis and FeeMarket, but we felt like this might be confusing to users and would rather avoid doing this if we can help it.
Happy to change my mind, but just want to understand what that would bring us?
There was a problem hiding this comment.
Was asking because I find it awkward to have hardcoded strings like this and would be nice if we can avoid it. The fee_market_module would be something like EIP-1559 (in case other chains use the same) or FeeMarket but yes the fact that the query response is different in the first place is another annoyance and I wish they could be aligned. They are not that much different actually and maybe it wouldn't be hard to ask/ have a consistent response.
But we can wait and see if there is convergence on the skip's module or we start seeing more dynamic fee impls.
There was a problem hiding this comment.
Makes sense! But just to recap my point was that there is no benefit to making this configurable because we need bespoke code for handling the potentially different responses format and we cannot just try and deserialize the response o to one format or the other as the various responses might all be proto if compatible and deserialize into each other but with wrong results.
But we can wait and see if there is convergence on the skip's module or we start seeing more dynamic fee impls.
Agreed, I think we should indeed wait until we see how many different impls we end up with. Until then I suggest we hide this from users and only special case osmosis for now. After we'll see if we need a more generic way of configuring this or if everybody converges on Skip's module.
Co-authored-by: Romain Ruetschi <romain@informal.systems> Signed-off-by: Luca Joss <43531661+ljoss17@users.noreply.github.com>
Closes: #4000
Description
This PR updates the dynamic gas fee query to query the Osmosis endpoint only if the chain is Osmosis. Else it will query the Fee Market endpoint, using the configured gas denom.
PR author checklist:
unclog.Added tests: integration (for Hermes) or unit/mock tests (for modules).docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.