-
Notifications
You must be signed in to change notification settings - Fork 11
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
improve: update sdk queries to accurately estimate gas costs #64
Changes from 5 commits
713ea6b
02ca252
11c6252
16f55d0
00daec1
1e82c4e
cf521fa
ffa6704
8be8c9f
451c80e
ede380a
d082441
45df24b
0fdc138
ad4fe99
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 |
---|---|---|
|
@@ -2,6 +2,8 @@ | |
// NODE_URL_42161 | ||
// NODE_URL_288 | ||
// NODE_URL_10 | ||
// NODE_URL_1 | ||
// NODE_URL_137 | ||
Comment on lines
+5
to
+6
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. These two environment variables may need to be added to the e2e test on Git Actions. 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. Alternatively we could use the public infura key here but I do think setting env vars in github actions is more sustainable 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. Do you want to try to add these to the github actions file in |
||
|
||
import dotenv from "dotenv"; | ||
|
||
|
@@ -35,7 +37,8 @@ describe("Queries", function () { | |
]); | ||
}); | ||
test("Ethereum", async function () { | ||
const ethereumQueries = new EthereumQueries(); | ||
const provider = new providers.JsonRpcProvider(process.env.NODE_URL_1); | ||
const ethereumQueries = new EthereumQueries(provider); | ||
await Promise.all([ | ||
ethereumQueries.getGasCosts("USDC"), | ||
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. Should we test that the return values are sensible values? For example, the gas costs are a wei value, decimals are 6, etc? |
||
ethereumQueries.getTokenDecimals("USDC"), | ||
|
@@ -52,7 +55,8 @@ describe("Queries", function () { | |
]); | ||
}); | ||
test("Polygon", async function () { | ||
const polygonQueries = new PolygonQueries(); | ||
const provider = new providers.JsonRpcProvider(process.env.NODE_URL_137); | ||
const polygonQueries = new PolygonQueries(provider); | ||
await Promise.all([ | ||
polygonQueries.getGasCosts("USDC"), | ||
polygonQueries.getTokenDecimals("USDC"), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
import { BigNumber, ethers } from "ethers"; | ||
import { BaseContract, BigNumber, ethers, PopulatedTransaction, providers, VoidSigner } from "ethers"; | ||
import * as uma from "@uma/sdk"; | ||
import Decimal from "decimal.js"; | ||
import { isL2Provider, L2Provider } from "@eth-optimism/sdk"; | ||
|
||
export type BigNumberish = string | number | BigNumber; | ||
export type BN = BigNumber; | ||
|
@@ -222,3 +223,63 @@ export async function retry<T>(call: () => Promise<T>, times: number, delayS: nu | |
}); | ||
return promiseChain; | ||
} | ||
|
||
/** | ||
* Estimates the total gas cost required to submit an unsigned (populated) transaction on-chain | ||
* @param unsignedTx The unsigned transaction that this function will estimate | ||
* @param relayerAddress The address that the transaction will be submitted from () | ||
* @param provider A valid ethers provider - will be used to reason the gas price | ||
* @param gasPrice A manually provided gas price - if set, this function will not resolve the current gas price | ||
* @returns The total gas cost to submit this transaction - i.e. gasPrice * estimatedGasUnits | ||
*/ | ||
export async function estimateTotalGasRequiredByUnsignedTransaction( | ||
unsignedTx: PopulatedTransaction, | ||
relayerAddress: string, | ||
mrice32 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
provider: providers.Provider | L2Provider<providers.Provider>, | ||
gasPrice?: BigNumberish | ||
): Promise<BigNumberish> { | ||
const voidSigner = new VoidSigner(relayerAddress, provider); | ||
// Verify if this provider has been L2Provider wrapped | ||
if (isL2Provider(provider)) { | ||
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. nit: can we make this comment more specific that this detects whether this is an Optimism-specific provider? The way it's written now, it almost seems like all L2s get captured in this if, whereas this logic is Optimism-specific. Alternatively, you could rename isL2Provider/L2Provider on import to something more optimism specific. Either way is cool with me! 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. Updated here. To really highlight the difference, I added both a comment & changed the function alias |
||
const populatedTransaction = await voidSigner.populateTransaction(unsignedTx); | ||
return (await provider.estimateTotalGasCost(populatedTransaction)).toString(); | ||
} else { | ||
// Estimate the Gas units required to submit this transaction | ||
const estimatedGasUnits = await voidSigner.estimateGas(unsignedTx); | ||
// Provide a default gas price of the market rate if this condition has not been set | ||
const resolvedGasPrice = gasPrice ?? (await provider.getGasPrice()); | ||
// Find the total gas cost by taking the product of the gas | ||
// price & the estimated number of gas units needed | ||
return BigNumber.from(resolvedGasPrice).mul(estimatedGasUnits).toString(); | ||
} | ||
} | ||
james-a-morris marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/** | ||
* Create an unsigned transaction of a fillRelay contract call | ||
* @param spokePool The specific spokepool that will populate this tx | ||
* @param destinationTokenAddress A valid ERC20 token (system-wide default is UDSC) | ||
* @param simulatedRelayerAddress The relayer address that relays this transaction | ||
* @returns A populated (but unsigned) transaction that can be signed/sent or used for estimating gas costs | ||
*/ | ||
export async function createUnsignedFillRelayTransaction( | ||
mrice32 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
spokePool: BaseContract, | ||
mrice32 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
destinationTokenAddress: string, | ||
simulatedRelayerAddress: string | ||
): Promise<PopulatedTransaction> { | ||
// Generate a baseline set of function parameters for the fillRelay contract function | ||
const contractFunctionParams = [ | ||
"0xBb23Cd0210F878Ea4CcA50e9dC307fb0Ed65Cf6B", | ||
nicholaspai marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"0xBb23Cd0210F878Ea4CcA50e9dC307fb0Ed65Cf6B", | ||
destinationTokenAddress, | ||
"10", | ||
"10", | ||
"1", | ||
"1", | ||
"1", | ||
"1", | ||
"1", | ||
{ from: simulatedRelayerAddress }, | ||
]; | ||
// Populate and return an unsigned tx as per the given spoke pool | ||
return await spokePool.populateTransaction.fillRelay(...contractFunctionParams); | ||
} | ||
james-a-morris marked this conversation as resolved.
Show resolved
Hide resolved
|
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.
There's quite a lot of duplication across these different classes (arbitrum, polygon, etc.). Can we potentially change the QueryInterface they currently implement to be a class they can inherit all these common methods from? I think currently only Polygon has a different implementation of getTokenPrice() and all other ones look exactly the same (just different addresses which can easily be passed in the constructor)
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 agree - I did notice code-duplication within the chain queries. In this PR I focused primarily on refactoring the duplication within
getGasCosts
But, I think changing QueryInterface to a class structure (assuming there aren't any frontend/relayer dependencies) would be a great move. As you mentioned, the inheritance would help refactor the code into a more succinct format.
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.
It would actually remove all the code for all chains, if I'm not mistaken, except for Polygon, which needs to override getTokenPrice().
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.
Yep! And if we pass a
coinGeckoBaseCurrency
variable to the base class, we can use the shared implementation ofsuper.getTokenPrice()
for the first half of the PolygongetTokenPrice
custom implementation (further cutting the code reuse).