Complete Ledger KYC Upload#54
Open
Satishpethani92 wants to merge 6 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Ledger hardware-wallet provider and helpers, centralized wallet error normalization, robust personal-sign recovery, expanded KYC /addKYC verification, and client wiring so KYC upload and transactions can be signed and sent via hardware-wallet or standard flows with consistent receipt and error handling. ChangesWallet Integration & KYC Flow Refactor
Sequence DiagramssequenceDiagram
participant App
participant LedgerProvider as Ledger Web3 Provider
participant Hooks as Signing Hooks
participant RPC as Underlying RPC
App->>LedgerProvider: eth_sendTransaction
LedgerProvider->>RPC: eth_getTransactionCount
RPC-->>LedgerProvider: nonce
LedgerProvider->>Hooks: prepareTransaction
Hooks-->>LedgerProvider: normalized tx
LedgerProvider->>Hooks: signTransaction
Hooks-->>LedgerProvider: signed tx
LedgerProvider->>Hooks: sendSignedTransaction
Hooks->>RPC: eth_sendRawTransaction
RPC-->>LedgerProvider: txHash
LedgerProvider-->>App: JSON-RPC response
sequenceDiagram
participant Component
participant AppVM as Vue App Instance
participant RPC as RPC Provider
Component->>AppVM: sendSignedTransaction(signedTx)
AppVM->>AppVM: verify signature & recover sender
AppVM->>RPC: eth_sendRawTransaction
RPC-->>AppVM: txHash
AppVM->>RPC: poll eth_getTransactionReceipt
alt Receipt found
RPC-->>AppVM: receipt
AppVM-->>Component: resolve(receipt)
else Timeout
AppVM-->>Component: reject(timeout error)
end
sequenceDiagram
participant User as User/Component
participant App as Vue App
participant IPFS as /api/ipfs/addKYC
participant Contract as Smart Contract
User->>App: uploadKYC()
App->>App: signMessage (hardware or web3)
App->>IPFS: POST {account, message, signature}
IPFS-->>App: {hash, txData}
alt Hardware Wallet
App->>App: sendHardwareWalletTransaction
else Standard Wallet
App->>App: sendContractTransaction
end
App->>Contract: uploadKYC(hash)
Contract-->>App: receipt
App->>User: success toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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🧪 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/app.js (2)
1138-1155:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap
case 'ledger'block in braces to fix scope leakage.The
const/letdeclarations on lines 1145-1147 are accessible from other switch cases, which can cause unexpected behavior. This is flagged by the static analyzer.Proposed fix
case 'ledger': + { await ensureLedgerEth(path) const signature = await Vue.prototype.appEth.signPersonalMessage( path, Buffer.from(message).toString('hex') ) const r = toHexBuffer(signature.r) const s = toHexBuffer(signature.s) let v = signature.v if (typeof v === 'string') { v = parseInt(v, 16) } if (v < 27) { v += 27 } result = ethUtils.toRpcSig(v, r, s) break + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/app.js` around lines 1138 - 1155, The switch case for 'ledger' leaks block-scoped variables (signature, r, s, v) because they are declared with const/let without an enclosing block; wrap the entire case 'ledger' body in braces (i.e., case 'ledger': { ... break } ) so ensureLedgerEth, Vue.prototype.appEth.signPersonalMessage, toHexBuffer and ethUtils.toRpcSig remain scoped to that case and do not bleed into other cases.
1156-1162:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOperator precedence bug:
'0x' + sig.payload.signature || ''never falls back to empty string.The expression evaluates as
('0x' + sig.payload.signature) || ''. Ifsignatureisundefined, the result is'0xundefined'rather than''.Proposed fix
case 'trezor': + { const sig = await TrezorConnect.ethereumSignMessage({ path, message }) - result = '0x' + sig.payload.signature || '' + result = sig.payload && sig.payload.signature + ? '0x' + sig.payload.signature + : '' break + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/app.js` around lines 1156 - 1162, The current assignment to result uses operator precedence incorrectly so '0x' + sig.payload.signature || '' yields '0xundefined' if signature is undefined; update the trezor branch (the sig variable returned by TrezorConnect.ethereumSignMessage and the result assignment) to explicitly check for a valid signature before prefixing '0x' — e.g., test sig && sig.payload && sig.payload.signature (or use a ternary) and only then set result = '0x' + sig.payload.signature, otherwise set result = ''.
🧹 Nitpick comments (7)
config/local.json (1)
9-11: 💤 Low valueConsider formatting the CORS array for better readability.
The CORS origins array is on a single line. For maintainability, consider formatting it with one origin per line:
📝 Proposed formatting improvement
"cors": [ - "https://swagger.apothem.network","https://localhost:5000" + "https://swagger.apothem.network", + "https://localhost:5000" ],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/local.json` around lines 9 - 11, The CORS origins array under the "cors" key in config/local.json is currently on a single line; reformat it so each origin is on its own line for readability and easier diffs—locate the "cors" JSON array and split entries onto separate lines with proper JSON commas and indentation (one origin per line inside the array) while preserving the existing origin strings and valid JSON syntax.helpers/ledgerWeb3Provider.js (1)
145-148: 💤 Low valueCallback signature missing second argument on error.
Standard Node.js callback convention is
callback(err, result). Passing only the error without the second argument may cause issues with consumers that destructure both parameters.Proposed fix
run() .then((response) => callback(null, response)) - .catch((error) => callback(toWalletError(error))) + .catch((error) => callback(toWalletError(error), null))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@helpers/ledgerWeb3Provider.js` around lines 145 - 148, The promise catch currently calls callback(toWalletError(error)) which violates Node.js (err, result) convention; update the catch handler in the run().then(...).catch(...) chain to call callback(toWalletError(error), null) (or undefined) so the second argument is explicitly provided, referencing the existing run() promise chain, the callback invocation, and the toWalletError conversion to locate the change.app/components/candidates/Apply.vue (3)
592-604: ⚡ Quick winFallback from
personal.signtoeth.signmay produce incompatible signatures.
personal.signprefixes the message with"\x19Ethereum Signed Message:\n"whileeth.signmay not. The server's recovery logic should handle this, but swallowing the first error without logging makes debugging difficult.Proposed fix: Log the initial error
} else { try { signedMessage = await this.web3.eth.personal.sign( message, signerAccount, '' ) } catch (e) { + console.warn('personal.sign failed, falling back to eth.sign', e.message) signedMessage = await this.web3.eth.sign(message, signerAccount) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/candidates/Apply.vue` around lines 592 - 604, The fallback from this.web3.eth.personal.sign to this.web3.eth.sign can produce incompatible signatures and the original error is swallowed; in the catch block around the personal.sign call in Apply.vue, log the caught error (e) with context (e.g., "personal.sign failed", signerAccount, message) before attempting the eth.sign fallback so debugging shows why personal.sign failed and what was attempted.
606-621: 💤 Low valueTriple transmission of auth fields is redundant.
The same
account,message, andsignedMessageare sent via FormData body (lines 607-609), query params (lines 611-615), and custom headers (lines 616-620). The server accepts any of these sources. Pick one authoritative source to reduce payload size and potential for mismatched values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/candidates/Apply.vue` around lines 606 - 621, The code currently sends account/message/signedMessage three times (in formData via formData.append, as query params in axios.post params, and as custom headers), which is redundant and risky; pick one authoritative transport (recommend keeping them in headers like 'x-kyc-account'/'x-kyc-message'/'x-kyc-signature' and remove the duplicate formData.append calls for account/message/signedMessage and the params object) so only the file is sent in formData (this.KYC.file) and authentication fields are sent once with axios.post('/api/ipfs/addKYC', formData, { headers: { ... } }); ensure any server-side handler still reads the chosen source (headers) and update any client-side references to signerAccount/message/signedMessage accordingly.
626-630: 💤 Low valueHardcoded gas value differs from other transactions.
Other transactions in this file use
self.chainConfig.gas(line 395-396), but here3000000is hardcoded. For consistency and configurability, consider using the chain config value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/candidates/Apply.vue` around lines 626 - 630, The txOptions in Apply.vue currently hardcodes gas as 3000000 (in the txOptions object where gasPrice is set via this.web3.utils.toHex(gasPrice) and gasLimit similarly), causing inconsistency with other transactions; replace the hardcoded 3000000 values for gas and gasLimit with the configured value (self.chainConfig.gas) and convert it to hex using this.web3.utils.toHex(self.chainConfig.gas) so it matches the other usages (see where self.chainConfig.gas is used around lines ~395).apis/ipfs.js (2)
90-107: ⚡ Quick winSilent error swallowing hinders debugging.
All
catch (e) {}blocks discard exception details. Consider adding structured debug logging to aid troubleshooting signature recovery failures in production.Proposed fix
// 2. Recover Signer (Ledger / hardware wallets may use alternate v bytes) let recovered = await recoverPersonalSignAddress(message, signedMessage) if (!recovered) { const candidateMessages = [message] try { candidateMessages.push(web3.utils.utf8ToHex(message)) - } catch (e) {} + } catch (e) { + console.debug('utf8ToHex encoding failed', e.message) + } try { candidateMessages.push(web3.utils.sha3(message)) - } catch (e) {} + } catch (e) { + console.debug('sha3 encoding failed', e.message) + } for (const candidateMessage of candidateMessages) { try { recovered = (await web3.eth.accounts.recover(candidateMessage, signedMessage) || '').toLowerCase() if (recovered) break - } catch (e) {} + } catch (e) { + console.debug('recover attempt failed', e.message) + } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apis/ipfs.js` around lines 90 - 107, The empty catch blocks in the recovery logic (around recoverPersonalSignAddress, web3.utils.utf8ToHex, web3.utils.sha3, and web3.eth.accounts.recover) swallow errors; update each catch to log structured debug info including the operation and the failing candidateMessage (but do not log raw signedMessage) using the project's logger (e.g., processLogger.error) with the caught error object as metadata, and fall back to console.error if the logger is unavailable so signature recovery failures are visible in logs for diagnostics.
83-88: 💤 Low value15-minute signature window is generous for replay protection.
The expiry increased from 5 to 15 minutes. While this accommodates hardware wallet signing delays, it widens the replay attack window. Ensure this tradeoff is acceptable for your threat model.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apis/ipfs.js` around lines 83 - 88, The signature expiry window (fifteenMinutes) used in the addKYC check is too large for replay protection; change it to a safer default and make it configurable: replace the hard-coded fifteenMinutes = 15 * 60 * 1000 with a configurable value (e.g., process.env.SIGNATURE_WINDOW_MS parsed to integer with a default of 5 * 60 * 1000), keep the existing isNaN(signedTime) / Math.abs(currentTime - signedTime) check but reference the new variable (e.g., signatureWindowMs) so operators can increase only if they accept the replay risk; optionally add a short log note when a non-default value is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apis/ipfs.js`:
- Around line 23-29: In normalizeValue the Array.isArray branch uses value[0]
which becomes undefined for empty arrays and yields the string "undefined";
change the array handling in normalizeValue (function normalizeValue) to check
for an empty array and return '' (or set value = ''), otherwise assign value =
value[0]; also ensure that after selecting the first element you treat
null/undefined by returning '' before calling String(...).trim().replace(...) so
empty arrays and null/undefined first elements do not become the string
"undefined".
In `@app/components/candidates/Apply.vue`:
- Around line 570-585: The account resolution order is inconsistent between
hardware and standard wallets (in Apply.vue) causing different accounts to be
chosen; standardize the lookup order used to set signerAccount so both branches
call getAccount() in the same relative position. Update the logic around
NetworkProvider/provider/store.get('network') and the signerAccount assignment
(the branches that call this.toRpcAddress and await this.getAccount()) to use a
single, consistent resolution sequence (e.g., await this.getAccount() ||
this.account || store.get('address') || '') for both hardware
('ledger'|'trezor') and non-hardware paths so toRpcAddress and toLowerCase
operate on the same resolved value.
- Around line 631-648: The code force-sets this.KYC.status = true after calling
getKYCStatus, which can mask failed transactions; instead, use the actual
on-chain result or transaction receipt to update status: after calling
sendHardwareWalletTransaction or sendContractTransaction capture the transaction
receipt or error, call getKYCStatus(this.account) and use its returned value to
set this.KYC.status (do not unconditionally assign true), and if receipts are
unavailable consider marking a pending state and optionally polling getKYCStatus
until it becomes true; update references in sendHardwareWalletTransaction,
sendContractTransaction and getKYCStatus to surface the receipt/result so the UI
reflects real on-chain state.
In `@helpers/ledgerWeb3Provider.js`:
- Line 158: The file mixes CommonJS requires with an ES module export; replace
the ES6 export with a CommonJS export so exports are consistent with the rest of
the helpers: export the createLedgerWeb3Provider and toRpcAddress symbols via
module.exports (e.g., set module.exports = { createLedgerWeb3Provider,
toRpcAddress }) and remove the `export { ... }` statement so the module uses
only CommonJS style.
In `@helpers/personalSign.js`:
- Around line 74-87: The ecrecover call in recoverPersonalSignAddress is passing
recoveryId (0/1) as v but ethereumjs-util expects legacy v (27/28), so change
the code to pass recoveryId + 27 (ensure it's a Number) when calling
ethUtil.ecrecover(msgHash, v, r, s); locate the loop using recoveryId, msgHash,
sigBuf and replace the v argument with (Number(recoveryId) + 27) and keep r as
sigBuf.slice(0,32) and s as sigBuf.slice(32,64), preserving the try/catch and
return behavior.
In `@helpers/walletError.js`:
- Around line 62-68: toWalletError currently mutates the original Error.message
which can double-format or lose context; instead, create and return a new Error
instance that uses formatWalletError(error) for its message while preserving
relevant properties (stack, name, and any custom fields) from the original
error; update the toWalletError function to construct a cloned Error (copy
stack/name/custom props) and set the formatted message rather than modifying the
passed-in Error; reference formatWalletError and toWalletError when making the
change.
---
Outside diff comments:
In `@app/app.js`:
- Around line 1138-1155: The switch case for 'ledger' leaks block-scoped
variables (signature, r, s, v) because they are declared with const/let without
an enclosing block; wrap the entire case 'ledger' body in braces (i.e., case
'ledger': { ... break } ) so ensureLedgerEth,
Vue.prototype.appEth.signPersonalMessage, toHexBuffer and ethUtils.toRpcSig
remain scoped to that case and do not bleed into other cases.
- Around line 1156-1162: The current assignment to result uses operator
precedence incorrectly so '0x' + sig.payload.signature || '' yields
'0xundefined' if signature is undefined; update the trezor branch (the sig
variable returned by TrezorConnect.ethereumSignMessage and the result
assignment) to explicitly check for a valid signature before prefixing '0x' —
e.g., test sig && sig.payload && sig.payload.signature (or use a ternary) and
only then set result = '0x' + sig.payload.signature, otherwise set result = ''.
---
Nitpick comments:
In `@apis/ipfs.js`:
- Around line 90-107: The empty catch blocks in the recovery logic (around
recoverPersonalSignAddress, web3.utils.utf8ToHex, web3.utils.sha3, and
web3.eth.accounts.recover) swallow errors; update each catch to log structured
debug info including the operation and the failing candidateMessage (but do not
log raw signedMessage) using the project's logger (e.g., processLogger.error)
with the caught error object as metadata, and fall back to console.error if the
logger is unavailable so signature recovery failures are visible in logs for
diagnostics.
- Around line 83-88: The signature expiry window (fifteenMinutes) used in the
addKYC check is too large for replay protection; change it to a safer default
and make it configurable: replace the hard-coded fifteenMinutes = 15 * 60 * 1000
with a configurable value (e.g., process.env.SIGNATURE_WINDOW_MS parsed to
integer with a default of 5 * 60 * 1000), keep the existing isNaN(signedTime) /
Math.abs(currentTime - signedTime) check but reference the new variable (e.g.,
signatureWindowMs) so operators can increase only if they accept the replay
risk; optionally add a short log note when a non-default value is used.
In `@app/components/candidates/Apply.vue`:
- Around line 592-604: The fallback from this.web3.eth.personal.sign to
this.web3.eth.sign can produce incompatible signatures and the original error is
swallowed; in the catch block around the personal.sign call in Apply.vue, log
the caught error (e) with context (e.g., "personal.sign failed", signerAccount,
message) before attempting the eth.sign fallback so debugging shows why
personal.sign failed and what was attempted.
- Around line 606-621: The code currently sends account/message/signedMessage
three times (in formData via formData.append, as query params in axios.post
params, and as custom headers), which is redundant and risky; pick one
authoritative transport (recommend keeping them in headers like
'x-kyc-account'/'x-kyc-message'/'x-kyc-signature' and remove the duplicate
formData.append calls for account/message/signedMessage and the params object)
so only the file is sent in formData (this.KYC.file) and authentication fields
are sent once with axios.post('/api/ipfs/addKYC', formData, { headers: { ... }
}); ensure any server-side handler still reads the chosen source (headers) and
update any client-side references to signerAccount/message/signedMessage
accordingly.
- Around line 626-630: The txOptions in Apply.vue currently hardcodes gas as
3000000 (in the txOptions object where gasPrice is set via
this.web3.utils.toHex(gasPrice) and gasLimit similarly), causing inconsistency
with other transactions; replace the hardcoded 3000000 values for gas and
gasLimit with the configured value (self.chainConfig.gas) and convert it to hex
using this.web3.utils.toHex(self.chainConfig.gas) so it matches the other usages
(see where self.chainConfig.gas is used around lines ~395).
In `@config/local.json`:
- Around line 9-11: The CORS origins array under the "cors" key in
config/local.json is currently on a single line; reformat it so each origin is
on its own line for readability and easier diffs—locate the "cors" JSON array
and split entries onto separate lines with proper JSON commas and indentation
(one origin per line inside the array) while preserving the existing origin
strings and valid JSON syntax.
In `@helpers/ledgerWeb3Provider.js`:
- Around line 145-148: The promise catch currently calls
callback(toWalletError(error)) which violates Node.js (err, result) convention;
update the catch handler in the run().then(...).catch(...) chain to call
callback(toWalletError(error), null) (or undefined) so the second argument is
explicitly provided, referencing the existing run() promise chain, the callback
invocation, and the toWalletError conversion to locate the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 416d03e7-9c79-4fc9-b47b-732aed1e9c2c
📒 Files selected for processing (21)
apis/candidates.jsapis/ipfs.jsapp/app.jsapp/components/Setting.vueapp/components/candidates/Apply.vuebuild/app.0cda748de7cad1273504.jsbuild/app.d6ee356cd701e09ed0fb.jsbuild/index.htmlbuild/node-vendor.27b07d58b842f5b9d966.jsbuild/node-vendor.27b07d58b842f5b9d966.js.LICENSE.txtbuild/node-vendor.d4d835193df6f3eb9196.jsbuild/runtime.04647efee1a41c5e77ac.jsbuild/runtime.f6886b9416edacf69bd3.jsbuild/style.67d0ad01ba802b0f47f8.jsbuild/style.9b2a3757cae03d13690f.jsbuild/vendor.a4b84a6e0d4780dac5e3.jsbuild/vendor.d9e3b7255f6258bc8ca1.jsconfig/local.jsonhelpers/ledgerWeb3Provider.jshelpers/personalSign.jshelpers/walletError.js
💤 Files with no reviewable changes (2)
- build/runtime.04647efee1a41c5e77ac.js
- build/vendor.d9e3b7255f6258bc8ca1.js
Comment on lines
23
to
29
| function normalizeValue (value) { | ||
| if (value === undefined || value === null) return '' | ||
| if (Array.isArray(value)) { | ||
| value = value[0] | ||
| } | ||
| return String(value).trim().replace(/^["']|["']$/g, '') | ||
| } |
There was a problem hiding this comment.
Empty array case returns "undefined" string.
When value is an empty array, value[0] is undefined, and String(undefined) produces the literal string "undefined" which will pass through normalization.
Proposed fix
function normalizeValue (value) {
if (value === undefined || value === null) return ''
if (Array.isArray(value)) {
+ if (value.length === 0) return ''
value = value[0]
}
+ if (value === undefined || value === null) return ''
return String(value).trim().replace(/^["']|["']$/g, '')
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function normalizeValue (value) { | |
| if (value === undefined || value === null) return '' | |
| if (Array.isArray(value)) { | |
| value = value[0] | |
| } | |
| return String(value).trim().replace(/^["']|["']$/g, '') | |
| } | |
| function normalizeValue (value) { | |
| if (value === undefined || value === null) return '' | |
| if (Array.isArray(value)) { | |
| if (value.length === 0) return '' | |
| value = value[0] | |
| } | |
| if (value === undefined || value === null) return '' | |
| return String(value).trim().replace(/^["']|["']$/g, '') | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apis/ipfs.js` around lines 23 - 29, In normalizeValue the Array.isArray
branch uses value[0] which becomes undefined for empty arrays and yields the
string "undefined"; change the array handling in normalizeValue (function
normalizeValue) to check for an empty array and return '' (or set value = ''),
otherwise assign value = value[0]; also ensure that after selecting the first
element you treat null/undefined by returning '' before calling
String(...).trim().replace(...) so empty arrays and null/undefined first
elements do not become the string "undefined".
Comment on lines
+570
to
+585
| const network = this.NetworkProvider || | ||
| this.provider || | ||
| store.get('network') | ||
| let signerAccount | ||
| if (network === 'ledger' || network === 'trezor') { | ||
| signerAccount = this.toRpcAddress( | ||
| await this.getAccount() || | ||
| this.account || | ||
| store.get('address') || | ||
| '' | ||
| ).toLowerCase() | ||
| } else { | ||
| signerAccount = this.toRpcAddress( | ||
| this.account || store.get('address') || (await this.getAccount()) || '' | ||
| ).toLowerCase() | ||
| } |
There was a problem hiding this comment.
Inconsistent account resolution order between hardware and standard wallets.
For hardware wallets, getAccount() is called first before falling back to this.account. For standard wallets, the order is reversed. This asymmetry could lead to different accounts being used for the same user session.
Proposed fix: Use consistent resolution order
const network = this.NetworkProvider ||
this.provider ||
store.get('network')
let signerAccount
- if (network === 'ledger' || network === 'trezor') {
- signerAccount = this.toRpcAddress(
- await this.getAccount() ||
- this.account ||
- store.get('address') ||
- ''
- ).toLowerCase()
- } else {
- signerAccount = this.toRpcAddress(
- this.account || store.get('address') || (await this.getAccount()) || ''
- ).toLowerCase()
- }
+ signerAccount = this.toRpcAddress(
+ this.account ||
+ store.get('address') ||
+ (await this.getAccount()) ||
+ ''
+ ).toLowerCase()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const network = this.NetworkProvider || | |
| this.provider || | |
| store.get('network') | |
| let signerAccount | |
| if (network === 'ledger' || network === 'trezor') { | |
| signerAccount = this.toRpcAddress( | |
| await this.getAccount() || | |
| this.account || | |
| store.get('address') || | |
| '' | |
| ).toLowerCase() | |
| } else { | |
| signerAccount = this.toRpcAddress( | |
| this.account || store.get('address') || (await this.getAccount()) || '' | |
| ).toLowerCase() | |
| } | |
| const network = this.NetworkProvider || | |
| this.provider || | |
| store.get('network') | |
| let signerAccount | |
| signerAccount = this.toRpcAddress( | |
| this.account || | |
| store.get('address') || | |
| (await this.getAccount()) || | |
| '' | |
| ).toLowerCase() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/components/candidates/Apply.vue` around lines 570 - 585, The account
resolution order is inconsistent between hardware and standard wallets (in
Apply.vue) causing different accounts to be chosen; standardize the lookup order
used to set signerAccount so both branches call getAccount() in the same
relative position. Update the logic around
NetworkProvider/provider/store.get('network') and the signerAccount assignment
(the branches that call this.toRpcAddress and await this.getAccount()) to use a
single, consistent resolution sequence (e.g., await this.getAccount() ||
this.account || store.get('address') || '') for both hardware
('ledger'|'trezor') and non-hardware paths so toRpcAddress and toLowerCase
operate on the same resolved value.
Comment on lines
+74
to
+87
| for (const recoveryId of recoveryIds) { | ||
| try { | ||
| const pubKey = ethUtil.ecrecover( | ||
| msgHash, | ||
| recoveryId, | ||
| sigBuf.slice(0, 32), | ||
| sigBuf.slice(32, 64) | ||
| ) | ||
| const recovered = ('0x' + ethUtil.pubToAddress(pubKey).toString('hex')).toLowerCase() | ||
| if (recovered) { | ||
| return recovered | ||
| } | ||
| } catch (e) {} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ethereumjs-util ecrecover function signature v parameter
💡 Result:
In ethereumjs-util, the ecrecover function’s v parameter is the signature recovery parameter (the “recovery id”) encoded in Ethereum’s v format, used together with r and s to recover the signer’s public key/address [1][2]. How v is interpreted - The API is ecrecover(msgHash, v, r, s, chainId?) where v is a number-like input for signature recovery [1]. - If v is invalid, ethereumjs-util throws “Invalid signature v value” [1]. - With an optional chainId argument, ethereumjs-util is designed to handle non-legacy v values: it converts the provided v into the underlying recovery value used by secp256k1 (which is effectively 0/1) [3]. Legacy vs EIP-155-style v - In many Ethereum signing utilities, you’ll see v derived as sig.recovery + 27 (i.e., legacy 27/28) [4]. - For EIP-155-style v values, the common relation shown is chain_id = (v − 35) / 2 [4], and ethereumjs-util’s optional chainId parameter is what enables it to interpret those v values correctly [3].
Citations:
- 1: https://github.com/ethereumjs/ethereumjs-util/blob/0ac965f884ab595398fdd9d05e9386d75ce57db9/src/signature.ts
- 2: https://github.com/ethereumjs/ethereumjs-util/blob/master/docs/modules/_signature_.md
- 3: ecrecover is problematically hardcoded to pre-EIP-155 transactions ethereumjs/ethereumjs-monorepo#2743
- 4: https://ethereum.stackexchange.com/questions/2256/ethereum-ecrecover-signature-verification-and-encryption
Fix ethereumjs-util.ecrecover v argument in helpers/personalSign.js
recoverPersonalSignAddress passes recoveryId (0/1) as the v parameter to ethUtil.ecrecover(msgHash, v, r, s). In ethereumjs-util, ecrecover expects legacy v values (27/28) when no chainId is provided, so 0/1 will be rejected (caught) and the manual recovery won’t work correctly.
Proposed fix
for (const recoveryId of recoveryIds) {
try {
const pubKey = ethUtil.ecrecover(
msgHash,
- recoveryId,
+ recoveryId + 27,
sigBuf.slice(0, 32),
sigBuf.slice(32, 64)
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const recoveryId of recoveryIds) { | |
| try { | |
| const pubKey = ethUtil.ecrecover( | |
| msgHash, | |
| recoveryId, | |
| sigBuf.slice(0, 32), | |
| sigBuf.slice(32, 64) | |
| ) | |
| const recovered = ('0x' + ethUtil.pubToAddress(pubKey).toString('hex')).toLowerCase() | |
| if (recovered) { | |
| return recovered | |
| } | |
| } catch (e) {} | |
| } | |
| for (const recoveryId of recoveryIds) { | |
| try { | |
| const pubKey = ethUtil.ecrecover( | |
| msgHash, | |
| recoveryId + 27, | |
| sigBuf.slice(0, 32), | |
| sigBuf.slice(32, 64) | |
| ) | |
| const recovered = ('0x' + ethUtil.pubToAddress(pubKey).toString('hex')).toLowerCase() | |
| if (recovered) { | |
| return recovered | |
| } | |
| } catch (e) {} | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helpers/personalSign.js` around lines 74 - 87, The ecrecover call in
recoverPersonalSignAddress is passing recoveryId (0/1) as v but ethereumjs-util
expects legacy v (27/28), so change the code to pass recoveryId + 27 (ensure
it's a Number) when calling ethUtil.ecrecover(msgHash, v, r, s); locate the loop
using recoveryId, msgHash, sigBuf and replace the v argument with
(Number(recoveryId) + 27) and keep r as sigBuf.slice(0,32) and s as
sigBuf.slice(32,64), preserving the try/catch and return behavior.
Comment on lines
+62
to
+68
| function toWalletError (error) { | ||
| if (error instanceof Error) { | ||
| error.message = formatWalletError(error) | ||
| return error | ||
| } | ||
| return new Error(formatWalletError(error)) | ||
| } |
There was a problem hiding this comment.
Mutating original Error.message can cause unexpected behavior.
The function mutates the message property of the original Error object. If the same error instance is caught and formatted multiple times (e.g., logged then re-thrown), subsequent handlers see the already-formatted message, potentially losing context or double-formatting.
Proposed fix: clone instead of mutate
function toWalletError (error) {
if (error instanceof Error) {
- error.message = formatWalletError(error)
- return error
+ const formatted = new Error(formatWalletError(error))
+ formatted.stack = error.stack
+ return formatted
}
return new Error(formatWalletError(error))
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helpers/walletError.js` around lines 62 - 68, toWalletError currently mutates
the original Error.message which can double-format or lose context; instead,
create and return a new Error instance that uses formatWalletError(error) for
its message while preserving relevant properties (stack, name, and any custom
fields) from the original error; update the toWalletError function to construct
a cloned Error (copy stack/name/custom props) and set the formatted message
rather than modifying the passed-in Error; reference formatWalletError and
toWalletError when making the change.
rajshah1609
approved these changes
May 25, 2026
rajshah1609
requested review from
AnilChinchawale,
RushabhParmar12 and
rajshah1609
rajshah1609
previously approved these changes
May 25, 2026
rajshah1609
previously approved these changes
May 26, 2026
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)
app/components/candidates/Apply.vue (2)
610-620:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop echoing the signed KYC auth tuple into query params and headers.
These fields are already present in the multipart body, and
apis/ipfs.jsreadsreq.bodybefore falling back to headers/query. Sending the same signed payload again viaparamsexposes a replayable auth tuple to URL logs and intermediaries for no functional gain. Keep it in the body only.Suggested change
- const { data } = await axios.post('/api/ipfs/addKYC', formData, { - params: { - account: signerAccount, - message: message, - signedMessage: signedMessage - }, - headers: { - 'x-kyc-account': signerAccount, - 'x-kyc-message': message, - 'x-kyc-signature': signedMessage - } - }) + const { data } = await axios.post('/api/ipfs/addKYC', formData)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/candidates/Apply.vue` around lines 610 - 620, The axios.post call to '/api/ipfs/addKYC' in Apply.vue is duplicating the signed auth tuple (signerAccount, message, signedMessage) into params and headers; remove the params and headers objects that set account/message/signedMessage and the 'x-kyc-*' headers so the signed payload is only sent inside the multipart formData body (formData already contains signerAccount, message, signedMessage). Leave the formData and the axios.post call intact otherwise so apis/ipfs.js continues to read req.body.
637-642:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
signerAccountas the transaction sender in the standard-wallet path.
uploadKYC(data.hash)only carries the hash, so the address written on-chain is determined byfrom. Here the upload is signed forsignerAccount, and Line 645 checks status for that same address, but the transaction is sent from a separately resolvedfromAccount. Ifthis.accountorstore.get('address')is stale after an account switch, the upload is authorized for one address and recorded on-chain for another.Suggested change
- const fromAccount = (this.account || store.get('address') || '').toLowerCase() - const txParams = Object.assign({ from: fromAccount }, txOptions) + const txParams = Object.assign({ from: signerAccount }, txOptions) await this.sendContractTransaction( contract.methods.uploadKYC(data.hash), txParams )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/candidates/Apply.vue` around lines 637 - 642, The transaction is being sent with a potentially stale fromAccount even though the upload is signed by signerAccount; update the txParams used in sendContractTransaction so the from field uses signerAccount instead of the resolved fromAccount in the standard-wallet path. Locate the upload call (contract.methods.uploadKYC(data.hash)) and the txParams creation (currently Object.assign({ from: fromAccount }, txOptions)), replace or override that from value with signerAccount (the same account used to sign and later checked) so sendContractTransaction and the on-chain write use the same address as signerAccount.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/components/candidates/Apply.vue`:
- Around line 610-620: The axios.post call to '/api/ipfs/addKYC' in Apply.vue is
duplicating the signed auth tuple (signerAccount, message, signedMessage) into
params and headers; remove the params and headers objects that set
account/message/signedMessage and the 'x-kyc-*' headers so the signed payload is
only sent inside the multipart formData body (formData already contains
signerAccount, message, signedMessage). Leave the formData and the axios.post
call intact otherwise so apis/ipfs.js continues to read req.body.
- Around line 637-642: The transaction is being sent with a potentially stale
fromAccount even though the upload is signed by signerAccount; update the
txParams used in sendContractTransaction so the from field uses signerAccount
instead of the resolved fromAccount in the standard-wallet path. Locate the
upload call (contract.methods.uploadKYC(data.hash)) and the txParams creation
(currently Object.assign({ from: fromAccount }, txOptions)), replace or override
that from value with signerAccount (the same account used to sign and later
checked) so sendContractTransaction and the on-chain write use the same address
as signerAccount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6dc87227-e0a1-43a9-81b4-802d3cb420cb
📒 Files selected for processing (1)
app/components/candidates/Apply.vue
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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
2 participants
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.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores