Skip to content

[SDK] Fix: Hedera toWei Conversion #5466

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[SDK] Fix: Hedera toWei Conversion #5466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/bright-mayflies-think.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"thirdweb": patch
---

Account for Hedera wei decimals difference
2 changes: 1 addition & 1 deletion packages/thirdweb/src/contract/deployment/zksync/zkDeployCreate2Factory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export async function zkDeployCreate2Factory(
privateKey: PUBLISHED_PRIVATE_KEY,
});

const valueToSend = toWei("0.01");
const valueToSend = toWei("0.01", options.chain);
const balance = await getWalletBalance({
address: create2Signer.address,
chain: options.chain,
Expand Down
4 changes: 3 additions & 1 deletion packages/thirdweb/src/extensions/erc20/write/deposit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ export type DepositParams =
*/
export function deposit(options: BaseTransactionOptions<DepositParams>) {
const value =
"amountWei" in options ? options.amountWei : toWei(options.amount);
"amountWei" in options
? options.amountWei
: toWei(options.amount, options.contract.chain);
return prepareContractCall({
contract: options.contract,
method: [FN_SELECTOR, [], []] as const,
Expand Down
2 changes: 1 addition & 1 deletion packages/thirdweb/src/react/core/hooks/wallets/useSendToken.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ export function useSendToken(client: ThirdwebClient) {
chain: activeChain,
client,
to,
value: toWei(amount),
value: toWei(amount, activeChain),
});

await sendTransaction({
Expand Down
2 changes: 1 addition & 1 deletion ...s/thirdweb/src/react/web/ui/ConnectWallet/screens/Buy/swap/TransferConfirmationScreen.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ export function TransferConfirmationScreen(
client,
chain,
to: receiverAddress,
value: toWei(tokenAmount),
value: toWei(tokenAmount, chain),
})
: transfer({
contract: getContract({
Expand Down
18 changes: 16 additions & 2 deletions packages/thirdweb/src/utils/units.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import type { Chain } from "../chains/types.js";

/**
* Converts a given number of units to a string representation with a specified number of decimal places.
* @param units - The number of units to convert.
Expand Down Expand Up @@ -125,10 +127,22 @@ export function toUnits(tokens: string, decimals: number): bigint {
* toWei('1')
* // 1000000000000000000n
* ```
*
* For chains with varying decimals, you can provide the chain.
* ```ts
* import { toWei } from "thirdweb/utils";
* toWei('1', defineChain(295))
* // 10000000n
* ```
* @utils
*/
export function toWei(tokens: string) {
return toUnits(tokens, 18);
export function toWei(tokens: string, chain?: Chain) {
Copy link
Member

@joaquim-verges joaquim-verges Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@gregfromstl gregfromstl Nov 21, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

@joaquim-verges joaquim-verges Nov 21, 2024

Choose a reason for hiding this comment

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

Yeah but I assume we put this decimals value in the chains db right?

Copy link
Contributor Author

@gregfromstl gregfromstl Nov 21, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

@joaquim-verges joaquim-verges Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@gregfromstl gregfromstl Nov 26, 2024

Choose a reason for hiding this comment

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

I agree, I hate it but not sure of any other solution

switch (chain?.id) {
case 295:
return toUnits(tokens, 8);
default:
return toUnits(tokens, 18);
}
}

/**
Expand Down
Loading