[SDK] Fix: Hedera toWei Conversion #5466
Conversation
gregfromstl
added
the
Bug
gregfromstl
requested review from
MananTank,
edwardysun,
jnsdls and
joaquim-verges
as code owners
🦋 Changeset detectedLatest commit: 8110d8c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
size-limit report 📦
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5466 +/- ##
==========================================
+ Coverage 43.81% 43.89% +0.07%
==========================================
Files 1069 1075 +6
Lines 55726 55951 +225
Branches 3882 3911 +29
==========================================
+ Hits 24415 24557 +142
- Misses 30628 30711 +83
Partials 683 683
*This pull request uses carry forward flags. Click here to find out more.
|
| */ | ||
| export function toWei(tokens: string) { | ||
| return toUnits(tokens, 18); | ||
| export function toWei(tokens: string, chain?: Chain) { |
There was a problem hiding this comment.
wonder if a better call would be to get the chain metadata async in the places where we do toWei above, and use toUnits with the chain native currency decimals?
There was a problem hiding this comment.
It's not a native currency decimals thing, it's the literal wei on the chain. Peep the slack thread linked in the PR description
There was a problem hiding this comment.
Yeah but I assume we put this decimals value in the chains db right?
There was a problem hiding this comment.
We don't as of right now because (based on my understanding) it's something EVM chains would normally never deviate from. We have the native currency decimals, but that's different. It's an extreme edge case as Firekeeper said
There was a problem hiding this comment.
gotcha, its just a bit weird to me to change the public API of toWei for this. as a user i would be confused why i would pass a chain there
There was a problem hiding this comment.
I agree, I hate it but not sure of any other solution
gregfromstl
added
the
merge-queue
Merge activity
|
[Relevant Slack discussion](https://thirdwebdev.slack.com/archives/C04P6J3K0ES/p1731616237108399?thread_ts=1730826933.868789&cid=C04P6J3K0ES) CNCT-2368 <!-- start pr-codex --> --- ## PR-Codex overview This PR focuses on adjusting the `toWei` function to accommodate different decimal values for various chains, particularly for Hedera, improving token handling in the `thirdweb` library. ### Detailed summary - Updated `toWei` function to accept a `chain` parameter for variable decimal handling. - Modified `useSendToken.ts` to pass `activeChain` to `toWei`. - Changed `TransferConfirmationScreen.tsx` to use `chain` for `toWei`. - Adjusted `zkDeployCreate2Factory.ts` to use `options.chain` in `toWei`. - Updated `deposit.ts` to consider `options.contract.chain` in `toWei`. > ✨ Ask PR-Codex anything about this PR by commenting with `/codex {your question}` <!-- end pr-codex -->
gregfromstl
force-pushed
the
greg/cnct-2368-fix-hedera-network-claim
branch
from
5f0a093 to
8110d8c
Compare
graphite-app
bot
removed
the
merge-queue
|
Closing because I don't like this |
Relevant Slack discussion
CNCT-2368
PR-Codex overview
This PR focuses on adjusting the
toWeifunction to account for different decimal configurations across chains, specifically for Hedera and zkSync, enhancing token handling in various components.Detailed summary
toWeifunction to accept achainparameter for variable decimals.useSendToken.tsto usetoWei(amount, activeChain).TransferConfirmationScreen.tsxto usetoWei(tokenAmount, chain).zkDeployCreate2Factory.tsto usetoWei("0.01", options.chain).deposit.tsto handleoptions.amountwith respect tooptions.contract.chain.