feat(gasPriceOracle): Allow caller to pass in base fee multiplier #801
Conversation
We should have quoting API apply its GAS_MARKUP to the base fee rather than the estimated gas units
Based on SDK changes in across-protocol/sdk#801
src/utils/common.ts
Outdated
| const gasMultiplier = 1.0; // Don't apply any multiplier to the gas price. | ||
| const queries = [ | ||
| gasUnits ? Promise.resolve(BigNumber.from(gasUnits)) : voidSigner.estimateGas(unsignedTx), | ||
| _gasPrice ? Promise.resolve({ maxFeePerGas: _gasPrice }) : getGasPriceEstimate(provider, chainId, transport), | ||
| _gasPrice ? Promise.resolve({ maxFeePerGas: _gasPrice }) : getGasPriceEstimate(provider, chainId, gasMultiplier, transport), |
There was a problem hiding this comment.
@nicholaspai I updated this to fix a merge conflcit after merging my PR, but I'm curious about why we don't want to apply any buffer here. This is where the API would otherwise want to apply it, if I've understood correctly.
There was a problem hiding this comment.
Oh you're right this is a mistake. I only applied it to the fresh query to the oracle but not to the input value if defined
There was a problem hiding this comment.
Is this what you were thinking? 4a0858a
There was a problem hiding this comment.
@nicholaspai I'm not sure tbh. Do we want to apply the multiplier if the caller specifies the gasPrice? There is a risk that we revert to the recursive gas price multiplication that we had a while ago (i.e. the API sources a gas price including multiplier, then supplies it back to the gasPriceOracle and it gets multiplied again).
But I was also looking at line 268, where we have const gasMultiplier = 1.0. I think we want that to be user-supplied, rather than hardcoded in this file, right?
I'm thinking that we might need a more advanced way of specifying inputs, such that the user can supply a gas price or a gasPriceMultiplier, but not both. OK if I propose that?
There was a problem hiding this comment.
You're right this is a mistake. We should allow caller to pass in gas price multiplier and NOT apply it if the gas price is passed in.
| logger: Logger = DEFAULT_LOGGER, | ||
| gasMarkup = 0 | ||
| logger: Logger = DEFAULT_LOGGER | ||
| // gasMarkup = 0 | ||
| ) { | ||
| assert(isDefined(spokePoolAddress)); | ||
| super( | ||
| provider, | ||
| symbolMapping, | ||
| spokePoolAddress, | ||
| simulatedRelayerAddress, | ||
| gasMarkup, | ||
| // gasMarkup, |
There was a problem hiding this comment.
fwiw I think we need to retain these, otherwise the API will just inherit the default 1.0 multiplier.
There was a problem hiding this comment.
i'm confused by your comment here, what do you mean? It seems that we are removing the gasMarkup param from the relayFeeCalculator's constructor so why would we need to retain it?
There was a problem hiding this comment.
Also I think this might be relevant that we'll merge in this pr alongside an import of this sdk release into the FE
There was a problem hiding this comment.
Ah, I understand now. So the gas multiplier would effectively only apply in the API?
There was a problem hiding this comment.
That's how we currently use the multiplier so yeah
| @@ -256,18 +256,21 @@ export async function estimateTotalGasRequiredByUnsignedTransaction( | |||
| options: Partial<{ | |||
| gasPrice: BigNumberish; | |||
| gasUnits: BigNumberish; | |||
| baseFeeMultiplier: number; | |||
There was a problem hiding this comment.
@pxrl I added in an optional multiplier arg here. I think in most existing use cases of this function we only want the default multiplier but PTAL
Builds on across-protocol/sdk#801 and applies relayer's base fee multiplier to new SDK parameter to scale the base fee
We should have quoting API apply its GAS_MARKUP to the base fee rather than the estimated gas units