Skip to content

Conversation

@gregdhill
Copy link
Contributor

@gregdhill gregdhill commented Sep 11, 2025

Summary by CodeRabbit

  • New Features

    • Adds preflight BTC balance check for onramps.
    • Adds detailed ETH gas budgeting for offramp (approval vs order creation), enforces sufficient ETH before proceeding, and performs ERC20 approval only when required.
    • Validates token decimals minimum and improves gas/balance error messages (values shown in wei/satoshis).
  • Tests

    • Expands gateway tests to mock balance, gas estimation, and fee APIs to cover new preflight flows.
  • Chores

    • Package version bumped (4.2.9).

Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
@vercel
Copy link

vercel bot commented Sep 11, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
bob-docs Ready Ready Preview Comment Sep 16, 2025 0:50am

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 11, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds BTC and ETH preflight balance checks and gas budgeting to GatewayApiClient.executeQuote: verifies onramp BTC via Esplora, checks ETH balance and estimates gas/fees for ERC20 approve and createOrder on the offramp path, computes total gas cost, conditionally submits approve, and enforces sufficiency before proceeding.

Changes

Cohort / File(s) Summary
Gateway client preflight & conditional approval
sdk/src/gateway/client.ts
- Added Esplora BTC balance check for onramp
- Fetched ETH balance and fee components for offramp (getBalance, estimateFeesPerGas / getGasPrice)
- Estimated gas for ERC20 approve and offramp createOrder, computed approvalGasCost & createOrderGasCost
- Replaced simple allowance check with multicall for allowance + decimals, validated decimals >= 8, used BigInt scaling to compute required amount and needsApproval
- Computed totalGasCost and enforced ethBalance >= totalGasCost, throwing detailed wei-based errors when insufficient
- Conditionally simulated/submitted ERC20 approve when needed, then simulated and submitted createOrder, preserving receipt waits
Test publicClient mocks updated
sdk/test/gateway.test.ts
- Extended mockPublicClient with getBalance, estimateContractGas, estimateFeesPerGas, and getGasPrice to exercise new balance and gas/fee estimation paths in tests
Package version bump
sdk/package.json
- Bumped package version from 4.2.8 to 4.2.9

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant App as App
  participant GAC as GatewayApiClient
  participant Esplora as Esplora
  participant RPC as EVM Node
  participant ERC20 as ERC20 Token
  participant Offramp as Offramp Contract

  Note over GAC: Preflight (BTC/ETH balance + gas budgeting)

  App->>GAC: executeQuote(quote)
  alt Onramp (BTC)
    GAC->>Esplora: getBalance(fromUserAddress)
    Esplora-->>GAC: btcBalance
    alt Insufficient BTC
      GAC-->>App: throw Error("insufficient BTC ...")
    end
  else Offramp (ETH/ERC20)
    GAC->>RPC: getBalance(accountAddress)
    GAC->>RPC: estimateFeesPerGas()/getGasPrice()
    GAC->>ERC20: multicall(allowance, decimals)
    GAC->>RPC: estimateContractGas.approve(...)
    GAC->>RPC: estimateContractGas.createOrder(...)
    GAC->>GAC: compute requiredAmount, needsApproval, totalGasCost
    alt Insufficient ETH
      GAC-->>App: throw Error("insufficient ETH for gas ...")
    else
      opt needsApproval
        GAC->>ERC20: sendTransaction approve(...)
        ERC20-->>GAC: txReceipt
      end
      GAC->>Offramp: simulate createOrder(...)
      Offramp-->>GAC: simulationResult
      GAC->>Offramp: sendTransaction createOrder(...)
      Offramp-->>GAC: txReceipt
      GAC-->>App: result (receipt)
    end
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • nakul1010
  • slavastartsev
  • danielsimao

Poem

A rabbit counts satoshis and wei with glee,
Paws tap the gas estimates carefully.
Approve hops only when coins fall short,
Then createOrder finishes the court.
Receipt arrives — carrot party! 🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "fix: check eth estimate in offramp" is a short, single-sentence summary that accurately captures the primary change in the diff — adding ETH balance and gas estimation/preflight checks to the offramp flow — and is specific and actionable without extraneous noise. It maps directly to the changes described in the raw summary (gas budgeting, allowance logic, and throwing when ETH is insufficient).
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/eth-check

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 306f7dd and 6669f45.

