Smart Contract Optimization via Custom Errors & Structural Refactoring Recducing String "requires"

[aniket866]

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

  • Replaced string revert messages with a unified set of explicit error codes for clearer, consistent failure reporting across invoice creation, payments, cancellations and batch operations; strengthened input and access validation while preserving public interfaces.

• Tests

  • Updated tests to expect the new error-driven behavior (error selectors) while keeping existing control flow and assertions.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Replaces string-based require messages in contracts/src/Chainvoice.sol with many new custom error definitions and switches contract validation and transactional reverts to use those errors; two tests updated to expect the new error selectors.

Changes

Cohort / File(s)	Summary
Contract Error Handling Refactor contracts/src/Chainvoice.sol	Adds many custom errors (e.g., Unauthorized, Reentrancy, ZeroAddress, SelfInvoicing, NotContract, InvalidToken, ArrayLengthMismatch, InvalidAmount, InvalidInvoiceId, NotInvoiceCreator, InvoiceNotCancellable, InvoiceAlreadyPaid, InvoiceCancelledError, IncorrectPaymentAmount, NativeTransferFailed, FeeMustBeNative, TokenTransferFailed, TreasuryNotSet, NoFeesAvailable, WithdrawFailed, etc.) and replaces string require messages with revert(<Error>()) across constructor, modifiers, invoice lifecycle, batch operations, payment flows, and withdrawal logic; minor formatting/comment tweaks only.
Test Assertions Updated contracts/test/Chainvoice.t.sol	Updates two tests to expect error selectors (NotAuthorizedPayer, IncorrectPaymentAmount) instead of former string revert messages.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through code with careful paws,
Swapping strings for errors with no pause.
Selectors crisp, reverts now clear,
Invoices, batches — all hop near.
🥕✨

🚥 Pre-merge checks | ✅ 3

✅ 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 references the main change (custom errors replacing string requires) but contains a typo ('Recducing' instead of 'Reducing') and is slightly awkward phrasing.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

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 | 🟡 Minor

Inconsistent validation: amountDue is not validated for zero in single invoice creation.

createInvoicesBatch validates if (amt == 0) revert InvalidAmount() (line 176), but createInvoice allows 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 encryptedHash is 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 if statement 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 when i=0 in 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 NotAuthorizedPayer and IncorrectPaymentAmount are tested. Many new error paths are untested:

• ZeroAddress, SelfInvoicing (in createInvoice)
• InvalidInvoiceId, NotInvoiceCreator, InvoiceNotCancellable (in cancelInvoice)
• InvoiceAlreadyPaid, InvoiceCancelledError (in payInvoice)
• Batch-specific errors (ArrayLengthMismatch, InvalidAmount, MixedTokenBatch)

Would you like me to generate additional test cases covering these error paths?