-
Notifications
You must be signed in to change notification settings - Fork 17
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 all 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"), | ||
|
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).