-
Notifications
You must be signed in to change notification settings - Fork 245
refactor(apps/price_pusher): Use viem instead of web3 for evm pusher #1829
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
b8229e0
to
fe7f491
Compare
cf04594
to
a090bef
Compare
a090bef
to
50fd666
Compare
50fd666
to
265dc31
Compare
265dc31
to
110a618
Compare
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.
looks good to me
let gasPrice = | ||
Number(await this.customGasStation?.getCustomGasPrice()) || | ||
Number(fees.gasPrice) || | ||
Number(fees.maxFeePerGas); |
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.
is maxFeePerGas here potentially a very large number? wondering if there's a failure mode where we accidentally drain the wallet balance.
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 think this is equivalent to to the gas price itself. If we add a limit, we add a failure chance in spikes of gas price.
@@ -0,0 +1,656 @@ | |||
const IPyth = [ |
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.
this is a bit annoying because we have to update it when the contract API changes but i guess it's fine...
One thought is that we could update the the pyth EVM SDK to export an object like this, then import that here. That would also help Pyth users who use viem.
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.
Isn't this the same as the json exported from the evm sdk? Is this only here like this so that you can get viem type checking to work out?
If that's what it is, I would advise writing and exporting the abi as typescript in the evm sdk and having tooling to generate the json while building the package instead of writing & exporting json directly, because IIUC viem's static typing relies on dependent types and you cannot import json as const in typescript yet. This way, any consumers in typescript could import the .ts extensions, but the .json extension files will still be there for other users and will be identical.
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 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.
Thank you for this. We need viem now.
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.
Overall lgtm, most of my comments are just suggestions for more idiomatic typescript code, feel free to disregard
@@ -0,0 +1,656 @@ | |||
const IPyth = [ |
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.
Isn't this the same as the json exported from the evm sdk? Is this only here like this so that you can get viem type checking to work out?
If that's what it is, I would advise writing and exporting the abi as typescript in the evm sdk and having tooling to generate the json while building the package instead of writing & exporting json directly, because IIUC viem's static typing relies on dependent types and you cannot import json as const in typescript yet. This way, any consumers in typescript could import the .ts extensions, but the .json extension files will still be there for other users and will be identical.
d946dba
to
b3a66f1
Compare
1f6736a
to
e46c461
Compare
e46c461
to
d48d298
Compare
Web3 library is not very widely used anymore due to its complex and magical internal design. Some users were reporting issues like strange timeouts or unsupported RPC calls via web3 (and the deprecated HDWalletProvider by truffle that we use). This change refactors the code to use Viem. The experience with Viem is nice: it has strong types and good utilities. The error handling is also very well designed. The downside is that it has a steep learning curve to get it right.
This change refactors the code based on the PR reviews to make it simpler as well.
Lastly the version is bumped as a major release because the behaviour and logs have changed and it might affect production environments. It also signals the users to test it out properly before using it which is good because all the failure cases might not be handled.