Smart Contract Optimization via Custom Errors & Structural Refactoring Recducing String "requires"#102
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces string-based require checks in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/Chainvoice.sol (1)
107-123:⚠️ Potential issue | 🟡 MinorInconsistent validation:
amountDueis not validated for zero in single invoice creation.
createInvoicesBatchvalidatesif (amt == 0) revert InvalidAmount()(line 176), butcreateInvoiceallows zero-value invoices. This inconsistency could lead to unnecessary storage consumption and confusing user experience.🛡️ Proposed fix to add zero-amount validation
function createInvoice( address to, uint256 amountDue, address tokenAddress, string memory encryptedInvoiceData, string memory encryptedHash ) external { if (to == address(0)) revert ZeroAddress(); if (to == msg.sender) revert SelfInvoicing(); + if (amountDue == 0) revert InvalidAmount(); if (tokenAddress != address(0)) {
🧹 Nitpick comments (4)
contracts/src/Chainvoice.sol (3)
52-54: Minor formatting issue: inline comment placement.The trailing comment for
encryptedHashis awkwardly placed on a separate line. Consider moving it inline.✨ Suggested formatting fix
- string encryptedHash; - // Content hash or integrity ref + string encryptedHash; // Content hash or integrity ref }
162-168: Minor: extra leading space at line 163.There's an extra space before the
ifstatement on line 163, causing inconsistent indentation.✨ Formatting fix
if (tokenAddress != address(0)) { - if (tokenAddress.code.length == 0) revert NotContract(); + if (tokenAddress.code.length == 0) revert NotContract(); (bool ok, ) = tokenAddress.staticcall(
267-274: Optional: First invoice is validated twice (here and again in the loop).The validation at lines 271-274 for the first invoice (
invoiceIds[0]) is repeated wheni=0in the loop at lines 279-290. This is correct but slightly gas-inefficient.⛽ Consider starting loop at i=1 and including first invoice's amount
InvoiceDetails storage inv0 = invoices[firstId]; if (msg.sender != inv0.to) revert NotAuthorizedPayer(); if (inv0.isPaid || inv0.isCancelled) revert AlreadySettled(); address token = inv0.tokenAddress; - uint256 totalAmounts = 0; + uint256 totalAmounts = inv0.amountDue; uint256 totalNativeFee = fee * n; // Validate and sum - for (uint256 i = 0; i < n; i++) { + for (uint256 i = 1; i < n; i++) {contracts/test/Chainvoice.t.sol (1)
95-115: Consider expanding test coverage for new custom errors.Only
NotAuthorizedPayerandIncorrectPaymentAmountare tested. Many new error paths are untested:
ZeroAddress,SelfInvoicing(increateInvoice)InvalidInvoiceId,NotInvoiceCreator,InvoiceNotCancellable(incancelInvoice)InvoiceAlreadyPaid,InvoiceCancelledError(inpayInvoice)- Batch-specific errors (
ArrayLengthMismatch,InvalidAmount,MixedTokenBatch)Would you like me to generate additional test cases covering these error paths?
|
@DengreSarthak @kumawatkaran523 all the logic replacements look correct, inversions are done properly. also fixes the missing amountDue == 0 check in single createInvoice that batch already had. it is merge worthy |
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 (3)
contracts/test/Chainvoice.t.sol (1)
99-125:⚠️ Potential issue | 🟠 MajorThe test migration is still incomplete.
These two assertions were updated, but Line 125 still expects the old
"Only owner can call"string even thoughonlyOwnernow revertsUnauthorized(). That leaves the suite out of sync with the contract, and the new selector-based failure paths added increateInvoice,cancelInvoice, batch pay, andwithdrawFeesare still untested.As per coding guidelines, "Verify that any modification to contract logic includes corresponding updates to automated tests", "Ensure failure paths and revert scenarios are explicitly handled and validated", and "Ensure security-sensitive logic changes are not introduced without adequate test coverage."Minimum fix for the stale ownership assertion
- vm.expectRevert("Only owner can call"); + vm.expectRevert(Chainvoice.Unauthorized.selector);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/test/Chainvoice.t.sol` around lines 99 - 125, Update the stale test assertion in testInitiateOwnershipTransfer to expect the Unauthorized() selector instead of the old "Only owner can call" string; locate the failing vm.expectRevert call in testInitiateOwnershipTransfer and replace the string expectation with Chainvoice.Unauthorized.selector (or the appropriate Unauthorized selector symbol). Additionally, add or update tests to cover the new selector-based revert paths introduced in createInvoice, cancelInvoice, batch pay (batchPayInvoices or similar), and withdrawFees by using vm.expectRevert(...selector) with the corresponding Chainvoice.* selector symbols to validate those failure scenarios.contracts/src/Chainvoice.sol (2)
343-372:⚠️ Potential issue | 🟡 Minor
getPaymentStatus()overstates payability for ERC20 invoices.In the token branch,
canPayignores the nativefeethatpayInvoice()enforces withFeeMustBeNative(). A payer with enough tokens and allowance but 0 ETH still getscanPay == true, which will mislead any UI that relies on this helper.Include the native fee in the ERC20 branch
return ( - bal >= invoice.amountDue && allw >= invoice.amountDue, + payer.balance >= fee && bal >= invoice.amountDue && allw >= invoice.amountDue, bal, allw );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/Chainvoice.sol` around lines 343 - 372, getPaymentStatus currently reports canPay=true for ERC20 invoices without checking the native ETH fee required by payInvoice (FeeMustBeNative), so update the token branch in getPaymentStatus to also verify the payer's native balance covers fee: call payer.balance >= fee and include that in the boolean alongside IERC20(invoice.tokenAddress).balanceOf(payer) >= invoice.amountDue and IERC20(...).allowance(payer, address(this)) >= invoice.amountDue; return the same payer token balance and allowance as before but ensure canPay reflects both token and native-fee checks for accurate UI signals.
290-301:⚠️ Potential issue | 🟠 MajorReject duplicate invoice IDs in a batch.
The validation loop checks every entry against current storage before any
isPaidflag is flipped, so[id, id]passes, doublestotalAmounts/totalNativeFee, and pays the same invoice twice. WithMAX_BATCH == 50, an O(n²) uniqueness check is cheap here.Prevent duplicate IDs during validation
+ error DuplicateInvoiceId();for (uint256 i = 0; i < n; i++) { uint256 id = invoiceIds[i]; if (id >= invoices.length) revert InvalidInvoiceId(); + for (uint256 j = i + 1; j < n; j++) { + if (invoiceIds[j] == id) revert DuplicateInvoiceId(); + } InvoiceDetails storage inv = invoices[id];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/Chainvoice.sol` around lines 290 - 301, The validation loop must reject duplicate invoice IDs in the batch: before accumulating totalAmounts/totalNativeFee, add a uniqueness check over invoiceIds (e.g. for each i, loop j = i+1..n-1 and if invoiceIds[i] == invoiceIds[j] revert DuplicateInvoiceId()) so duplicate ids like [id,id] fail; perform this check in the same function where invoiceIds, invoices and InvoiceDetails storage inv are validated (or introduce a DuplicateInvoiceId() error) and run it prior to summing inv.amountDue and checking/setting inv.isPaid.
🤖 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/src/Chainvoice.sol`:
- Around line 20-40: Replace the require string revert in setTreasuryAddress
with the new custom error: locate the setTreasuryAddress function and swap the
line that currently does require(newTreasury != address(0), "Zero address") to
instead revert with the ZeroAddress() custom error so the contract uses the new
error types consistently (reference: setTreasuryAddress and ZeroAddress()).
- Around line 128-133: The creation-time token probe must validate both
balanceOf and allowance to ensure future payments work: implement or extend the
helper _isERC20(address tokenAddress, address spender) to first verify
tokenAddress is a contract (tokenAddress.code.length > 0), then staticcall
balanceOf(address) and allowance(address,address) and ensure both calls succeed
and return >= 32 bytes; update createInvoice() and createInvoicesBatch() to call
_isERC20(tokenAddress, someSpender) (pass the contract or expected spender used
in payInvoice/payInvoicesBatch) instead of the current duplicated inline
balanceOf-only checks so invoices for non-ERC20-like tokens are rejected
up-front.
---
Outside diff comments:
In `@contracts/src/Chainvoice.sol`:
- Around line 343-372: getPaymentStatus currently reports canPay=true for ERC20
invoices without checking the native ETH fee required by payInvoice
(FeeMustBeNative), so update the token branch in getPaymentStatus to also verify
the payer's native balance covers fee: call payer.balance >= fee and include
that in the boolean alongside IERC20(invoice.tokenAddress).balanceOf(payer) >=
invoice.amountDue and IERC20(...).allowance(payer, address(this)) >=
invoice.amountDue; return the same payer token balance and allowance as before
but ensure canPay reflects both token and native-fee checks for accurate UI
signals.
- Around line 290-301: The validation loop must reject duplicate invoice IDs in
the batch: before accumulating totalAmounts/totalNativeFee, add a uniqueness
check over invoiceIds (e.g. for each i, loop j = i+1..n-1 and if invoiceIds[i]
== invoiceIds[j] revert DuplicateInvoiceId()) so duplicate ids like [id,id]
fail; perform this check in the same function where invoiceIds, invoices and
InvoiceDetails storage inv are validated (or introduce a DuplicateInvoiceId()
error) and run it prior to summing inv.amountDue and checking/setting
inv.isPaid.
In `@contracts/test/Chainvoice.t.sol`:
- Around line 99-125: Update the stale test assertion in
testInitiateOwnershipTransfer to expect the Unauthorized() selector instead of
the old "Only owner can call" string; locate the failing vm.expectRevert call in
testInitiateOwnershipTransfer and replace the string expectation with
Chainvoice.Unauthorized.selector (or the appropriate Unauthorized selector
symbol). Additionally, add or update tests to cover the new selector-based
revert paths introduced in createInvoice, cancelInvoice, batch pay
(batchPayInvoices or similar), and withdrawFees by using
vm.expectRevert(...selector) with the corresponding Chainvoice.* selector
symbols to validate those failure scenarios.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b3eec8c-6f10-4693-9523-26f474a64bfa
📒 Files selected for processing (2)
contracts/src/Chainvoice.solcontracts/test/Chainvoice.t.sol
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/src/Chainvoice.sol (2)
295-306:⚠️ Potential issue | 🔴 CriticalReject duplicate invoice IDs in
payInvoicesBatch().The settlement checks run before anything is marked paid, so
[id, id]currently passes validation and charges the payer twice for the same invoice. That is an irreversible overpayment bug in both the native and ERC-20 branches. WithMAX_BATCHcapped at 50, a simple O(n²) duplicate precheck is cheap enough here.Suggested fix
+ error DuplicateInvoiceId();for (uint256 i = 0; i < n; i++) { uint256 id = invoiceIds[i]; if (id >= invoices.length) revert InvalidInvoiceId(); + for (uint256 j = 0; j < i; j++) { + if (invoiceIds[j] == id) revert DuplicateInvoiceId(); + } InvoiceDetails storage inv = invoices[id];As per coding guidelines, "Review for common smart contract vulnerabilities, including but not limited to: Improper input validation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/Chainvoice.sol` around lines 295 - 306, payInvoicesBatch currently allows duplicate invoiceIds (e.g., [id,id]) which passes validation and causes double payment; before performing settlement checks in payInvoicesBatch iterate the invoiceIds once to detect duplicates (either with an in-memory mapping(uint256=>bool) seen or an O(n^2) nested loop given MAX_BATCH<=50) and revert on duplicates (e.g., DuplicateInvoiceId()) referencing the invoiceIds array and InvoiceDetails entries; place this duplicate-precheck at the start of payInvoicesBatch (before the loop that checks inv.isPaid/isCancelled and accumulates totalAmounts) so no invoice can be processed twice in the same batch.
356-377:⚠️ Potential issue | 🟡 MinorMake
getPaymentStatus()match the actual payment preconditions.
canPaycan still returntruefor an already-paid invoice, and the ERC-20 branch ignores the required nativefee. That means off-chain callers can be told an invoice is payable even thoughpayInvoice()will revert.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/Chainvoice.sol` around lines 356 - 377, getPaymentStatus incorrectly reports invoices as payable when they're already paid and the ERC-20 branch ignores the native fee; update the function (the logic around invoices[invoiceId], InvoiceDetails, payer, fee, and the IERC20 balanceOf/allowance checks) to first treat already-paid invoices as not payable (e.g., check invoice.isPaid or invoice.amountPaid >= invoice.amountDue and return canPay=false), and in the ERC-20 branch require both token balance/allowance >= invoice.amountDue AND payer.balance >= fee (ensure the native fee check is included) so canPay is true only when invoice not paid, token funds/allowance cover amountDue, and the payer has native balance for the fee.
♻️ Duplicate comments (1)
contracts/src/Chainvoice.sol (1)
178-184:⚠️ Potential issue | 🟠 MajorHarden batch token validation to match the single-invoice path.
createInvoicesBatch()still accepts any contract that answersbalanceOf(address), whilecreateInvoice()now requires bothbalanceOfandallowanceto succeed with valid return data. That inconsistency lets batch-created invoices slip through with tokens that later fail inpayInvoicesBatch(), leaving invoices that cannot be settled. Please reuse the stricter probe here, ideally via a shared helper.Suggested fix
if (tokenAddress != address(0)) { - if (tokenAddress.code.length == 0) revert NotContract(); - (bool ok, ) = tokenAddress.staticcall( - abi.encodeWithSignature("balanceOf(address)", address(this)) - ); - if (!ok) revert InvalidToken(); + if (tokenAddress.code.length == 0) revert NotContract(); + (bool balOk, bytes memory balData) = tokenAddress.staticcall( + abi.encodeWithSelector(IERC20.balanceOf.selector, address(this)) + ); + (bool allowanceOk, bytes memory allowanceData) = tokenAddress.staticcall( + abi.encodeWithSelector(IERC20.allowance.selector, address(this), address(this)) + ); + if (!balOk || balData.length < 32 || !allowanceOk || allowanceData.length < 32) { + revert InvalidToken(); + } }As per coding guidelines, "Ensure failure paths and revert scenarios are explicitly handled and validated."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/Chainvoice.sol` around lines 178 - 184, createInvoicesBatch currently only probes token contracts with balanceOf, causing inconsistency with createInvoice which probes both balanceOf and allowance; implement a shared helper (e.g., _probeTokenContract(address tokenAddress)) and call it from both createInvoice and createInvoicesBatch; in the helper ensure tokenAddress != address(0) and tokenAddress.code.length > 0, perform staticcall for both "balanceOf(address)" and "allowance(address,address)", validate calls succeeded and that returned data decodes to a uint256 for balance and allowance (revert InvalidToken() on any failure), then reuse this helper in payInvoicesBatch-related validation paths as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@contracts/src/Chainvoice.sol`:
- Around line 295-306: payInvoicesBatch currently allows duplicate invoiceIds
(e.g., [id,id]) which passes validation and causes double payment; before
performing settlement checks in payInvoicesBatch iterate the invoiceIds once to
detect duplicates (either with an in-memory mapping(uint256=>bool) seen or an
O(n^2) nested loop given MAX_BATCH<=50) and revert on duplicates (e.g.,
DuplicateInvoiceId()) referencing the invoiceIds array and InvoiceDetails
entries; place this duplicate-precheck at the start of payInvoicesBatch (before
the loop that checks inv.isPaid/isCancelled and accumulates totalAmounts) so no
invoice can be processed twice in the same batch.
- Around line 356-377: getPaymentStatus incorrectly reports invoices as payable
when they're already paid and the ERC-20 branch ignores the native fee; update
the function (the logic around invoices[invoiceId], InvoiceDetails, payer, fee,
and the IERC20 balanceOf/allowance checks) to first treat already-paid invoices
as not payable (e.g., check invoice.isPaid or invoice.amountPaid >=
invoice.amountDue and return canPay=false), and in the ERC-20 branch require
both token balance/allowance >= invoice.amountDue AND payer.balance >= fee
(ensure the native fee check is included) so canPay is true only when invoice
not paid, token funds/allowance cover amountDue, and the payer has native
balance for the fee.
---
Duplicate comments:
In `@contracts/src/Chainvoice.sol`:
- Around line 178-184: createInvoicesBatch currently only probes token contracts
with balanceOf, causing inconsistency with createInvoice which probes both
balanceOf and allowance; implement a shared helper (e.g.,
_probeTokenContract(address tokenAddress)) and call it from both createInvoice
and createInvoicesBatch; in the helper ensure tokenAddress != address(0) and
tokenAddress.code.length > 0, perform staticcall for both "balanceOf(address)"
and "allowance(address,address)", validate calls succeeded and that returned
data decodes to a uint256 for balance and allowance (revert InvalidToken() on
any failure), then reuse this helper in payInvoicesBatch-related validation
paths as appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6857459d-3635-4a2f-b3c9-d1235ed2238c
📒 Files selected for processing (1)
contracts/src/Chainvoice.sol
|
✅ Actions performedComments resolved and changes approved. |
3 participants
Closes #103
What is one by this PR:
Refactored Chainvoice.sol to replace expensive string revert messages with gas-efficient Custom Errors across all functions.
Optimized contract modifiers and internal naming conventions to align with best practices and resolve linting warnings.
Updated Chainvoice.t.sol test suite to verify specific custom error selectors and cleaned up unused imports.
@kumawatkaran523 Please review
Summary by CodeRabbit
Refactor
Tests