Skip to content

Conversation

@alecananian
Copy link
Contributor

@alecananian alecananian commented Jan 30, 2025

When connecting to an ecosystem wallet with smart wallet options configured in the Thirdweb dashboard, there is a lookup made to fetch any missing details for account abstraction, like the entrypoint address. If the default chain on the dashboard is set to a ZKsync chain (e.g., Treasure chain ID 61166), there is an RPC call made to the factory address that always returns 0x because the factory doesn't exist and is irrelevant for native account abstraction. This change saves the extraneous RPC call during login.


PR-Codex overview

This PR focuses on optimizing the handling of ZkSync chains in the connectSmartAccount function by adding a direct check for these chains and simplifying the gas management logic.

Detailed summary

  • Added a check for ZkSync chains to skip factory entrypoint lookup.
  • Introduced sponsorGas variable to manage gas settings more effectively.
  • Removed redundant code related to ZkSync account creation when a factory is passed.

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

@alecananian alecananian requested review from a team as code owners January 30, 2025 18:06
@changeset-bot
Copy link

changeset-bot bot commented Jan 30, 2025

🦋 Changeset detected

Latest commit: 47dc301

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
thirdweb Patch
@thirdweb-dev/wagmi-adapter Patch

Not sure what this means? Click here to learn what changesets are.

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

@vercel
Copy link

vercel bot commented Jan 30, 2025

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

A member of the Team first needs to authorize it.

@github-actions github-actions bot added packages SDK Involves changes to the thirdweb SDK labels Jan 30, 2025
@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 30, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • merge-queue - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

"gasless" in options ? options.gasless : options.sponsorGas;

if (await isZkSyncChain(chain)) {
if (isZksyncChain) {
Copy link
Member

Choose a reason for hiding this comment

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

might be cleaner to just move this block at the very top

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking was the paymaster and entrypoint address configs were still valid for ZKsync chains, but if that's not the case then yes the conditions can be moved around

Copy link
Member

Choose a reason for hiding this comment

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

no entrypoint is only for non-zk chains, so makes sense to check zk chain first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! In that case, I've pushed an update that simplifies the ZKsync chain check

@codecov
Copy link

codecov bot commented Feb 1, 2025

Codecov Report

Attention: Patch coverage is 0% with 13 lines in your changes missing coverage. Please review.

Project coverage is 42.23%. Comparing base (54c9333) to head (47dc301).
Report is 21 commits behind head on main.

Files with missing lines Patch % Lines
packages/thirdweb/src/wallets/smart/index.ts 0.00% 13 Missing ⚠️

❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

❗ There is a different number of reports uploaded between BASE (54c9333) and HEAD (47dc301). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (54c9333) HEAD (47dc301)
packages 2 1
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #6116       +/-   ##
===========================================
- Coverage   56.87%   42.23%   -14.65%     
===========================================
  Files        1153     1153               
  Lines       63896    63802       -94     
  Branches     5180     3816     -1364     
===========================================
- Hits        36344    26947     -9397     
- Misses      26825    36154     +9329     
+ Partials      727      701       -26     
Flag Coverage Δ *Carryforward flag
legacy_packages 65.68% <ø> (ø) Carriedforward from 54c9333
packages 37.41% <0.00%> (-17.66%) ⬇️

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

Files with missing lines Coverage Δ
packages/thirdweb/src/wallets/smart/index.ts 4.54% <0.00%> (-54.92%) ⬇️

... and 266 files with indirect coverage changes

@joaquim-verges joaquim-verges merged commit 9d5828e into thirdweb-dev:main Feb 1, 2025
25 of 31 checks passed
@joaquim-verges joaquim-verges mentioned this pull request Feb 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

packages SDK Involves changes to the thirdweb SDK

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants