-
Notifications
You must be signed in to change notification settings - Fork 406
add dynamic gasfee pricing as a signing client option - Feemarket/Osmosis #1911
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
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| // Multiply by multiplier 18 fractional digits for Dec type | ||
| const fractionalDigits = minGasPrice.amount.fractionalDigits; | ||
| const adjustedGasPrice = multiplyDecimalByNumber( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a cleaner way to do this please advise
|
|
||
| // Replace with your mnemonic or set MNEMONIC env var | ||
| // WARNING: Never commit real mnemonics to git! | ||
| const MNEMONIC = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throwaway mnemonic for you to test with.
| @@ -0,0 +1,278 @@ | |||
| #!/usr/bin/env node | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a complete integration test to validate the PR, can be removed once merged
45ed0de to
713f6bb
Compare
| } | ||
| } | ||
|
|
||
| // Minimal protobuf type definitions matching cosmjs-types pattern |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or should this actually go into cosmjs/types? they refer to /feemarket module ofcourse and not base SDK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's good enough as is and can be refactored later on
webmaster128
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice stuff. Some first thoughts here
| // E.g. https://github.com/cosmos/cosmos-sdk/issues/16020 | ||
| private readonly defaultGasMultiplier = 1.4; | ||
| // Default multiplier for dynamic gas price (applied on top of queried price) | ||
| private readonly defaultDynamicGasMultiplier = 1.3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a gas price multiplier, not a gas multiplier, right? I think it is important to highlight this e.g. by renaming
| private readonly defaultDynamicGasMultiplier = 1.3; | |
| private readonly defaultDynamicGasPriceMultiplier = 1.3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right, oversight by me.
can we make it dynamicGasFeeMultiplier though seeing as it considers the fee portion of the total gasprice configuration ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A "fee" in Cosmos SDK contains the gas (gas limit) a transaction can use as well as the amount of Coins paid. This the price per gas can be calculated from that. So there is everything mixed together.
What we do here is having two data sources for the components of the fee:
- Tha gas (gas limit) is calculated through simulation
- The gas price is either configured or queried from chain starting with this PR
The multiplier here only affects 2. and not 1.
I.e. we do not multiply the fee (gas limit remains unchanged, granter and payer cannot be multiplied) but the gas price.
| fee: "auto" | number, | ||
| ): Promise<StdFee> { | ||
| const gasEstimation = await this.simulate(signerAddress, messages, memo); | ||
| const multiplier = typeof fee === "number" ? fee : this.defaultGasMultiplier; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point we know the gas limit, right? Everything else below is just about the price.
Adding
const gasLimit = Math.ceil(gasEstimation * multiplier)here would remove the 4x Math.round(gasEstimation * multiplier) from below and increase readability.
| const product = (BigInt(normalizedValue.atomics) * BigInt(multiplierDecimal.atomics)) / factor; | ||
|
|
||
| return Decimal.fromAtomics(product.toString(), fractionalDigits); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation might be correct. But as we do not need decimal math here we can also use float math more or less like this:
const x = value.toFloatApproximation() * multiplier; // the value we want
// now convert x to DecimalNow exactly as the Decimal API is limited but I think you get the idea
| const normalizedValue = | ||
| value.fractionalDigits === fractionalDigits | ||
| ? value | ||
| : Decimal.fromUserInput(value.toString(), fractionalDigits); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is something the class Decimal should have. I.e. convert decimal "2.0" to "2.000" by changing the number of decimals from 1 to 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #1916 which will give you an API for that
| } | ||
| } | ||
|
|
||
| // Minimal protobuf type definitions matching cosmjs-types pattern |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's good enough as is and can be refactored later on
| import { Decimal } from "@cosmjs/math"; | ||
| import { fromUtf8 } from "@cosmjs/encoding"; | ||
|
|
||
| import { GasPrice } from "./fee"; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
| const response = new Uint8Array([ | ||
| 10, 17, 51, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, | ||
| ]); | ||
| const queryClient = createMockQueryClient(response); |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
| 10, 25, 10, 5, 117, 97, 116, 111, 109, 18, 16, 53, 51, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, | ||
| 48, 48, | ||
| ]); | ||
| const queryClient = createMockQueryClient(response); |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
|
Thanks for the comments @webmaster128 will resolve remaining issues tomorrow, didnt have time this week yet. |
Summary
Adds automatic gas price discovery for chains with dynamic feemarket modules (Osmosis EIP-1559 and Skip feemarket). Instead of manually setting static gas prices, CosmJS can now query current prices from the chain, preventing transaction failures from outdated pricing.
Changes
Usage
Features
Graceful fallback to minGasPrice if feemarket query fails
Min/max constraint enforcement with configurable multiplier
backward compatible - existing code unchanged
Automatic chain detection for Osmosis variants
Inspired by Hermes relayer PR #4004.