Skip to content

Gas fee optimization #92

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
aniket866 wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Gas fee optimization #92

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
59f984a
Gas fee optimization
aniket866 Jan 28, 2026
b27d627
Code-Rabbit follow-up
aniket866 Jan 28, 2026
2a15139
Merge branch 'main' into patch-2
aniket866 Mar 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions contracts/src/Chainvoice.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,15 +236,17 @@ contract Chainvoice {

// ========== Pay single invoice ==========
function payInvoice(uint256 invoiceId) external payable nonReentrant {
if (invoiceId >= invoices.length) revert InvalidInvoiceId();
InvoiceDetails storage invoice = invoices[invoiceId];

if (msg.sender != invoice.to) revert NotAuthorizedPayer();
if (invoice.isPaid) revert InvoiceAlreadyPaid();
if (invoice.isCancelled) revert InvoiceCancelledError();
require(invoiceId < invoices.length, "Invalid invoice ID");

InvoiceDetails storage invoiceStorage = invoices[invoiceId]; //(read once from storage)
InvoiceDetails memory invoice = invoiceStorage; // now read all from this invoice(memory)
require(msg.sender == invoice.to, "Not authorized");
Comment on lines +241 to +243
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 25, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

fd -t f "Chainvoice.sol" --exec cat -n {} \;

Repository: StabilityNexus/Chainvoice

Length of output: 20284

Cache only required fields instead of copying the entire struct from storage to memory.

Line 242 copies the entire InvoiceDetails struct (including unused fields and dynamic strings) from storage to memory. This introduces unnecessary SLOAD operations for id, encryptedInvoiceData, and encryptedHash—none of which are used by payInvoice. Extract only the six fields actually needed: to, isPaid, isCancelled, tokenAddress, amountDue, and from.

♻️ Proposed refactor (cache only used fields) 
-        InvoiceDetails storage invoiceStorage = invoices[invoiceId]; //(read once from storage)
-        InvoiceDetails memory invoice = invoiceStorage;   // now read all from this invoice(memory)
-        require(msg.sender == invoice.to, "Not authorized");
-        require(!invoice.isPaid, "Already paid");
-        require(!invoice.isCancelled, "Invoice is cancelled");
+        InvoiceDetails storage invoiceStorage = invoices[invoiceId];
+        address to = invoiceStorage.to;
+        bool isPaid = invoiceStorage.isPaid;
+        bool isCancelled = invoiceStorage.isCancelled;
+        address tokenAddress = invoiceStorage.tokenAddress;
+        uint256 amountDue = invoiceStorage.amountDue;
+        address from = invoiceStorage.from;
+
+        if (msg.sender != to) revert NotAuthorizedPayer();
+        if (isPaid) revert InvoiceAlreadyPaid();
+        if (isCancelled) revert InvoiceCancelledError();

         // Effects first for CEI (mark paid, bump fees), then interactions
         invoiceStorage.isPaid = true;

-        if (invoice.tokenAddress == address(0)) {
-            if (msg.value != invoice.amountDue + fee) revert IncorrectPaymentAmount();
+        if (tokenAddress == address(0)) {
+            if (msg.value != amountDue + fee) revert IncorrectPaymentAmount();
             accumulatedFees += fee;

-            (bool sent, ) = payable(invoice.from).call{value: invoice.amountDue}("");
+            (bool sent, ) = payable(from).call{value: amountDue}("");
             if (!sent) revert NativeTransferFailed();
         } else {
             if (msg.value != fee) revert FeeMustBeNative();
-            if (IERC20(invoice.tokenAddress).allowance(msg.sender, address(this)) < invoice.amountDue) {
+            if (IERC20(tokenAddress).allowance(msg.sender, address(this)) < amountDue) {
                 revert InsufficientAllowance();
             }
             accumulatedFees += fee;

-            bool transferSuccess = IERC20(invoice.tokenAddress).transferFrom(
+            bool transferSuccess = IERC20(tokenAddress).transferFrom(
                 msg.sender,
-                invoice.from,
-                invoice.amountDue
+                from,
+                amountDue
             );
             if (!transferSuccess) revert TokenTransferFailed();
         }

         emit InvoicePaid(
             invoiceId,
-            invoice.from,
-            invoice.to,
-            invoice.amountDue,
-            invoice.tokenAddress
+            from,
+            to,
+            amountDue,
+            tokenAddress
         );
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@contracts/src/Chainvoice.sol` around lines 241 - 243, The code currently
copies the entire InvoiceDetails struct from storage into memory (InvoiceDetails
invoice = invoiceStorage) causing unnecessary SLOADs; modify payInvoice to read
only the needed fields from invoices[invoiceId]—cache to, isPaid, isCancelled,
tokenAddress, amountDue, and from—rather than copying encryptedInvoiceData,
encryptedHash, id, etc.; use local variables (e.g., address invoiceTo =
invoices[invoiceId].to; bool paid = invoices[invoiceId].isPaid; ...) or a small
lightweight struct to minimize storage reads and then use those locals in the
subsequent require and logic.

require(!invoice.isPaid, "Already paid");
require(!invoice.isCancelled, "Invoice is cancelled");
Comment on lines +239 to +245
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 25, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify revert style mismatch between contract and tests.
rg -n -C2 'function payInvoice|require\(|revert ' contracts/src/Chainvoice.sol
rg -n -C2 'expectRevert\(Chainvoice\..*selector\)' contracts/test/Chainvoice.t.sol

Repository: StabilityNexus/Chainvoice

Length of output: 9894

Revert ABI changed from custom errors to strings (breaks selector-based tests and increases gas).

Lines 239–245 replace custom-error reverts with require strings. Test line 103 expects Chainvoice.NotAuthorizedPayer.selector, but the code uses require(msg.sender == invoice.to, "Not authorized");, causing test failure. This is also inconsistent with the rest of the codebase, which uses custom errors throughout.

🔧 Proposed fix 
-        require(invoiceId < invoices.length, "Invalid invoice ID");
+        if (invoiceId >= invoices.length) revert InvalidInvoiceId();

         InvoiceDetails storage invoiceStorage = invoices[invoiceId]; //(read once from storage)
         InvoiceDetails memory invoice = invoiceStorage;   // now read all from this invoice(memory)
-        require(msg.sender == invoice.to, "Not authorized");
-        require(!invoice.isPaid, "Already paid");
-        require(!invoice.isCancelled, "Invoice is cancelled");
+        if (msg.sender != invoice.to) revert NotAuthorizedPayer();
+        if (invoice.isPaid) revert AlreadySettled();
+        if (invoice.isCancelled) revert AlreadySettled();

Additionally, lines 241–242 copy the entire struct from storage to memory, including unused dynamic fields (encryptedInvoiceData, encryptedHash). Cache only the required fields (e.g., to, isPaid, isCancelled, tokenAddress, amountDue, from) to reduce gas overhead.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@contracts/src/Chainvoice.sol` around lines 239 - 245, Replace the
string-revert requires with the contract's custom errors (e.g., revert
NotAuthorizedPayer(), revert InvoiceAlreadyPaid(), revert InvoiceCancelled()) so
selector-based tests continue to work and gas usage stays low; do this in the
function using invoiceId/invoices by checking msg.sender, isPaid, and
isCancelled and reverting with the matching custom errors instead of
require(..., "message"). Also stop copying the entire InvoiceDetails storage
struct into memory (InvoiceDetails memory invoice = invoiceStorage); instead
read and cache only the needed fields from InvoiceDetails storage invoiceStorage
— e.g., address to = invoiceStorage.to, bool isPaid = invoiceStorage.isPaid,
bool isCancelled = invoiceStorage.isCancelled, address tokenAddress =
invoiceStorage.tokenAddress, uint256 amountDue = invoiceStorage.amountDue,
address from = invoiceStorage.from — and use those locals for checks and further
logic to avoid copying dynamic fields like encryptedInvoiceData and
encryptedHash.


// Effects first for CEI (mark paid, bump fees), then interactions
invoice.isPaid = true;
invoiceStorage.isPaid = true;

if (invoice.tokenAddress == address(0)) {
if (msg.value != invoice.amountDue + fee) revert IncorrectPaymentAmount();
accumulatedFees += fee;
Expand Down
Loading