Skip to content

Conversation

@jxom
Copy link
Contributor

@jxom jxom commented Jun 6, 2024

Fixes issue related to: wevm/viem#2306


PR-Codex overview

This PR updates various files in thirdweb package related to Ethereum adapters, wallet functions, package dependencies, and test cases.

Detailed summary

  • Updated ethers6.ts and ethers5.ts to include accessList property
  • Updated package.json with new versions for dependencies
  • Modified test cases to run based on a secret key
  • Refactored wallet functions to use serializeTypedData
  • Updated dependencies and resolved peer dependency issues

The following files were skipped due to too many changes: packages/thirdweb/src/extensions/marketplace/english-auctions/english-auctions.test.ts, packages/thirdweb/src/extensions/marketplace/direct-listings/direct-listings.test.ts

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@vercel
Copy link

vercel bot commented Jun 6, 2024

@jxom is attempting to deploy a commit to the thirdweb Team on Vercel.

A member of the Team first needs to authorize it.

@changeset-bot
Copy link

changeset-bot bot commented Jun 6, 2024

⚠️ No Changeset found

Latest commit: 89637f6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@jxom jxom changed the title fix: resolves https://github.com/wevm/viem/issues/2306 fix: resolves viem#2306 Jun 6, 2024
@joaquim-verges
Copy link
Member

thanks for this @jxom ! Not lowercasing strings types makes sense, but I'm actually surprised we need this lowercasing of address types in the first place, would have expected checksum instead.

Can you point me to some resources to understand why addresses need to be lowercased before sending with eth_signTypedData? Reason I'm asking is because we might have the same bug in Unity/dotnet, want to make sure we do the right thing there.

@codspeed-hq
Copy link

codspeed-hq bot commented Jun 6, 2024

CodSpeed Performance Report

Merging #3237 will degrade performances by 16.01%

Comparing jxom:viem-2306 (89637f6) with main (dbf74aa)

Summary

⚡ 2 improvements
❌ 1 regressions
✅ 6 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main jxom:viem-2306 Change
encode tx (contract abi) 403.2 µs 202.1 µs +99.46%
encode tx (prepared method) 338.1 µs 188.2 µs +79.63%
keccakId 1.3 ms 1.6 ms -16.01%

@jxom
Copy link
Contributor Author

jxom commented Jun 6, 2024

Not all Wallets behave correctly when they are passed a checksum address. Can’t remember off the top of my head which Wallets didn’t behave correctly. This behavior exists in Ethers as well.

Even if we think it may not be an issue anymore, I don’t think it is worth the risk. Typed data spec is brittle in itself.

@socket-security
Copy link

socket-security bot commented Jun 6, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/viem@2.13.7 None 0 0 B

🚮 Removed packages: npm/viem@2.10.9

View full report↗︎

@codecov
Copy link

codecov bot commented Jun 6, 2024

Codecov Report

Attention: Patch coverage is 9.09091% with 30 lines in your changes missing coverage. Please review.

Project coverage is 52.32%. Comparing base (8acf564) to head (89637f6).
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #3237       +/-   ##
===========================================
- Coverage   63.35%   52.32%   -11.03%     
===========================================
  Files         833      828        -5     
  Lines       63389    63026      -363     
  Branches     3428     2576      -852     
===========================================
- Hits        40157    32980     -7177     
- Misses      22555    29376     +6821     
+ Partials      677      670        -7     
Flag Coverage Δ *Carryforward flag
legacy_packages 65.61% <ø> (ø) Carriedforward from 8acf564
packages 49.54% <9.09%> (-13.34%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

Files Coverage Δ
packages/thirdweb/src/adapters/ethers6.ts 56.66% <0.00%> (-1.13%) ⬇️
...thirdweb/src/wallets/coinbase/coinbaseSDKWallet.ts 33.39% <14.28%> (-0.27%) ⬇️
packages/thirdweb/src/wallets/injected/index.ts 20.90% <14.28%> (-0.43%) ⬇️
.../thirdweb/src/wallets/wallet-connect/controller.ts 20.49% <14.28%> (-0.22%) ⬇️
packages/thirdweb/src/adapters/ethers5.ts 55.44% <0.00%> (-1.11%) ⬇️

... and 192 files with indirect coverage changes

@joaquim-verges joaquim-verges merged commit 558885a into thirdweb-dev:main Jun 6, 2024
@jxom jxom deleted the viem-2306 branch June 6, 2024 22:15
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.

2 participants