fix(escrow): correct paisa-to-MATIC conversion — remove 10,000x magnitude error#621
Conversation
…_PAISA env var bid_amount is stored in paisa (1 INR = 100 paisa). The previous code did bid_amount / 100 to get INR, then passed that number directly to ethers.parseEther() as if it were an ETH/MATIC value — a 10,000x magnitude error on every funded order. Replace with a configurable ESCROW_MATIC_PER_PAISA rate. If the env var is not set, the deposit is skipped with a warning (same behaviour as a missing wallet address). A MAX_ESCROW_MATIC safety cap rejects any deposit that exceeds the configured ceiling before it hits the chain. Document both variables in .env.example.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe escrow deposit logic in the bid-acceptance route is updated to compute the on-chain MATIC amount using a configurable ChangesEscrow Deposit Amount Fix and Config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
backend
|
🎉 Thank you for your contribution! Your pull request has been received and will be reviewed shortly. If you enjoy the project, please consider giving the repository a ⭐. You can also follow my GitHub profile to stay updated on future open-source projects. Thanks for being part of the community! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/api/src/routes/orderRoutes.js (1)
672-697: 🏗️ Heavy liftAdd regression tests for the new escrow conversion and failure paths.
This block changes critical money-flow behavior. Please add route-level tests for: (1) missing/invalid
ESCROW_MATIC_PER_PAISAskip path, (2) cap breach 400 path, and (3)escrowDepositreturning notxHash-> 500 path.🤖 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 `@backend/api/src/routes/orderRoutes.js` around lines 672 - 697, Add route-level regression tests for the escrow deposit logic to cover the three critical paths: first, test the scenario where ESCROW_MATIC_PER_PAISA is missing or invalid to verify the skip path with logger.warn is triggered; second, test the scenario where the computed maticAmount exceeds maxEscrowMatic to verify a 400 status response is returned; third, test the scenario where the escrowDeposit function completes successfully with a txHash to verify escrowTxHash is captured, and test when escrowDeposit returns without a txHash to verify a 500 status response is returned. These tests should use mocked environment variables and the escrowDeposit function to validate the money-flow behavior changes.
🤖 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 `@backend/api/.env.example`:
- Around line 42-43: The ESCROW_MATIC_PER_PAISA configuration in the
.env.example file is set to a real live exchange rate value (0.000004), which
contradicts the comment on the previous line stating that leaving it unset
disables escrow deposits. When teams copy this example file as-is, escrow
deposits will be enabled with a potentially stale rate. Remove or comment out
the value for ESCROW_MATIC_PER_PAISA on line 43 to leave it unset by default,
ensuring teams that use the example configuration will have deposits disabled
until they explicitly set a current exchange rate.
In `@backend/api/src/routes/orderRoutes.js`:
- Around line 677-680: The parseFloat call on MAX_ESCROW_MATIC environment
variable is not validated, so if the env value is invalid or non-numeric, it
will parse to NaN. When NaN is compared with maticAmount in the safety check,
the comparison always evaluates to false, disabling the safety cap. After
assigning maxEscrowMatic on line 677, add validation to check if the parsed
value is NaN using Number.isNaN() or by checking isFinite(), and either use a
safe default fallback value or handle the invalid configuration appropriately
before proceeding to the comparison check on line 678.
---
Nitpick comments:
In `@backend/api/src/routes/orderRoutes.js`:
- Around line 672-697: Add route-level regression tests for the escrow deposit
logic to cover the three critical paths: first, test the scenario where
ESCROW_MATIC_PER_PAISA is missing or invalid to verify the skip path with
logger.warn is triggered; second, test the scenario where the computed
maticAmount exceeds maxEscrowMatic to verify a 400 status response is returned;
third, test the scenario where the escrowDeposit function completes successfully
with a txHash to verify escrowTxHash is captured, and test when escrowDeposit
returns without a txHash to verify a 500 status response is returned. These
tests should use mocked environment variables and the escrowDeposit function to
validate the money-flow behavior changes.
🪄 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 Plus
Run ID: 13ba4c04-732f-432d-8dac-027958903c2e
📒 Files selected for processing (2)
backend/api/.env.examplebackend/api/src/routes/orderRoutes.js
There was a problem hiding this comment.
Pull request overview
Fixes escrow deposit amount calculation during bid acceptance by replacing the incorrect paisa→INR→parseEther() flow (which treated INR as MATIC) with an explicit, configurable paisa→MATIC exchange rate and a hard safety cap, and documents the new configuration.
Changes:
- Update bid-accept escrow funding to use
ESCROW_MATIC_PER_PAISA(paisa-granularity exchange rate) instead of(bid_amount/100)interpreted as MATIC. - Add
MAX_ESCROW_MATICguardrail to reject deposits above a configured ceiling before sending an on-chain transaction. - Document escrow configuration in
backend/api/.env.example.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| backend/api/src/routes/orderRoutes.js | Reworks escrow deposit amount computation to be exchange-rate driven and adds a max-cap check plus improved failure handling. |
| backend/api/.env.example | Documents ESCROW_MATIC_PER_PAISA and MAX_ESCROW_MATIC and provides an example configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nv.example NaN from a misconfigured MAX_ESCROW_MATIC would make the cap comparison always false, silently disabling the safety check. Now explicitly validate the parsed value and fail closed with 500 if it is invalid. Clear the ESCROW_MATIC_PER_PAISA default value in .env.example — shipping a concrete exchange rate causes teams that copy the file as-is to enable live deposits with a potentially stale rate, contradicting the adjacent comment that leaving it unset disables escrow.
|
CI failures are pre-existing — backend test failures (expected 500, got 422) exist on |
# Conflicts: # backend/api/src/routes/orderRoutes.js
KanishJebaMathewM
merged commit 4bb998b
into
KanishJebaMathewM:main
|
🎉 Thank you for your contribution! Your pull request has been merged successfully. We appreciate your work and look forward to your future contributions. 🚀 |
Closes #606
What was broken
bid_amountis stored in paisa (1 INR = 100 paisa, documented inlib/pricing.js). The accept-bid handler was doing:bid_amount / 100converts paisa to INR. Thenethers.parseEther(INR_value)treats that number as MATIC. A ₹5,000 bid (bid_amount=500000) would lock 5,000 MATIC (~$2,000) on-chain for a $60 order.Fix
Replaced with a configurable
ESCROW_MATIC_PER_PAISAenv var that holds the explicit INR→MATIC exchange rate at the paisa granularity. If the var is not set the deposit is skipped with a warning (same existing behaviour as a missing wallet address). AMAX_ESCROW_MATICcap rejects any deposit above the configured ceiling before it reaches the chain.Both variables are documented in
.env.example.Summary by CodeRabbit
New Features
Bug Fixes