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

feat: bump ethers to v6 #6224

Merged
merged 68 commits into from
Feb 27, 2024
Merged

feat: bump ethers to v6 #6224

merged 68 commits into from
Feb 27, 2024

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Feb 15, 2024

Description

This bumps ethers to the latest major i.e v6.
While at it, this also moves many of ethers utils to their viem equivalent, minimizing our reliance on ethers. Things like providers and signature/transaction utilities have been kept the ethers way, as:

  • providers instantiating work quite a bit differently in ethers and would warrant their own PR
  • viem doesn't provide things like splitSignature and we would have to manually do secp256k1 elliptic curve business with e.g @noble/curves/secp256k1 or similar libs

Notes:

  1. Not all packages support ethers 6 just yet i.e @uniswap/sdk so this keeps ethers v5 under the ethers5 yarn alias, and also adds ethers v5 providers in our ethers singleton
  2. hdwallet has not been updated to v6 yet, hence required skipping some tests for the time being - despite still using v5, vitest would try and use v5 there as we cannot leverage the ethers/lib/utils react-app-rewired aliasing that we use in web, in vitest.

Tested:

  • asset generation script is happy - seems to be, although will need to be tested with Zerion API key cc @woodenfurniture
  • unchained-client parsing is happy
  • FOXy is happy
  • hdwallet is happy
  • wherever is happy
  • EVM Li.Fi is happy
  • CoW swaps are happy
  • Ledger EVM is happy
  • DeFi opportunities are happy
  • errors.ts:117 Uncaught (in promise) TypeError: unknown ProviderEvent (argument="event", value="accountsChanged", code=INVALID_ARGUMENT, version=6.11.1) - Browser provider does not allow to subscribe for EIP1193 events ethers-io/ethers.js#4334 still open upstream, but tested accounts and chain switch locally and this only seems to be error spew and not actually break anything
  • skip useSendDetails and useFormSend tests until wip: bump ethers to v6 hdwallet#666 goes in

Pull Request Type

  • 🐛 Bug fix (Non-breaking Change: Fixes an issue)
  • 🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • 💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

Risk

High Risk PRs Require 2 approvals

Very high. Anything EVM could be broken, see the testing done in the PR description body above. We should give this the highest scrutiny and not be in any kind of rush to get this in, as this puts us in a better state lib-wise, leveraging the latest ethers major, but isn't something that should get in ASAP.

What protocols, transaction types or contract interactions might be affected by this PR?

Testing

  • See Tested above. Anything EVM should be tested. Connection (for all wallets), accounts balances, DeFi metadata and user data, accounts/chain switch, approvals/allowance checks, Tx building and broadcasting (send, DeFi), message signing (CoW/wherever), etc.
  • This should be tested with MM, native, and Ledger at the very least.
  • There is no need to specifically test broadcasting mainnet Txs, any EVM chain will give us guarantees.

Engineering

  • The changeset looks sane with v5 still being consumed, and we are confident in the temporary disabling of the send tests

Operations

  • ☝🏽

Screenshots (if applicable)

Copy link
Contributor Author

gomesalexandre commented Feb 15, 2024

@gomesalexandre gomesalexandre force-pushed the feat_switch_static_json_rpc_provider branch from ab15bed to 5a0dd6d Compare February 19, 2024 14:00
Base automatically changed from feat_switch_static_json_rpc_provider to develop February 19, 2024 17:30
@woodenfurniture woodenfurniture self-assigned this Feb 25, 2024
Copy link
Member

@woodenfurniture woodenfurniture left a comment

Choose a reason for hiding this comment

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

code-wise looking fantastic ser, will wait for second pass to dig into deep functional testing

@jxom
Copy link

jxom commented Feb 26, 2024

viem doesn't provide things like splitSignature and we would have to manually do secp256k1 elliptic curve business with e.g @noble/curves/secp256k1 or similar libs

Viem should have all you need here. We do have a splitSignature equiv here: https://viem.sh/docs/utilities/hexToSignature

@woodenfurniture
Copy link
Member

woodenfurniture commented Feb 26, 2024

Testing pass 1 notes

✅ Asset generation script happy
✅ Wherever happy
✅ Connecting metamask
✅ Connecting native
✅ Switching between metamask and native
✅ Switching between native and native
✅ EVM quotes looking correct with no errors
✅ EVM gas estimation working as intended (on quote, approval, trade execution, lending)
✅ EVM swaps on lifi (see below for errors though)
✅ EVM swaps on 0x (see below for errors though)
✅ EVM swaps on CoW (see below for errors though)
✅ ETH Lending -> borrow, full user flow, UI and requests working without errors
✅ ETH Lending -> repay, basic initial load (not full repayment flow), UI and requests working without errors


❌ EVM allowance approvals - the allowance step disappears from the UI steps which is confusing. Unsure if this is related to this PR or an existing issue

https://github.com/shapeshift/web/assets/125113430/42387e9c-ea2c-4e52-9f4a-167a2e50deab
https://github.com/shapeshift/web/assets/125113430/c798af86-40e6-43c0-b7e2-408e65f3e642


❌ voting power snapshot requests fire periodically (as expected), but frequently failing with code 500
https://github.com/shapeshift/web/assets/125113430/e9527fc4-d444-4bbe-b916-6023dfa80048

image
image


❌ EVM swap on 0x - unsure if PR related or other cause
https://snowtrace.dev/tx/0x8418ff1d5118a57a4605d0bc014070613b8b15602892918babe825e3f1109de1

image
image


❌ ERC20 allowance approval on lifi - clicking "sign transaction" on the allowance approval resulted in error toast:
image
image
image

Then it seems the allowance succeeds somhow (?) but the allowance step disappears. Clicking "sign transaction" results in successful trade with an error on the approval:
image
image

Both txs (approval and swap txs) are successful though
https://snowtrace.dev/tx/0x94161307df7732867db4bea79cb1c5b229986ee99d109ed80ce4258f5b2eb201
https://snowtrace.dev/tx/0x610b1d014fcc2be4f5abb46a6190b663a25713204cab335a18943bf53fcf0d35


Non-blockers / unrelated to this PR

Lending page shows react error spam related to missing key prop
image

Copy link
Member

@woodenfurniture woodenfurniture left a comment

Choose a reason for hiding this comment

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

LGTM now, all issues found unrelated to this pr or have existing ticket for separate fix

@gomesalexandre gomesalexandre enabled auto-merge (squash) February 27, 2024 03:33
@gomesalexandre gomesalexandre merged commit 4c92cde into develop Feb 27, 2024
4 checks passed
@gomesalexandre gomesalexandre deleted the feat_ethers_v6 branch February 27, 2024 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants