Skip to content
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

refactor(apps/price_pusher): Use viem instead of web3 for evm pusher #1829

Merged
merged 8 commits into from
Aug 21, 2024

Conversation

ali-bahjati
Copy link
Collaborator

@ali-bahjati ali-bahjati commented Aug 16, 2024

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.

Copy link

vercel bot commented Aug 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
api-reference ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 21, 2024 11:18am
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
xc-admin-frontend ⬜️ Ignored (Inspect) Visit Preview Aug 21, 2024 11:18am

Copy link
Contributor

@jayantk jayantk left a 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);
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

apps/price_pusher/src/evm/evm.ts Show resolved Hide resolved
@@ -0,0 +1,656 @@
const IPyth = [
Copy link
Contributor

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cprussin The JSON file is autogenerated from the contract itself.

@jayantk I'll make a separate PR for it because I need to do some clean up there as well.

Copy link
Member

@aditya520 aditya520 left a 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.

Copy link
Collaborator

@cprussin cprussin left a 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

apps/price_pusher/src/evm/chains.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,656 @@
const IPyth = [
Copy link
Collaborator

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.

apps/price_pusher/src/utils.ts Outdated Show resolved Hide resolved
apps/price_pusher/src/evm/evm.ts Outdated Show resolved Hide resolved
apps/price_pusher/src/evm/evm.ts Outdated Show resolved Hide resolved
apps/price_pusher/src/evm/evm.ts Outdated Show resolved Hide resolved
apps/price_pusher/src/evm/evm.ts Outdated Show resolved Hide resolved
apps/price_pusher/src/evm/evm.ts Outdated Show resolved Hide resolved
@ali-bahjati ali-bahjati merged commit 5c3be6a into main Aug 21, 2024
5 checks passed
@ali-bahjati ali-bahjati deleted the price-pusher/use-viem-for-evm branch August 21, 2024 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants