improve: update sdk queries to accurately estimate gas costs #64
Conversation
| // NODE_URL_1 | ||
| // NODE_URL_137 |
There was a problem hiding this comment.
These two environment variables may need to be added to the e2e test on Git Actions.
There was a problem hiding this comment.
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.
Do you want to try to add these to the github actions file in ./.github/?
| @@ -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.
Should we test that the return values are sensible values? For example, the gas costs are a wei value, decimals are 6, etc?
| @@ -19,9 +23,8 @@ export class ArbitrumQueries implements QueryInterface { | |||
| } | |||
|
|
|||
| async getGasCosts(_tokenSymbol: string): Promise<BigNumberish> { | |||
There was a problem hiding this comment.
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.
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.
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.
Yep! And if we pass a coinGeckoBaseCurrency variable to the base class, we can use the shared implementation of super.getTokenPrice() for the first half of the Polygon getTokenPrice custom implementation (further cutting the code reuse).
There was a problem hiding this comment.
This is a great refactor! I left some comments on constructor variable visibility consistency and suggestions for the e2e tests. I also want to make sure that @kevinuma reviews this to make sure its consistent with the potential changes in his relayer-v2 PR
src/utils.ts
Outdated
| // Verify if this provider has been L2Provider wrapped | ||
| if (isL2Provider(provider)) { |
There was a problem hiding this comment.
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.
Updated here. To really highlight the difference, I added both a comment & changed the function alias
| import { SymbolMapping } from "./ethereum"; | ||
| import { Coingecko } from "../../coingecko/Coingecko"; | ||
| import { ArbitrumSpokePool__factory, ArbitrumSpokePool } from "@across-protocol/contracts-v2"; | ||
| import { ArbitrumSpokePool__factory, SpokePool } from "@across-protocol/contracts-v2"; |
There was a problem hiding this comment.
nit: shouldn't these factories all be changed to be SpokePool__factory? I'm a bit surprised that the types are working (I didn't think that on-chain inheritance implies typechain type inheritance, but that's cool!). Anyway, it's probably a good idea to use the same factory type as the contract type you're using just to avoid any type issues in the future.
Co-authored-by: Matt Rice <[email protected]>
This PR aims to reflect the gas cost estimation changes proposed by PR 212 at the SDK level.
During the development of this PR, I found a pattern in the call structure across all chain queries to estimate gas costs. I encapsulated it in two utility functions.
Finally, the SDK e2e test was found to have an error on production, so this PR includes a patch.