📒 Files selected for processing (2)
  • sdk/package.json (2 hunks)
  • sdk/src/gateway/client.ts (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • sdk/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • sdk/src/gateway/client.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: test

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Summary of Changes

Hello @gregdhill, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a crucial improvement to the offramp transaction process by proactively checking for sufficient ETH balance to cover gas fees. It ensures that users have enough funds before attempting transactions, thereby reducing failed transactions and improving the overall user experience.

Highlights

  • Gas Fee Pre-check for Offramp Transactions: Implemented a pre-check mechanism within the offramp transaction flow to estimate and verify sufficient ETH balance for gas fees, preventing transaction failures due to low funds.
  • Dynamic Gas Cost Calculation: Introduced logic to dynamically calculate the total estimated gas cost, accounting for both ERC20 approve and offramp createOrder transactions, based on whether an approval is required.
  • Insufficient Balance Error Handling: Added an explicit error throw if the user's ETH balance is less than the calculated total gas cost, providing clear feedback on insufficient funds.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a helpful pre-flight check in the offramp process to ensure the user has sufficient ETH for gas fees. It estimates the gas for both the createOrder and a potential approve transaction and throws an error if the user's balance is too low. This is a great improvement for user experience, preventing failed transactions. My review includes a suggestion to improve the accuracy of the gas fee estimation by using modern EIP-1559 methods, and another to optimize the logic to avoid an unnecessary network call.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
sdk/src/gateway/client.ts (2)

855-861: Propagate safe account to simulate approve

If walletClient.account is not set, simulateContract should still carry an account. Use the same fallback as above.

-                const { request } = await publicClient.simulateContract({
-                    account: walletClient.account,
+                const { request } = await publicClient.simulateContract({
+                    account: walletClient.account ?? accountAddress,
                     address: tokenAddress,
                     abi: erc20Abi,
                     functionName: 'approve',
                     args: [offrampRegistryAddress, maxUint256],
                 });

868-874: Propagate safe account to simulate createOrder

Same reasoning as above; keep the execution path consistent when no default account is configured.

-            const { request } = await publicClient.simulateContract({
-                account: walletClient.account,
+            const { request } = await publicClient.simulateContract({
+                account: walletClient.account ?? accountAddress,
                 address: offrampRegistryAddress,
                 abi: offrampOrder.offrampABI,
                 functionName: offrampOrder.offrampFunctionName,
                 args: offrampOrder.offrampArgs,
             });
🧹 Nitpick comments (3)
sdk/src/gateway/client.ts (3)

804-819: Estimate approval gas only if needed; trim RPC calls and reduce failure surface

You eagerly estimate gas for approve even when allowance is already sufficient. Compute needsApproval first (multicall), then:

  • always estimate createOrder gas,
  • conditionally estimate approve gas only if needsApproval is true,
  • sum the costs.

This cuts one RPC on the common “already approved” path and avoids estimating an approve some tokens might reject.

Also applies to: 845-847


848-852: Polish error message: show ETH values instead of raw wei

User-facing error in wei is hard to read. Prefer formatting both values in ETH.

Example change (requires formatEther from viem):

-                throw new Error(
-                    `Insufficient ETH balance for gas fees. Required: ${totalGasCost} wei, Available: ${ethBalance} wei`
-                );
+                throw new Error(
+                    `Insufficient ETH for gas. Required: ${formatEther(totalGasCost)} ETH, available: ${formatEther(ethBalance)} ETH.`
+                );

Outside this hunk, add:

-import {
+import {
   Account,
   Address,
   erc20Abi,
   isAddress,
   PublicClient,
   Transport,
   Chain as ViemChain,
   WalletClient,
   zeroAddress,
   maxUint256,
   Hash,
   Hex,
+  formatEther,
 } from 'viem';

854-866: Handle non-standard ERC-20s that forbid “non-zero → non-zero” approvals

Some tokens require resetting allowance to 0 before setting a new value. Consider a guarded fallback to approve(0) then approve(max) when the first approve reverts.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0ca91dc and b08d41b.

📒 Files selected for processing (1)
  • sdk/src/gateway/client.ts (2 hunks)
🧰 Additional context used
🪛 GitHub Check: Tests
sdk/src/gateway/client.ts

[failure] 800-800: test/gateway.test.ts > Gateway Tests > should return evm txid for offramp
TypeError: Cannot read properties of undefined (reading 'address')
❯ GatewayApiClient.executeQuote src/gateway/client.ts:800:47
❯ test/gateway.test.ts:742:25

🪛 GitHub Actions: SDK
sdk/src/gateway/client.ts

[error] 800-800: Cannot read properties of undefined (reading 'address') in GatewayApiClient.executeQuote when accessing walletClient.account.address. (src/gateway/client.ts:800)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: test

Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (3)
sdk/src/gateway/client.ts (3)

816-831: Optimize gas estimation to avoid unnecessary RPC calls

You're always estimating gas for the approval transaction even when it might not be needed. Check the allowance first, then only estimate approval gas if required.

Apply this optimization:

-// Estimate gas for both potential transactions
-const [approvalGasEstimate, createOrderGasEstimate] = await Promise.all([
-    publicClient.estimateContractGas({
-        address: tokenAddress,
-        abi: erc20Abi,
-        functionName: 'approve',
-        args: [offrampRegistryAddress, maxUint256],
-        account: walletClient.account,
-    }),
-    publicClient.estimateContractGas({
-        address: offrampRegistryAddress,
-        abi: offrampOrder.offrampABI,
-        functionName: offrampOrder.offrampFunctionName,
-        args: offrampOrder.offrampArgs,
-        account: walletClient.account,
-    }),
-]);
+// First check if approval is needed
+const [allowance, decimals] = await publicClient.multicall({
+    allowFailure: false,
+    contracts: [
+        {
+            address: tokenAddress,
+            abi: erc20Abi,
+            functionName: 'allowance',
+            args: [accountAddress, offrampRegistryAddress],
+        },
+        {
+            address: tokenAddress,
+            abi: erc20Abi,
+            functionName: 'decimals',
+        },
+    ],
+});
+
+if (decimals < 8) {
+    throw new Error('Tokens with less than 8 decimals are not supported');
+}
+const multiplier = 10n ** BigInt(decimals - 8);
+const requiredAmount = BigInt(quote.amountLockInSat) * multiplier;
+const needsApproval = requiredAmount > allowance;
+
+// Only estimate approval gas if needed
+const approvalGasEstimate = needsApproval
+    ? await publicClient.estimateContractGas({
+          address: tokenAddress,
+          abi: erc20Abi,
+          functionName: 'approve',
+          args: [offrampRegistryAddress, maxUint256],
+          account: walletClient.account,
+      })
+    : 0n;
+
+const createOrderGasEstimate = await publicClient.estimateContractGas({
+    address: offrampRegistryAddress,
+    abi: offrampOrder.offrampABI,
+    functionName: offrampOrder.offrampFunctionName,
+    args: offrampOrder.offrampArgs,
+    account: walletClient.account,
+});

Then move the multicall and decimals check logic before gas estimation, and remove the duplicate code from lines 839-862.


833-837: Consider EIP-1559 priority for better UX

Good use of estimateFeesPerGas() with fallback to getGasPrice() for accurate fee estimation on EIP-1559 compatible chains.


856-861: LGTM! Proper BigInt arithmetic for decimal conversion

The implementation correctly validates decimals >= 8 and uses BigInt-only arithmetic to avoid precision loss.

🧹 Nitpick comments (1)
sdk/src/gateway/client.ts (1)

866-870: Consider more user-friendly units for error message

The gas cost error message displays values in wei, which can be hard to interpret. Consider showing the amounts in ETH for better user experience.

Apply this enhancement:

 if (ethBalance < totalGasCost) {
+    const formatEth = (wei: bigint) => {
+        const eth = Number(wei) / 1e18;
+        return eth.toFixed(6);
+    };
     throw new Error(
-        `Insufficient ETH balance for gas fees. Required: ${totalGasCost} wei, Available: ${ethBalance} wei`
+        `Insufficient ETH balance for gas fees. Required: ${formatEth(totalGasCost)} ETH, Available: ${formatEth(ethBalance)} ETH`
     );
 }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b08d41b and a9d9b8e.

📒 Files selected for processing (1)
  • sdk/src/gateway/client.ts (3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-17T11:18:53.195Z
Learnt from: slavastartsev
PR: bob-collective/bob#634
File: sdk/src/gateway/client.ts:136-140
Timestamp: 2025-06-17T11:18:53.195Z
Learning: The codebase has a requirement not to introduce discriminated unions in the Gateway SDK client methods, including the getQuote method return types in sdk/src/gateway/client.ts.

Applied to files:

  • sdk/src/gateway/client.ts
🧬 Code graph analysis (1)
sdk/src/gateway/client.ts (2)
sdk/src/esplora.ts (1)
  • EsploraClient (154-517)
sdk/src/gateway/utils.ts (1)
  • formatBtc (108-110)
🪛 GitHub Check: Tests
sdk/src/gateway/client.ts

[failure] 763-763: test/gateway.test.ts > Gateway Tests > should return btc txid for onramp
Error: Insufficient BTC balance in address bc1qafk4yhqvj4wep57m62dgrmutldusqde8adh20d. Required: 0.00001
❯ GatewayApiClient.executeQuote src/gateway/client.ts:763:23
❯ test/gateway.test.ts:680:25


[failure] 811-811: test/gateway.test.ts > Gateway Tests > should return evm txid for offramp
TypeError: publicClient.getBalance is not a function
❯ GatewayApiClient.executeQuote src/gateway/client.ts:811:51
❯ test/gateway.test.ts:742:25

🪛 GitHub Actions: SDK
sdk/src/gateway/client.ts

[error] 763-763: Insufficient BTC balance in address bc1qafk4yhqvj4wep57m62dgrmutldusqde8adh20d. Required: 0.00001


[error] 811-811: TypeError: publicClient.getBalance is not a function

🔇 Additional comments (2)
sdk/src/gateway/client.ts (2)

808-813: LGTM! Good fix for handling undefined wallet account

The implementation correctly handles the case where walletClient.account may be undefined by falling back to params.fromUserAddress. This prevents the crash reported in tests.


810-870: Mock publicClient.getBalance (and related RPC calls) in tests

CI failures show publicClient.getBalance is not a function in the test environment. Repository search returned no test files (test/gateway.test.ts and test/ directory not found), so I couldn't confirm mock locations. Add/extend your test setup mocks to provide the chain RPC methods used in this snippet:

  • getBalance
  • estimateContractGas
  • estimateFeesPerGas (or provide maxFeePerGas in the mocked response)
  • getGasPrice
  • multicall (returning allowance and decimals)

After adding those mocks, rerun CI or provide the test path for re-check.

Comment on lines +758 to +759
const esploraClient = new EsploraClient(this.chain.id === bob.id ? Network.mainnet : Network.signet);

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix incorrect network parameter in EsploraClient constructor

The EsploraClient constructor expects a string literal ('mainnet' | 'signet' | 'testnet' | 'regtest'), but you're passing Network.mainnet and Network.signet which are enums from bitcoin-address-validation. This type mismatch will cause runtime errors.

Apply this fix:

-const esploraClient = new EsploraClient(this.chain.id === bob.id ? Network.mainnet : Network.signet);
+const esploraClient = new EsploraClient(this.chain.id === bob.id ? 'mainnet' : 'signet');
📝 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.

Suggested change
const esploraClient = new EsploraClient(this.chain.id === bob.id ? Network.mainnet : Network.signet);
const esploraClient = new EsploraClient(this.chain.id === bob.id ? 'mainnet' : 'signet');
🤖 Prompt for AI Agents
In sdk/src/gateway/client.ts around lines 758-759, the EsploraClient is being
constructed with Network.mainnet/Network.signet (enum values) but the
constructor expects string literals ('mainnet'|'signet'|'testnet'|'regtest');
replace the enum usage with the corresponding string literals (e.g. use
this.chain.id === bob.id ? 'mainnet' : 'signet') so the correct string is passed
to EsploraClient, and update any type annotations if necessary to reflect the
string union.

Comment on lines 761 to 766
const availableBtcBalance = await esploraClient.getBalance(quote.bitcoinAddress);
if (availableBtcBalance.total < BigInt(quote.satoshis)) {
throw new Error(
`Insufficient BTC balance in address ${quote.bitcoinAddress}. Required: ${formatBtc(BigInt(quote.satoshis))}`
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use confirmed balance for preflight check instead of total

The BTC balance check uses availableBtcBalance.total which includes unconfirmed mempool transactions. This could lead to issues if those transactions are replaced, dropped, or double-spent. Consider using only the confirmed balance for more reliable validation.

Apply this fix:

-if (availableBtcBalance.total < BigInt(quote.satoshis)) {
+if (availableBtcBalance.confirmed < quote.satoshis) {
     throw new Error(
-        `Insufficient BTC balance in address ${quote.bitcoinAddress}. Required: ${formatBtc(BigInt(quote.satoshis))}`
+        `Insufficient confirmed BTC balance in address ${quote.bitcoinAddress}. Required: ${formatBtc(BigInt(quote.satoshis))} BTC, Available: ${formatBtc(BigInt(availableBtcBalance.confirmed))} BTC`
     );
 }

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 GitHub Check: Tests

[failure] 763-763: test/gateway.test.ts > Gateway Tests > should return btc txid for onramp
Error: Insufficient BTC balance in address bc1qafk4yhqvj4wep57m62dgrmutldusqde8adh20d. Required: 0.00001
❯ GatewayApiClient.executeQuote src/gateway/client.ts:763:23
❯ test/gateway.test.ts:680:25

🪛 GitHub Actions: SDK

[error] 763-763: Insufficient BTC balance in address bc1qafk4yhqvj4wep57m62dgrmutldusqde8adh20d. Required: 0.00001

🤖 Prompt for AI Agents
In sdk/src/gateway/client.ts around lines 761 to 766, the preflight BTC balance
check uses availableBtcBalance.total (which includes unconfirmed mempool funds);
change it to use only availableBtcBalance.confirmed for the comparison so
unconfirmed inputs aren't relied upon. Replace the comparison to check if
availableBtcBalance.confirmed < BigInt(quote.satoshis) and keep the same error
message/formatBtc usage, ensuring types remain consistent (BigInt) when
comparing.

Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
sdk/src/gateway/client.ts (3)

840-855: Allowance owner must match signer; and compute needsApproval before estimating approve gas

Owner should be accountAddress (not params.fromUserAddress) to avoid false allowance decisions when the wallet’s default account differs from the passed param. Then gate approval gas estimation by needsApproval.

Apply:

-            const [allowance, decimals] = await publicClient.multicall({
+            const [allowance, decimals] = await publicClient.multicall({
                 allowFailure: false,
                 contracts: [
                     {
                         address: tokenAddress,
                         abi: erc20Abi,
                         functionName: 'allowance',
-                        args: [params.fromUserAddress as Address, offrampRegistryAddress],
+                        args: [accountAddress, offrampRegistryAddress],
                     },
                     {
                         address: tokenAddress,
                         abi: erc20Abi,
                         functionName: 'decimals',
                     },
                 ],
             });
@@
-            const multiplier = 10n ** BigInt(decimals - 8);
-            const requiredAmount = BigInt(quote.amountLockInSat) * multiplier;
-            const needsApproval = requiredAmount > allowance;
+            const multiplier = 10n ** BigInt(decimals - 8);
+            const requiredAmount = BigInt(quote.amountLockInSat) * multiplier;
+            const needsApproval = requiredAmount > allowance;
@@
-            // Calculate total gas cost needed
-            const totalGasCost = needsApproval ? approvalGasCost + createOrderGasCost : createOrderGasCost;
+            // Calculate total gas cost (estimate approve only if needed)
+            let approvalGasCost = 0n;
+            if (needsApproval) {
+                const approvalGasEstimate = await publicClient.estimateContractGas({
+                    address: tokenAddress,
+                    abi: erc20Abi,
+                    functionName: 'approve',
+                    args: [offrampRegistryAddress, maxUint256],
+                    account: accountAddress,
+                });
+                approvalGasCost = approvalGasEstimate * fee;
+            }
+            const totalGasCost = createOrderGasCost + approvalGasCost;

873-885: Use accountAddress for approve simulation

Avoid passing a possibly undefined walletClient.account.

Apply:

-                const { request } = await publicClient.simulateContract({
-                    account: walletClient.account,
+                const { request } = await publicClient.simulateContract({
+                    account: accountAddress,
                     address: tokenAddress,
                     abi: erc20Abi,
                     functionName: 'approve',
                     args: [offrampRegistryAddress, maxUint256],
                 });

887-893: Use accountAddress for createOrder simulation

Same rationale as above.

Apply:

-            const { request } = await publicClient.simulateContract({
-                account: walletClient.account,
+            const { request } = await publicClient.simulateContract({
+                account: accountAddress,
                 address: offrampRegistryAddress,
                 abi: offrampOrder.offrampABI,
                 functionName: offrampOrder.offrampFunctionName,
                 args: offrampOrder.offrampArgs,
             });
♻️ Duplicate comments (2)
sdk/src/gateway/client.ts (2)

816-833: Use accountAddress in estimates and avoid estimating approve gas unless needed

  • Pass account: accountAddress (current code may be undefined).
  • Small perf: estimate approve gas only if allowance is insufficient.

Apply:

-            // Estimate gas for both potential transactions
-            const [approvalGasEstimate, createOrderGasEstimate] = await Promise.all([
-                publicClient.estimateContractGas({
-                    address: tokenAddress,
-                    abi: erc20Abi,
-                    functionName: 'approve',
-                    args: [offrampRegistryAddress, maxUint256],
-                    account: walletClient.account,
-                }),
-                publicClient.estimateContractGas({
-                    address: offrampRegistryAddress,
-                    abi: offrampOrder.offrampABI,
-                    functionName: offrampOrder.offrampFunctionName,
-                    args: offrampOrder.offrampArgs,
-                    account: walletClient.account,
-                }),
-            ]);
+            // Estimate createOrder gas first (always needed)
+            const createOrderGasEstimate = await publicClient.estimateContractGas({
+                address: offrampRegistryAddress,
+                abi: offrampOrder.offrampABI,
+                functionName: offrampOrder.offrampFunctionName,
+                args: offrampOrder.offrampArgs,
+                account: accountAddress,
+            });

Additional changes below compute needsApproval first, then conditionally estimate approval gas. See follow‑up diffs in lines 840‑866 and 873‑893.


758-767: Fix Esplora network arg, use confirmed balance, correct address in error, and drop console.log

  • Pass string literals to EsploraClient (not the Network enum).
  • Use confirmed BTC only for preflight.
  • Error message should reference the user’s address, not the gateway deposit address.
  • Remove noisy console.log in SDK code.

Apply:

-            const esploraClient = new EsploraClient(this.chain.id === bob.id ? Network.mainnet : Network.signet);
+            const esploraClient = new EsploraClient(this.chain.id === bob.id ? 'mainnet' : 'signet');
@@
-            const availableBtcBalance = await esploraClient.getBalance(params.fromUserAddress!);
-            console.log(availableBtcBalance);
-            if (availableBtcBalance.total < BigInt(quote.satoshis)) {
-                throw new Error(
-                    `Insufficient BTC balance in address ${quote.bitcoinAddress}. Required: ${formatBtc(BigInt(quote.satoshis))}, Got: ${formatBtc(BigInt(availableBtcBalance.total))}`
-                );
-            }
+            const availableBtcBalance = await esploraClient.getBalance(params.fromUserAddress!);
+            const confirmed = BigInt(availableBtcBalance.confirmed);
+            if (confirmed < BigInt(quote.satoshis)) {
+                throw new Error(
+                    `Insufficient confirmed BTC balance in address ${params.fromUserAddress}. Required: ${formatBtc(BigInt(quote.satoshis))} BTC, Available: ${formatBtc(confirmed)} BTC`
+                );
+            }

Note: apply the same EsploraClient arg fix in finalizeOnrampOrder (Line 963).

🧹 Nitpick comments (1)
sdk/test/gateway.test.ts (1)

713-721: Mocks align with EIP‑1559 fee path; good coverage

Adding getBalance, estimateContractGas, estimateFeesPerGas, and getGasPrice stubs unblocks the new gas budgeting flow.

To de‑flake the onramp test (preflight BTC balance), consider stubbing Esplora instead of hitting the network:

vi.spyOn((await import('../src/esplora')).EsploraClient.prototype, 'getBalance')
  .mockResolvedValue({ confirmed: 2_000_000, unconfirmed: 0, total: 2_000_000 });
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a9d9b8e and 306f7dd.

📒 Files selected for processing (2)
  • sdk/src/gateway/client.ts (3 hunks)
  • sdk/test/gateway.test.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
sdk/src/gateway/client.ts (2)
sdk/src/esplora.ts (1)
  • EsploraClient (154-517)
sdk/src/gateway/utils.ts (1)
  • formatBtc (108-110)
🔇 Additional comments (1)
sdk/src/gateway/client.ts (1)

834-839: LGTM on EIP‑1559 fee selection

Using estimateFeesPerGas().maxFeePerGas ?? getGasPrice() is the right fallback chain.

Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
@gregdhill gregdhill merged commit c16c5ce into master Sep 16, 2025
7 of 8 checks passed
@gregdhill gregdhill deleted the fix/eth-check branch September 16, 2025 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants