Main - #64
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds a new enterprise API (key issuance and authenticated PDF KYC upload to IPFS) with a supporting Mongoose model, adds a voter balance-lookup endpoint used by client wallet balance fallback, adds PDF-only validation to an existing IPFS upload route, and includes unrelated formatting refactors, an RPC URL fallback fix, and a crawler DB lookup removal. ChangesEnterprise Credentialing and KYC Submission
Estimated code review effort: 3 (Moderate) | ~25 minutes Voter Balance Endpoint and Client Integration
Estimated code review effort: 2 (Simple) | ~10 minutes Unrelated Fixes and Refactors
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant EnterpriseRouter
participant EnterpriseKeyDB
participant IPFS
Client->>EnterpriseRouter: POST /keys (Bearer masterToken, enterpriseName)
EnterpriseRouter->>EnterpriseRouter: validate token & enterpriseName
EnterpriseRouter->>EnterpriseKeyDB: store apiKey/apiSecret
EnterpriseRouter-->>Client: 201 { apiKey, apiSecret }
Client->>EnterpriseRouter: POST /addKYC (x-api-key, timestamp, signature, PDF file)
EnterpriseRouter->>EnterpriseKeyDB: fetch secret by apiKey
EnterpriseKeyDB-->>EnterpriseRouter: apiSecret
EnterpriseRouter->>EnterpriseRouter: verify signature & timestamp
EnterpriseRouter->>EnterpriseRouter: validate PDF type/size
EnterpriseRouter->>IPFS: upload file
IPFS-->>EnterpriseRouter: content hash
EnterpriseRouter-->>Client: 200 { hash }
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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: 11
🧹 Nitpick comments (2)
crawl.js (1)
196-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove dead code instead of commenting it out.
The
candateInDBlookup was already unused in this function (newStatus/ownerare derived from theprevStatus/prevOwnerparameters, not this variable), so commenting it out has no functional effect. Delete it outright rather than leaving commented-out code.♻️ Proposed cleanup
if (candidate !== 'xdc0000000000000000000000000000000000000000') { - // check current status - // const candateInDB = await db.Candidate.findOne({ - // smartContractAddress: config.get('blockchain.validatorAddress'), - // candidate: candidate - // }) || {} - let newStatus = prevStatus || 'STANDBY'🤖 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 `@crawl.js` around lines 196 - 199, Remove the dead commented-out Candidate lookup in crawl.js rather than leaving it in place; the unused candateInDB query is no longer needed in this flow because newStatus and owner come from prevStatus and prevOwner. Delete the commented block from the function that handles the candidate status update so the code stays clean and the remaining logic is easier to follow.apis/voters.js (1)
28-58: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant RPC round-trip when the first lookup already succeeds.
Both
web3.eth.getBalance(rpcAddr)andweb3.eth.getBalance(xdcAddr)are called unconditionally, even when the first call already returns a valid non-zero balance. This doubles RPC load per request for the common case.♻️ Short-circuit the second call when the first already returned a usable value
try { balance0x = await web3.eth.getBalance(rpcAddr) success = true } catch (e) {} - try { - balanceXdc = await web3.eth.getBalance(xdcAddr) - success = true - } catch (e) {} + if (!balance0x || balance0x === '0' || balance0x === '0x0') { + try { + balanceXdc = await web3.eth.getBalance(xdcAddr) + success = true + } 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 `@apis/voters.js` around lines 28 - 58, The getBalance route in voters.js always performs both web3.eth.getBalance calls even after the first one succeeds, causing an unnecessary extra RPC round-trip. Update the balance lookup logic in the router.get('/getBalance/:address') handler to short-circuit after the first successful, usable result by checking balance0x before querying xdcAddr, while keeping the existing fallback and error handling behavior in place.
🤖 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/enterprise.js`:
- Around line 102-106: The HMAC validation in enterprise.js is currently bound
only to method, path, and timestamp, which allows a valid signature to be
replayed with a different uploaded PDF within the timestamp window. Update the
signature flow around stringToSign to incorporate a digest of the request body
(or uploaded file content) and, if helpful, file metadata such as filename and
size; also add a nonce or idempotency key to prevent replay. Make sure both the
signature generation and the verification logic in the same request validation
path use the same expanded signed payload.
- Around line 142-144: The KYC upload flow in the enterprise handler is logging
the IPFS content hash, which should not be emitted to application logs. Update
the logic in the enterprise KYC upload path so the authenticated response still
returns the hash from the upload result, but remove or redact it from the
console output in the KYC handler. Use the existing upload/responder code around
the hash assignment and response send to keep the behavior unchanged except for
logging.
- Around line 124-127: The file validation in the uploaded-file check is too
permissive because the `&&` in the `enterprise.js` PDF guard allows a file
through if either the MIME type or filename looks like PDF; update the condition
in the upload handler to require both checks to pass by using `||` for
rejection, and if possible strengthen the `uploadedFile` validation further by
verifying the PDF magic bytes in addition to the existing MIME/extension checks.
- Around line 20-24: The IPFS upload in axios.post within the addKYC flow has no
timeout, so a stalled endpoint can hang the request indefinitely. Add an
explicit timeout to the axios.post call in the IPFS upload path and keep the
existing headers/maxBodyLength/maxContentLength settings intact. Use the
axios.post invocation in the enterprise.js flow as the place to apply the
timeout so the /addKYC request fails cleanly instead of hanging.
- Around line 113-114: The signature check in the enterprise API path uses a
plain string comparison, so update the logic around the
`apiSignature`/`expectedSignature` check to use constant-time HMAC verification
instead. Decode both hex digests, reject malformed or length-mismatched values
before comparing, and use `crypto.timingSafeEqual` in the `unauthorized(res,
'invalid_signature')` flow to preserve the existing failure behavior.
In `@apis/ipfs.js`:
- Around line 157-160: The PDF validation in the IPFS upload flow is too
permissive because the check in the upload handler allows a file through if
either the MIME type or filename matches. Update the condition in the imageFile
validation so it rejects unless both PDF checks pass, and consider adding a PDF
magic-bytes check in the same upload path before the IPFS upload to further
verify the file content.
In `@apis/voters.js`:
- Around line 38-46: The RPC balance lookups in Web3RpcInternal() currently
swallow failures in the two getBalance try/catch blocks, and the HttpProvider is
created without any timeout. Update the catch blocks to log the error details
for both rpcAddr and xdcAddr balance fetches, and configure the Web3
HttpProvider with a timeout so hung or unreachable RPC nodes fail fast instead
of leaving the endpoint silent or blocked.
In `@app/app.js`:
- Line 1483: The signature assignment in the app.js logic has an
operator-precedence bug: the fallback in `result = '0x' + sig.payload.signature
|| ''` never applies. Update the code around the signature formatting path to
explicitly guard `sig.payload.signature` before concatenation, using the
relevant result-building logic where `sig` and `sig.payload.signature` are
handled, so missing values don’t turn into a truthy `"0xundefined"` string.
- Around line 1461-1482: The ledger and trezor branches in the message-signing
switch declare const/let variables directly inside case clauses, which triggers
noSwitchDeclarations and can cause scope/TDZ issues. Wrap the bodies of the
'ledger' and 'trezor' cases in their own blocks, following the same pattern used
by getAccount and detectNetwork, and keep the existing signPersonalMessage,
toHexBuffer, and TrezorConnect.ethereumSignMessage logic inside those blocks.
- Around line 843-852: The getBalanceSafe helper currently makes an axios GET to
/api/voters/getBalance/... without any timeout, which can block the fallback
path if the server or RPC stalls. Update Vue.prototype.getBalanceSafe to pass a
timeout on the axios request so the catch path can run promptly, and keep the
existing fallback logging/context behavior intact.
In `@models/mongodb/enterpriseKey.js`:
- Around line 13-15: The enterprise key model’s apiSecret field is currently
stored and returned in plaintext, so update the EnterpriseKey schema/model to
encrypt and decrypt this value transparently while keeping it excluded from
default projections. Add the necessary schema hooks or getters/setters around
apiSecret in enterpriseKey.js, and make sure any code that reads it for HMAC
verification explicitly opts in to selecting the field when needed.
---
Nitpick comments:
In `@apis/voters.js`:
- Around line 28-58: The getBalance route in voters.js always performs both
web3.eth.getBalance calls even after the first one succeeds, causing an
unnecessary extra RPC round-trip. Update the balance lookup logic in the
router.get('/getBalance/:address') handler to short-circuit after the first
successful, usable result by checking balance0x before querying xdcAddr, while
keeping the existing fallback and error handling behavior in place.
In `@crawl.js`:
- Around line 196-199: Remove the dead commented-out Candidate lookup in
crawl.js rather than leaving it in place; the unused candateInDB query is no
longer needed in this flow because newStatus and owner come from prevStatus and
prevOwner. Delete the commented block from the function that handles the
candidate status update so the code stays clean and the remaining logic is
easier to follow.
🪄 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: c94fb99c-be00-438b-8dcf-05346ec71494
📒 Files selected for processing (9)
apis/enterprise.jsapis/index.jsapis/ipfs.jsapis/voters.jsapp/app.jsapp/components/candidates/Apply.vuecrawl.jsmodels/blockchain/web3rpc.jsmodels/mongodb/enterpriseKey.js
| try { | ||
| balance0x = await web3.eth.getBalance(rpcAddr) | ||
| success = true | ||
| } catch (e) {} | ||
|
|
||
| try { | ||
| balanceXdc = await web3.eth.getBalance(xdcAddr) | ||
| success = true | ||
| } catch (e) {} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== apis/voters.js ==\n'
wc -l apis/voters.js
sed -n '1,120p' apis/voters.js
printf '\n== models/blockchain/web3rpc.js ==\n'
wc -l models/blockchain/web3rpc.js
sed -n '1,220p' models/blockchain/web3rpc.js
printf '\n== search for Web3RpcInternal and timeout references ==\n'
rg -n "Web3RpcInternal|timeout|request timeout|rpc timeout|AbortSignal|AbortController" -S .Repository: XinFinOrg/MasterNode-App
Length of output: 6803
🌐 Web query:
xdc3 HttpProvider timeout options default request timeout Web3.providers.HttpProvider
💡 Result:
In XDC3 (and its underlying web3.js-based architecture), you can configure the request timeout for an HttpProvider by passing a timeout option (in milliseconds) within the provider's options object during instantiation [1]. To set a custom timeout, include it in the options object passed to the XDC3ProvidersHttp constructor [1]: var Web3HttpProvider = require('xdc3-providers-http'); var options = { timeout: 20000, // Timeout in milliseconds keepAlive: true, //... other options }; var provider = new Web3HttpProvider('http://localhost:8545', options); Important distinctions to note: 1. Provider Timeout: The timeout option in HttpProviderOptions refers to the duration (in milliseconds) the HTTP request itself will wait before timing out [1]. 2. Transaction Polling Timeout: There is a separate setting called transactionPollingTimeout available on the web3.eth module [2][3]. This defines the number of seconds the library waits for a transaction receipt to be confirmed on the network when using HTTP connections [2][3]. Its default value is 750 seconds [2][3]. It can be adjusted globally or on contract instances: web3.eth.transactionPollingTimeout = 1000; // Sets the timeout to 1000 seconds [2] Ensure you do not confuse the provider's connection timeout with the transaction polling timeout, as they serve different purposes within the library [2][1].
Citations:
- 1: https://github.com/XinFinOrg/XDC3/blob/master/docs/include_package-core.rst
- 2: https://github.com/XinFinOrg/XDC3/blob/master/docs/web3-eth.rst
- 3: https://github.com/XinFinOrg/XDC3/blob/master/docs/web3-eth-contract.rst
Log RPC failures and set a timeout. Both catch blocks swallow errors, and Web3RpcInternal() instantiates HttpProvider without a timeout, so RPC outages or a hung node can leave this endpoint silent or stuck indefinitely.
🤖 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/voters.js` around lines 38 - 46, The RPC balance lookups in
Web3RpcInternal() currently swallow failures in the two getBalance try/catch
blocks, and the HttpProvider is created without any timeout. Update the catch
blocks to log the error details for both rpcAddr and xdcAddr balance fetches,
and configure the Web3 HttpProvider with a timeout so hung or unreachable RPC
nodes fail fast instead of leaving the endpoint silent or blocked.
…and configuration files
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor