Fix: Throw error if amount and gas is more than balance #7358
Conversation
🦋 Changeset detectedLatest commit: b6ba5bb The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
|
""" WalkthroughThe changes introduce pre-send validation in the token transfer logic to ensure the sender's balance covers both the transfer amount and estimated gas fees. The transaction confirmation waiting step now includes a maximum block wait time. Additionally, a redundant type alias was removed, and a function's return type was updated for clarity. A test suite for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant useSendToken
participant getWalletBalance
participant estimateGas
participant sendTransaction
User->>useSendToken: Initiate token transfer
useSendToken->>getWalletBalance: Fetch wallet balance
useSendToken->>estimateGas: Estimate gas for transfer
useSendToken->>useSendToken: Check if (amount + gas) > balance
alt Insufficient balance
useSendToken-->>User: Throw insufficient balance error
else Sufficient balance
useSendToken->>sendTransaction: Send transaction
sendTransaction-->>useSendToken: Return transaction
useSendToken->>useSendToken: Wait for receipt (maxBlocksWaitTime=10,000)
useSendToken-->>User: Return/send receipt
end
Suggested reviewers
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
💤 Files with no reviewable changes (4)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
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. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/thirdweb/src/react/core/hooks/wallets/useSendToken.ts (2)
103-107: Redundantawaitbeforereturn
return await sendTransaction(…)adds no value here (the surroundingasyncalready unwraps the
promise) and slightly obscures stack traces.-return await sendTransaction({ +return sendTransaction({ transaction: sendNativeTokenTx, account, });
122-125: Same redundantawaitas aboveConsider dropping the extra
awaitfor consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/thirdweb/src/react/core/hooks/wallets/useSendToken.ts(4 hunks)packages/thirdweb/src/wallets/utils/getWalletBalance.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/thirdweb/src/wallets/utils/getWalletBalance.ts (1)
packages/thirdweb/src/extensions/erc20/read/getBalance.ts (1)
GetBalanceResult(20-28)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Size
- GitHub Check: Unit Tests
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Build Packages
- GitHub Check: Lint Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/thirdweb/src/wallets/utils/getWalletBalance.ts (1)
42-42: Confirm all call-sites were migrated after the return-type rename
getWalletBalancenow returnsPromise<GetBalanceResult>instead of the removedGetWalletBalanceResult.
Search the codebase for any residual references to the old alias to avoid compile/runtime errors.#!/bin/bash # Locate any lingering usages of the old type alias. rg --type ts 'GetWalletBalanceResult'packages/thirdweb/src/react/core/hooks/wallets/useSendToken.ts (1)
137-138:maxBlocksWaitTimeof 10 000 ~= 33 h on EthereumIs such a long upper bound intentional? A smaller value (e.g. a few hundred blocks) usually suffices
and avoids keeping the UI in a pending state for a day.
| const gasEstimate = await estimateGas({ | ||
| transaction: sendNativeTokenTx, | ||
| account, | ||
| }); | ||
| const balance = await getWalletBalance({ | ||
| address: account.address, | ||
| chain: activeChain, | ||
| client, | ||
| }); | ||
| if (toWei(amount) + gasEstimate > balance.value) { | ||
| throw new Error("Insufficient balance for transfer amount and gas"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Gas cost comparison is incorrect – estimateGas returns units, not wei
estimateGas (EVM eth_estimateGas) returns the gas LIMIT (≈ 21 000) – not the cost in wei.
Adding raw gas units to amountWei therefore under-estimates the required balance by
≈9 orders of magnitude and will allow transactions that still revert for “insufficient funds”.
At minimum multiply the gas units by an appropriate (max) fee per gas, or use a helper that already returns the total fee.
-const gasEstimate = await estimateGas({ transaction: sendNativeTokenTx, account });
-…
-if (toWei(amount) + gasEstimate > balance.value) {
+const gasUnits = await estimateGas({ transaction: sendNativeTokenTx, account });
+const { maxFeePerGas } = await getFeeData({ client, chain: activeChain }); // or gasPrice
+const gasCost = gasUnits * maxFeePerGas;
+const amountWei = toWei(amount);
+
+if (amountWei + gasCost >= balance.value) {
throw new Error("Insufficient balance for transfer amount and gas");
}Failing to fix this will surface as sporadic “intrinsic gas too low / insufficient funds” errors in production.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const gasEstimate = await estimateGas({ | |
| transaction: sendNativeTokenTx, | |
| account, | |
| }); | |
| const balance = await getWalletBalance({ | |
| address: account.address, | |
| chain: activeChain, | |
| client, | |
| }); | |
| if (toWei(amount) + gasEstimate > balance.value) { | |
| throw new Error("Insufficient balance for transfer amount and gas"); | |
| } | |
| const gasUnits = await estimateGas({ | |
| transaction: sendNativeTokenTx, | |
| account, | |
| }); | |
| const { maxFeePerGas } = await getFeeData({ | |
| client, | |
| chain: activeChain, | |
| }); | |
| const gasCost = gasUnits * maxFeePerGas; | |
| const amountWei = toWei(amount); | |
| const balance = await getWalletBalance({ | |
| address: account.address, | |
| chain: activeChain, | |
| client, | |
| }); | |
| if (amountWei + gasCost >= balance.value) { | |
| throw new Error("Insufficient balance for transfer amount and gas"); | |
| } |
🤖 Prompt for AI Agents
In packages/thirdweb/src/react/core/hooks/wallets/useSendToken.ts between lines
90 and 102, the code incorrectly adds the gas estimate (in gas units) directly
to the token amount in wei, underestimating the required balance. To fix this,
multiply the gas estimate by the current gas price or max fee per gas to convert
it to wei before adding it to the amount. This ensures the balance check
accounts for the actual gas cost in wei and prevents insufficient funds errors.
gregfromstl
force-pushed
the
greg/fix-trilitech-transfer
branch
from
d9fc667 to
7cb5e37
Compare
size-limit report 📦
|
gregfromstl
force-pushed
the
greg/fix-trilitech-transfer
branch
from
7cb5e37 to
4d025d4
Compare
gregfromstl
force-pushed
the
greg/fix-trilitech-transfer
branch
from
4d025d4 to
1e8cd18
Compare
gregfromstl
force-pushed
the
greg/fix-trilitech-transfer
branch
from
2cf3a47 to
0553bb9
Compare
gregfromstl
force-pushed
the
greg/fix-trilitech-transfer
branch
from
0553bb9 to
7b5b7ea
Compare
gregfromstl
force-pushed
the
greg/fix-trilitech-transfer
branch
from
7b5b7ea to
b6ba5bb
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7358 +/- ##
==========================================
- Coverage 55.58% 52.35% -3.23%
==========================================
Files 909 939 +30
Lines 58683 63160 +4477
Branches 4163 4215 +52
==========================================
+ Hits 32617 33070 +453
- Misses 25959 29983 +4024
Partials 107 107
🚀 New features to boost your workflow:
|
PR-Codex overview
This PR focuses on fixing issues related to etherlink transfers, improving balance checks before transactions, and cleaning up test cases in the
thirdwebcodebase.Detailed summary
useBridgeError.test.tsandusePaymentMethods.test.ts.Status.test.ts.getWalletBalancefunction.useSendTokento include gas estimate and balance checks.paymentMachine.test.ts, removing unnecessary test cases.Summary by CodeRabbit