Skip to content

Fix: useWalletBalance and the useActiveAccount to work with 0x{string} addresses instead of strings. #6852

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

0xDAEF0F
Copy link

@0xDAEF0F 0xDAEF0F commented Apr 25, 2025

  • Updated type annotations for wallet/account addresses from generic string types to the more specific 0x${string} type across several files.
  • This change addresses the issue where users would see a plain string instead of the expected 0x{string} type for addresses, improving type safety and developer experience.
  • Ensured that this fix applies to both useActiveAccount and useWalletBalance hooks, so address types are now consistent and accurate in these commonly used hooks.
  • Only performed a type coercion (using as 0x${string}) in a single location, specifically in the deprecated fromViemWalletClient function, to maintain backward compatibility without introducing unnecessary changes elsewhere.
  • No functional logic was altered; only type definitions were updated for clarity and correctness.

solves issue #6810


PR-Codex overview

This PR focuses on enhancing type safety by changing the type of various address properties across multiple files to a more specific format, ensuring they conform to the Ethereum address pattern.

Detailed summary

  • Changed address type from Address to 0x${string} in wallet.ts.
  • Cast viemAccount.address to 0x${string} in viem.ts.
  • Updated address type to 0x${string} | undefined in useWalletBalance.ts.
  • Modified optional address type to 0x${string} in TokenListScreen.tsx.
  • Changed walletAddress type from string to 0x${string} in index.ts.

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

`0x{string}` addresses instead of strings.
@0xDAEF0F 0xDAEF0F requested review from a team as code owners April 25, 2025 05:29
Copy link

changeset-bot bot commented Apr 25, 2025

⚠️ No Changeset found

Latest commit: dbb9bff

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

Copy link
Contributor

graphite-app bot commented Apr 25, 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.

Copy link

vercel bot commented Apr 25, 2025

@0xDAEF0F 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 Apr 25, 2025
Copy link

codecov bot commented Apr 25, 2025

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 35.52%. Comparing base (68aa693) to head (dbb9bff).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
packages/thirdweb/src/adapters/viem.ts 0.00% 1 Missing ⚠️

❌ Your patch check 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 (68aa693) and HEAD (dbb9bff). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (68aa693) HEAD (dbb9bff)
packages 2 1
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #6852       +/-   ##
===========================================
- Coverage   55.31%   35.52%   -19.79%     
===========================================
  Files         896      893        -3     
  Lines       57023    56891      -132     
  Branches     3971     2221     -1750     
===========================================
- Hits        31541    20212    -11329     
- Misses      25385    36608    +11223     
+ Partials       97       71       -26     
Flag Coverage Δ
packages 35.52% <0.00%> (-19.79%) ⬇️
Files with missing lines Coverage Δ
...eb/src/react/core/hooks/others/useWalletBalance.ts 53.84% <ø> (-17.95%) ⬇️
packages/thirdweb/src/wallets/engine/index.ts 12.90% <ø> (ø)
packages/thirdweb/src/adapters/viem.ts 50.64% <0.00%> (ø)

... and 281 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

jnsdls commented Apr 25, 2025

I feel like if we're going to do this we should do it... for all addresses RETURNED by the SDK across the entire codebase

I think there is no harm in RETURNING the more strict type everywhere, we obviously can't force existing users to PASS the more strict type everywhere because that would be a breaking change, but returning a stricter type should be non-breaking for everyone.

@joaquim-verges @MananTank @gregfromstl for your thoughts on this

@jnsdls
Copy link
Member

jnsdls commented Apr 25, 2025

also thanks @0xDAEF0F for kicking this off, we'll get to a resolution on the overall plan here from our side and we'll get this merged if it fits the direction we want to go down

@0xDAEF0F
Copy link
Author

0xDAEF0F commented Apr 26, 2025

I feel like if we're going to do this we should do it... for all addresses RETURNED by the SDK across the entire codebase

I think there is no harm in RETURNING the more strict type everywhere, we obviously can't force existing users to PASS the more strict type everywhere because that would be a breaking change, but returning a stricter type should be non-breaking for everyone.

@joaquim-verges @MananTank @gregfromstl for your thoughts on this

It’s technically a breaking change, but only in cases where the user wasn’t passing the address correctly to begin with. In that sense, it’s a good break; it surfaces a misuse that would have failed at runtime anyway.

EDIT:
On another note, the easiest and most gradual way to fix the manual override on addresses for abitype and unify everything into a single Address type, is:

  1. Remove the manual override (tsc will break everywhere with Type 'string' is not assignable to type '0x${string}).
  2. Programatically type coerce with as 0x{string}` everywhere so the code compiles.
  3. Grep every as 0x{string} and start fixing the whole codebase so every Address type is not a real string but a 0x{string}

I don't know if type safety is a priority right now (ts is not real type safety anyways 😜) and if it's a good thing to mess so much with the codebase everywhere.

Just trying to fix the GH issue.

Copy link
Member

jnsdls commented Apr 26, 2025

hm it's actually the other direction that this becomes doable without breaking every user:

  1. we can remove the ABIType override
  2. everywhere (across the entire codebase) where an address is an INPUT param to any function we need to make sure it is explicitly typed as string and not as Address (we do NOT want to break inputs without a major version bump)
  3. everywhere where an address is an OUTPUT from any function it should be typed as Address (0x${string})

returning a more strict type is completely non breaking because 0x${string} is a subset of string (which is what we are returning today), meaning anything that relies on it being a string in a user's codebase will still be happy to receive 0x${string}.

Happy for you to take a stab at this @0xDAEF0F - however this is definitely a big chunk of the codebase (even though only tiny individual parts) that will need to change.

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