fix: prevent self-invoicing with frontend validation #84
Conversation
|
@SIDDHANTCOOKIE Although it’s good to prevent self-invoice scenarios, and great that you identified and fixed the issue, can we also work on improving the UI of the alert message? |
|
@kumawatkaran523 Thanks for the review. I will improve the UI of the alert message and update the PR shortly. |
📝 WalkthroughWalkthroughAdds inline client wallet address validation to the CreateInvoice form, integrates validation into prefill and submission flows (blocking submit on error), renders an inline clientAddressError alert, and introduces a duplicated Changes
Sequence Diagram(s)sequenceDiagram
participant U as User (UI)
participant C as CreateInvoice component
participant E as Ethers/isAddress check
participant S as Invoice API (createInvoiceRequest)
U->>C: Enter or prefill client address
C->>C: validateClientAddress(value)
C->>E: isAddress(value) / format checks
alt invalid or self-invoice
C-->>U: set `clientAddressError` (inline alert) and block submit
else valid
C-->>U: clear `clientAddressError`
U->>C: Submit form
C->>C: validateClientAddress(currentValue)
alt still valid
C->>S: createInvoiceRequest(payload)
S-->>C: success / failure
else invalid
C-->>U: show `clientAddressError` and abort
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
🤖 Fix all issues with AI agents
In `@frontend/src/page/CreateInvoice.jsx`:
- Around line 725-735: The URL param useEffect sets clientAddress but doesn't
run validation; update the same effect that reads the clientAddress URL param
(where setClientAddress(...) is called) to also call
validateClientAddress(clientAddressValue) after setting state so the error state
reflects URL-prefilled values; ensure you reference the clientAddress state, the
validateClientAddress function, and the useEffect that parses URL params to
perform this additional call.
🧹 Nitpick comments (1)
frontend/src/page/CreateInvoice.jsx (1)
336-347: Duplicate validation logic and inconsistent null safety.Two issues:
DRY violation: This block duplicates the logic already in
validateClientAddress. Consider refactoringvalidateClientAddressto return a boolean or error string, then reuse it here.Potential runtime error: Line 343 uses
account.address.toLowerCase()without optional chaining, but line 322 invalidateClientAddressusesaccount.address?.toLowerCase(). Ifaccount.addressis undefined, this will throw.♻️ Suggested refactor
-const validateClientAddress = (value) => { +const validateClientAddress = (value, setError = true) => { // Empty input, no error if (!value) { - setClientAddressError(""); - return; + if (setError) setClientAddressError(""); + return "Please enter a valid wallet address"; } // Do not validate until it looks like a full EVM address if (!value.startsWith("0x") || value.length < 42) { - setClientAddressError(""); - return; + if (setError) setClientAddressError(""); + return "Please enter a valid wallet address"; } // Invalid EVM address if (!ethers.isAddress(value)) { - setClientAddressError("Please enter a valid wallet address"); - return; + if (setError) setClientAddressError("Please enter a valid wallet address"); + return "Please enter a valid wallet address"; } // Self-invoicing check if (value.toLowerCase() === account.address?.toLowerCase()) { - setClientAddressError("You cannot create an invoice for your own wallet"); - return; + const msg = "You cannot create an invoice for your own wallet"; + if (setError) setClientAddressError(msg); + return msg; } // Valid other wallet - setClientAddressError(""); + if (setError) setClientAddressError(""); + return null; };Then in
createInvoiceRequest:-// Validate client address - if (!data.clientAddress || !ethers.isAddress(data.clientAddress)) { - setClientAddressError("Please enter a valid wallet address"); - return; - } - - // Check for self-invoicing - if (data.clientAddress.toLowerCase() === account.address.toLowerCase()) { - setClientAddressError("You cannot create an invoice for your own wallet"); - return; -} - setClientAddressError(""); + const validationError = validateClientAddress(data.clientAddress); + if (validationError) { + return; + }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/page/CreateInvoice.jsx`:
- Around line 318-376: Remove the duplicated verifyToken function (the one
defined around lines 318-376) and keep the original useCallback-wrapped
verifyToken; delete the duplicate's rpcUrls and logic, remove the debug
console.log, and ensure the retained (useCallback) verifyToken sets chainId on
setVerifiedToken (add chainId: chainIdToUse) and uses the shared
CHAIN_ID_TO_PUBLIC_RPC constant instead of redefining RPC URLs so provider
selection remains RPC-first for link-sharing.
🧹 Nitpick comments (1)
frontend/src/page/CreateInvoice.jsx (1)
378-405: Consider wrapping inuseCallbackfor consistency.The validation logic is correct. Since this function is passed as a callback to
onChangeand referencesaccount.address, consider wrapping it inuseCallbackwith[account.address]dependencies for better performance and to avoid unnecessary re-renders.♻️ Optional improvement
-const validateClientAddress = (value) => { +const validateClientAddress = useCallback((value) => { // Empty input, no error if (!value) { setClientAddressError(""); return; } // ... rest of function setClientAddressError(""); -}; +}, [account.address]);
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/page/CreateInvoice.jsx`:
- Around line 159-161: The useEffect calls validateClientAddress before its
const definition, creating a fragile TDZ risk; move the validateClientAddress
function above the useEffect that uses it (or convert it to a useCallback and
place its definition earlier) and then add that validateClientAddress callback
to the useEffect dependency array so the hook references a stable, defined
function (ensure validateClientAddress retains access to account.address via its
dependency list if converted to useCallback).
🧹 Nitpick comments (3)
frontend/src/page/CreateInvoice.jsx (3)
379-406: WrapvalidateClientAddressinuseCallbackfor memoization and closure safety.This function depends on
account.addressbut isn't memoized. Ifaccountchanges (e.g., user switches wallets), the closure captures the old value until the component re-renders. Additionally, withoutuseCallback, the function is recreated on every render.♻️ Proposed fix
-const validateClientAddress = (value) => { +const validateClientAddress = useCallback((value) => { if (!value) { setClientAddressError(""); return; } if (!value.startsWith("0x") || value.length < 42) { setClientAddressError(""); return; } if (!ethers.isAddress(value)) { setClientAddressError("Please enter a valid wallet address"); return; } if (value.toLowerCase() === account.address?.toLowerCase()) { setClientAddressError("You cannot create an invoice for your own wallet"); return; } setClientAddressError(""); -}; +}, [account.address]);
413-424: Validation at submission is correct, but consider returning a boolean fromvalidateClientAddress.Having validation both on input change and at submission time is good defense-in-depth. However, the logic is duplicated. Consider refactoring
validateClientAddressto return a boolean, which would allow reuse:const isValid = validateClientAddress(data.clientAddress); if (!isValid) return;This is optional since the current implementation works correctly.
807-810: Consider extracting the inline handler for readability.The inline handler works but is cramped. A named handler would be clearer:
const handleClientAddressChange = (e) => { const value = e.target.value; setClientAddress(value); validateClientAddress(value); };Then:
onChange={handleClientAddressChange}
|
@kumawatkaran523 as you told I've made the changes to the alerts, now the alerts are shown inline alert: when user puts the same address to prevent self-invoicing and when a user puts invalid address to remind of the invalid address. Visual of the alerts
|
2 participants
Problem
Users will get cryptic "missing revert data" error when trying to create an invoice to themselves. The contract correctly blocks self-invoicing, but the frontend doesn't validate this before submission, causing gas estimation to fail.
Solution
Added frontend validation in
CreateInvoice.jsxto check:Now users get a clear error message: "Cannot create invoice to yourself. Please use a different client address."
Fixes #82
Testing
@Zahnentferner @kumawatkaran523 is my approach correct ? awaiting review thanks
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.