-
Notifications
You must be signed in to change notification settings - Fork 70
[WIP] improve(TransactionUtils): Simple priority scaler for replacement transactions #2126
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: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ import { | |
bnZero, | ||
Contract, | ||
isDefined, | ||
fixedPointAdjustment, | ||
TransactionResponse, | ||
ethers, | ||
getContractInfoFromAddress, | ||
|
@@ -53,6 +54,8 @@ export async function getMultisender(chainId: number, baseSigner: Signer): Promi | |
return sdkUtils.getMulticall3(chainId, baseSigner); | ||
} | ||
|
||
const DEFAULT_RETRIES = 2; | ||
|
||
// Note that this function will throw if the call to the contract on method for given args reverts. Implementers | ||
// of this method should be considerate of this and catch the response to deal with the error accordingly. | ||
export async function runTransaction( | ||
|
@@ -63,7 +66,8 @@ export async function runTransaction( | |
value = bnZero, | ||
gasLimit: BigNumber | null = null, | ||
nonce: number | null = null, | ||
retriesRemaining = 2 | ||
retriesRemaining = DEFAULT_RETRIES, | ||
bumpGas = false | ||
): Promise<TransactionResponse> { | ||
const { provider } = contract; | ||
const { chainId } = await provider.getNetwork(); | ||
|
@@ -88,6 +92,16 @@ export async function runTransaction( | |
await contract.populateTransaction[method](...(args as Array<unknown>), { value }) | ||
); | ||
|
||
// Bump the priority fee incrementally on each retry to try to successfully replace a pending transaction. | ||
// Success is not guaranteed since the bot does not know the gas price of the transaction it is trying to replace. | ||
if (bumpGas && gas.maxPriorityFeePerGas) { | ||
const priorityScaler = 1.1 + (1 + DEFAULT_RETRIES - Math.min(retriesRemaining, DEFAULT_RETRIES)) / 10; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At least some aspect of this scaler should probably be configurable. |
||
const oldPriorityFee = gas.maxPriorityFeePerGas; | ||
const newPriorityFee = oldPriorityFee.mul(toBNWei(priorityScaler.toString()).div(fixedPointAdjustment)); | ||
gas.maxFeePerGas = gas.maxFeePerGas.add(newPriorityFee).sub(oldPriorityFee); | ||
gas.maxPriorityFeePerGas = newPriorityFee; | ||
} | ||
|
||
logger.debug({ | ||
at: "TxUtil", | ||
message: "Send tx", | ||
|
@@ -118,7 +132,8 @@ export async function runTransaction( | |
retriesRemaining, | ||
}); | ||
|
||
return await runTransaction(logger, contract, method, args, value, gasLimit, null, retriesRemaining); | ||
const bumpGas = isEthersError(error) && error.message.toLowerCase().includes("underpriced"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. todo: link up more cleanly with the provider layer. The provider layer should be able to explicitly identify these cases and tag them with a code we can evaluate. |
||
return await runTransaction(logger, contract, method, args, value, gasLimit, null, retriesRemaining, bumpGas); | ||
} else { | ||
// Empirically we have observed that Ethers can produce nested errors, so we try to recurse down them | ||
// and log them as clearly as possible. For example: | ||
|
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.
Legacy transactions are currently ignored since issues have only been seen on eip-1559 chains. It would make sense to scale legacy transactions though.