Fix: Logic checks ethBalance < BigInt(fee) but ignores gas costs.#124
Open
aniket866 wants to merge 3 commits intoStabilityNexus:mainfrom
Open
Fix: Logic checks ethBalance < BigInt(fee) but ignores gas costs.#124aniket866 wants to merge 3 commits intoStabilityNexus:mainfrom
aniket866 wants to merge 3 commits intoStabilityNexus:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR adds comprehensive test coverage for batch invoice operations and fee withdrawal functionality while enhancing the frontend payment flow with gas cost estimation and ERC20 token balance validation before payment processing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
frontend/src/page/ReceivedInvoice.jsx (2)
428-428:⚠️ Potential issue | 🟡 MinorUse
toast.infoinstead oftoast.successfor an intermediate step
toast.successat line 428 implies the payment succeeded. At this point only the pre-check passed; the transaction hasn't been submitted. A subsequent approval or on-chain failure will still surface an error, creating a confusing "success → error" sequence for the user.✏️ Proposed fix
- toast.success("Balance check passed! Processing payment..."); + toast.info("Balance check passed. Processing payment...");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/page/ReceivedInvoice.jsx` at line 428, The toast shown after the balance pre-check in ReceivedInvoice.jsx currently uses toast.success and misleads users into thinking payment completed; change the call to toast.info (or another neutral status like toast.pending) in the function where toast.success("Balance check passed! Processing payment...") is invoked so it communicates an intermediate state (e.g., "Balance check passed — processing payment...") instead of a final success; update any related tests/messages expecting the success toast accordingly.
238-296:⚠️ Potential issue | 🟠 Major
checkBalanceuses a fixed single-invoice fee for batch payments — balance check will underestimate required ETH
BigInt(fee)(the component state, a single-invoice fee) is used at both line 259 (ETH path) and line 285 (ERC20 ETH path). However, the actual batch transaction at line 577 sendsfeePerInvoice * BigInt(invoiceIds.length). For a batch of N invoices, the check only requires1×feeworth of ETH, while the transaction requiresN×fee. The check can pass and the on-chain transaction will then fail for any batch with N > 1 when the user holds exactly the minimum checked amount.
checkBalancealso capturesfeevia closure over the component state instead of the locally-fetched fee frompayInvoice(line 421'sconst fee = await contract.fee()shadows the state but does not reach the closure). If the protocol fee changes between page load and payment, the pre-check and the actualvalue:argument diverge.🐛 Proposed fix: add an `invoiceCount` parameter and pass it from the batch caller
- const checkBalance = async (tokenAddress, amount, symbol, signer) => { + const checkBalance = async (tokenAddress, amount, symbol, signer, invoiceCount = 1) => { const userAddress = await signer.getAddress(); const provider = signer.provider; const feeData = await provider.getFeeData(); const gasPrice = feeData.gasPrice || feeData.maxFeePerGas; if (!gasPrice) { throw new Error("Unable to fetch gas price"); } const estimatedGasLimit = BigInt(300000); const estimatedGasCost = gasPrice * estimatedGasLimit; if (tokenAddress === ethers.ZeroAddress) { const balance = await provider.getBalance(userAddress); const invoiceAmount = ethers.parseUnits(amount.toString(), 18); const totalRequired = - invoiceAmount + BigInt(fee) + estimatedGasCost; + invoiceAmount + BigInt(fee) * BigInt(invoiceCount) + estimatedGasCost; // ... } else { // ... - const totalEthRequired = BigInt(fee) + estimatedGasCost; + const totalEthRequired = BigInt(fee) * BigInt(invoiceCount) + estimatedGasCost; // ... } };And in
handleBatchPayment, pass the count:await checkBalance( group.tokenAddress, group.totalAmount, group.symbol, signer, + group.invoices.length );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/page/ReceivedInvoice.jsx` around lines 238 - 296, The balance check under checkBalance currently uses the component state fee and assumes a single invoice; update checkBalance to accept an invoiceCount (and/or feePerInvoice) parameter and use the actual fee used by the on‑chain call (multiply feePerInvoice * BigInt(invoiceCount)) when computing totalRequired/totalEthRequired in both the ETH branch (tokenAddress === ethers.ZeroAddress) and the ERC20 branch, and have the batch caller (handleBatchPayment) pass the invoice count and the locally-fetched fee (the fee read in payInvoice/handleBatchPayment) into checkBalance so the pre-check matches the actual value: argument names to look for are checkBalance, handleBatchPayment, and the local fee fetch (const fee = await contract.fee()).
🧹 Nitpick comments (1)
frontend/src/page/ReceivedInvoice.jsx (1)
250-252: Hardcoded 300,000 gas limit doesn't scale with batch size
estimatedGasLimit = BigInt(300000)is a constant regardless of how many invoices are in the batch. For a batch of 50 invoices, the actual gas cost is orders of magnitude higher. Consider accepting aninvoiceCountparameter (consistent with the fee fix above) and scaling the gas buffer proportionally, or usingprovider.estimateGasagainst the actual call data for a more accurate bound.♻️ Suggested approach
- const estimatedGasLimit = BigInt(300000); // adjust if needed + // Scale gas estimate proportionally with batch size (rough buffer per invoice) + const gasPerInvoice = BigInt(150000); + const estimatedGasLimit = gasPerInvoice * BigInt(invoiceCount);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/page/ReceivedInvoice.jsx` around lines 250 - 252, The hardcoded gas buffer in ReceivedInvoice.jsx uses estimatedGasLimit = BigInt(300000) which doesn't scale with batch size; update the logic in the area that computes estimatedGasLimit/estimatedGasCost to accept an invoiceCount parameter (or derive count from the batch) and scale the gas limit proportionally (e.g., per-invoice gas * invoiceCount), or replace the constant by calling provider.estimateGas with the actual transaction call data to compute a more accurate bound; adjust uses of estimatedGasLimit and estimatedGasCost accordingly so downstream code (the send/fee calculation path) uses the scaled/estimated value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/test/Chainvoice.t.sol`:
- Line 1: Replace the block-style SPDX header comment with the canonical
line-comment form so downstream tools recognize it: change the current "/*
SPDX-License-Identifier: Unlicense */" in contracts/test/Chainvoice.t.sol to a
single-line comment "// SPDX-License-Identifier: Unlicense" as the very first
line of the file.
- Around line 200-213: The contract is inconsistent: createInvoicesBatch
enforces non-zero amounts but createInvoice does not; either add the same
validation in the contract or make the test defensive. Fix option A: in the
createInvoice function add a require(amount > 0, "Amount zero") (mirror the
check used in createInvoicesBatch) so createInvoice and createInvoicesBatch
behave consistently. Fix option B: if you prefer to allow zero amounts, update
the fuzz test testFuzz_CreateInvoice to assume(amount > 0) (replace or add
vm.assume(amount > 0)) so the test only supplies valid inputs. Ensure you modify
the createInvoice function name or test function name referenced above
accordingly.
---
Outside diff comments:
In `@frontend/src/page/ReceivedInvoice.jsx`:
- Line 428: The toast shown after the balance pre-check in ReceivedInvoice.jsx
currently uses toast.success and misleads users into thinking payment completed;
change the call to toast.info (or another neutral status like toast.pending) in
the function where toast.success("Balance check passed! Processing payment...")
is invoked so it communicates an intermediate state (e.g., "Balance check passed
— processing payment...") instead of a final success; update any related
tests/messages expecting the success toast accordingly.
- Around line 238-296: The balance check under checkBalance currently uses the
component state fee and assumes a single invoice; update checkBalance to accept
an invoiceCount (and/or feePerInvoice) parameter and use the actual fee used by
the on‑chain call (multiply feePerInvoice * BigInt(invoiceCount)) when computing
totalRequired/totalEthRequired in both the ETH branch (tokenAddress ===
ethers.ZeroAddress) and the ERC20 branch, and have the batch caller
(handleBatchPayment) pass the invoice count and the locally-fetched fee (the fee
read in payInvoice/handleBatchPayment) into checkBalance so the pre-check
matches the actual value: argument names to look for are checkBalance,
handleBatchPayment, and the local fee fetch (const fee = await contract.fee()).
---
Nitpick comments:
In `@frontend/src/page/ReceivedInvoice.jsx`:
- Around line 250-252: The hardcoded gas buffer in ReceivedInvoice.jsx uses
estimatedGasLimit = BigInt(300000) which doesn't scale with batch size; update
the logic in the area that computes estimatedGasLimit/estimatedGasCost to accept
an invoiceCount parameter (or derive count from the batch) and scale the gas
limit proportionally (e.g., per-invoice gas * invoiceCount), or replace the
constant by calling provider.estimateGas with the actual transaction call data
to compute a more accurate bound; adjust uses of estimatedGasLimit and
estimatedGasCost accordingly so downstream code (the send/fee calculation path)
uses the scaled/estimated value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addressed Issues:
#123
Checklist
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit
New Features
Improvements