Escrow management#518
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a new escrow management dashboard at ChangesEscrow Dashboard and Payment Flow Consolidation
Sequence Diagram(s)sequenceDiagram
rect rgba(100, 149, 237, 0.5)
Note over EscrowPage,OceanProvider: Page Load
EscrowPage->>useEscrowData: mount → load()
useEscrowData->>OceanProvider: getUserFundsDetailed(token, user)
OceanProvider-->>useEscrowData: available, locked balances
useEscrowData->>nodeService: getEscrowEvents(Auth, payer)
nodeService-->>useEscrowData: EscrowEvent[] (spender addresses)
useEscrowData->>OceanProvider: getLocks(token, payer, spender)
OceanProvider-->>useEscrowData: EscrowLock[]
useEscrowData-->>EscrowPage: tokens, spenders, loading, reload
end
rect rgba(144, 238, 144, 0.5)
Note over EscrowTokenPanel,OceanProvider: Create Authorization
EscrowTokenPanel->>CreateAuthorizationModal: open(tokenAddress, existingConsumers)
CreateAuthorizationModal->>nodeService: GET /api/services/nodes (nodeId)
nodeService-->>CreateAuthorizationModal: node consumerAddress
CreateAuthorizationModal->>useAuthorizeTokens: handleAuthorize(tokenAddress, peerId, spender, lockParams)
useAuthorizeTokens->>OceanProvider: authorize on-chain
OceanProvider-->>useAuthorizeTokens: success
useAuthorizeTokens-->>CreateAuthorizationModal: toast(authorizationSuccessMessage)
CreateAuthorizationModal-->>EscrowTokenPanel: onSuccess → reload()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
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📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/run-job/payment-summary.tsx (1)
31-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOperator precedence bug:
NaNcomparison always returnsfalse.When
authorizationsisnullormaxLockedAmountisundefined,Number(undefined)returnsNaN. SinceNaN ?? 0still yieldsNaN(nullish coalescing doesn't catchNaN), the comparisonNaN < totalCostis alwaysfalse—so the "Insufficient allowance" chip will never appear when authorization data is missing.Move the
?? 0inside theNumber()call, consistent with lines 54 and 63:🐛 Proposed fix
- const insufficientAutorized = (Number(authorizations?.maxLockedAmount) ?? 0) < totalCost; + const insufficientAuthorized = Number(authorizations?.maxLockedAmount ?? 0) < totalCost;Also note the typo fix:
insufficientAutorized→insufficientAuthorized.Update the references on lines 64 and 68 accordingly.
🤖 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 `@src/components/run-job/payment-summary.tsx` at line 31, The nullish coalescing operator ?? 0 is being applied after Number() is called, which means NaN is not caught by the operator since NaN is not a nullish value. In the insufficientAutorized variable declaration, move the ?? 0 inside the Number() call so it becomes Number(authorizations?.maxLockedAmount ?? 0) to ensure undefined values are converted to 0 before the numeric comparison. Additionally, fix the typo in the variable name from insufficientAutorized to insufficientAuthorized and update all references to this variable on lines 64 and 68 to use the corrected spelling.
🤖 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 `@src/components/escrow/escrow-token-panel.tsx`:
- Around line 199-202: The form reset in the onSubmit callbacks always clears
user input regardless of transaction success or failure. Modify the
handleDeposit and handleWithdraw functions (from use-deposit-tokens.ts and
use-withdraw-tokens.ts respectively) to return a boolean value (true on
successful transaction, false when errors are handled internally) instead of
always resolving. Then update the onSubmit callbacks to conditionally call
formik.resetForm() only when handleDeposit or handleWithdraw returns true,
ensuring user input is preserved on failed transaction attempts for better retry
UX.
In `@src/lib/use-escrow-data.ts`:
- Around line 57-59: The early return condition in the escrow data hook when
ocean or account.address is unavailable does not clear the previous escrow state
(tokens and spenders), causing stale escrow data to persist after wallet
disconnect or account loss. Before the early return when !ocean ||
!account?.address is true, reset both the tokens and spenders state to empty
values (empty arrays or null as appropriate) to ensure no old wallet escrow data
renders after account context becomes unavailable.
- Around line 100-120: The Promise.all() call in the spenderInfos assignment
causes the entire spender refresh to fail if any single token/spender
authorization or locks call fails, resulting in stale spender state. Replace
Promise.all() with Promise.allSettled() to allow individual failures without
blocking the entire operation. Then update the filter logic to check each
result's status property and extract the value only from fulfilled results,
ensuring that partial failures are gracefully handled while still populating
spender information for successful calls.
In `@src/services/nodeService.ts`:
- Around line 69-73: The getEscrowEvents() function makes an unbounded fetch
call that lacks timeout protection and can hang indefinitely on slow or
unresponsive connections. Implement a timeout mechanism by creating an
AbortController instance, setting a timeout that aborts the controller after a
reasonable duration (e.g., 30 seconds), passing the controller's signal to the
fetch() call options, and catching any AbortError that results from the timeout
to provide appropriate error handling and user feedback in the dashboard.
---
Outside diff comments:
In `@src/components/run-job/payment-summary.tsx`:
- Line 31: The nullish coalescing operator ?? 0 is being applied after Number()
is called, which means NaN is not caught by the operator since NaN is not a
nullish value. In the insufficientAutorized variable declaration, move the ?? 0
inside the Number() call so it becomes Number(authorizations?.maxLockedAmount ??
0) to ensure undefined values are converted to 0 before the numeric comparison.
Additionally, fix the typo in the variable name from insufficientAutorized to
insufficientAuthorized and update all references to this variable on lines 64
and 68 to use the corrected spelling.
🪄 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: e75c85a4-4c4a-49b7-abc4-44ce316a5252
📒 Files selected for processing (26)
src/components/Navigation/profile-button.tsxsrc/components/escrow/authorization-form.module.csssrc/components/escrow/authorization-form.tsxsrc/components/escrow/create-authorization-modal.tsxsrc/components/escrow/edit-authorization-modal.tsxsrc/components/escrow/escrow-page.module.csssrc/components/escrow/escrow-page.tsxsrc/components/escrow/escrow-token-panel.module.csssrc/components/escrow/escrow-token-panel.tsxsrc/components/run-job/payment-authorize.tsxsrc/components/run-job/payment-summary.tsxsrc/components/token-amount/token-amount.module.csssrc/components/token-amount/token-amount.tsxsrc/components/withdraw/withdraw-page.module.csssrc/components/withdraw/withdraw-page.tsxsrc/components/withdraw/withdraw.module.csssrc/components/withdraw/withdraw.tsxsrc/lib/ocean-provider.tssrc/lib/use-deposit-tokens.tssrc/lib/use-escrow-data.tssrc/lib/use-withdraw-tokens.tssrc/pages/profile/escrow.tsxsrc/pages/withdraw.tsxsrc/services/nodeService.tssrc/types/payment.tssrc/utils/formatters.ts
💤 Files with no reviewable changes (5)
- src/components/withdraw/withdraw.tsx
- src/components/withdraw/withdraw-page.module.css
- src/components/withdraw/withdraw.module.css
- src/pages/withdraw.tsx
- src/components/withdraw/withdraw-page.tsx
| const response = await fetch(url); | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to fetch escrow events: ${response.status}`); | ||
| } | ||
| return (await response.json()) as EscrowEvent[]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether getEscrowEvents has explicit timeout/cancellation handling.
rg -n -C3 'getEscrowEvents|fetch\(|AbortController|signal:' src/services/nodeService.tsRepository: oceanprotocol/nodes-dashboard
Length of output: 714
🏁 Script executed:
#!/bin/bash
# Get full getEscrowEvents function and check line count of the file
wc -l src/services/nodeService.ts
echo "---"
# Read the complete getEscrowEvents function (lines 45-80 to capture full scope)
sed -n '45,80p' src/services/nodeService.tsRepository: oceanprotocol/nodes-dashboard
Length of output: 1406
🏁 Script executed:
#!/bin/bash
# Search for useEscrowData hook to understand how getEscrowEvents is called
rg -n 'useEscrowData|getEscrowEvents' src/ -A 5 -B 2Repository: oceanprotocol/nodes-dashboard
Length of output: 3345
🏁 Script executed:
#!/bin/bash
# Check if AbortController is used elsewhere in the codebase
rg -n 'AbortController|signal.*:' src/ -t ts -t tsxRepository: oceanprotocol/nodes-dashboard
Length of output: 101
🏁 Script executed:
#!/bin/bash
# Check for AbortController usage without type restriction
rg -n 'AbortController|signal.*:' src/Repository: oceanprotocol/nodes-dashboard
Length of output: 980
🏁 Script executed:
#!/bin/bash
# Read the complete useEscrowData implementation to see if timeout is handled there
cat -n src/lib/use-escrow-data.ts | head -120Repository: oceanprotocol/nodes-dashboard
Length of output: 5074
Add a request timeout to escrow-event fetches.
The getEscrowEvents() function at line 69 makes an unbounded fetch() call that can hang indefinitely on slow or broken node connections, stalling escrow data loading in the dashboard. Use AbortController with a timeout to prevent this.
Suggested patch
- const response = await fetch(url);
- if (!response.ok) {
- throw new Error(`Failed to fetch escrow events: ${response.status}`);
- }
- return (await response.json()) as EscrowEvent[];
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 10_000);
+ try {
+ const response = await fetch(url, { signal: controller.signal });
+ if (!response.ok) {
+ throw new Error(`Failed to fetch escrow events: ${response.status}`);
+ }
+ return (await response.json()) as EscrowEvent[];
+ } finally {
+ clearTimeout(timeoutId);
+ }📝 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.
| const response = await fetch(url); | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch escrow events: ${response.status}`); | |
| } | |
| return (await response.json()) as EscrowEvent[]; | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), 10_000); | |
| try { | |
| const response = await fetch(url, { signal: controller.signal }); | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch escrow events: ${response.status}`); | |
| } | |
| return (await response.json()) as EscrowEvent[]; | |
| } finally { | |
| clearTimeout(timeoutId); | |
| } |
🤖 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 `@src/services/nodeService.ts` around lines 69 - 73, The getEscrowEvents()
function makes an unbounded fetch call that lacks timeout protection and can
hang indefinitely on slow or unresponsive connections. Implement a timeout
mechanism by creating an AbortController instance, setting a timeout that aborts
the controller after a reasonable duration (e.g., 30 seconds), passing the
controller's signal to the fetch() call options, and catching any AbortError
that results from the timeout to provide appropriate error handling and user
feedback in the dashboard.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/node-details/balance.tsx (1)
47-85: 💤 Low valueConsider disabling the button when
envsis empty.The button is disabled only when
!ocean, but ifenvsis empty,nodeAddressbecomes an empty string, which could cause the gas fee deposit to fail or behave unexpectedly.🛡️ Optional defensive fix
<Button color="accent1" - disabled={!ocean} + disabled={!ocean || envs.length === 0} onClick={() => setIsGasFeeModalOpen(true)} size="md" variant="outlined" >🤖 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 `@src/components/node-details/balance.tsx` around lines 47 - 85, The Button component with the text "Send tokens for gas fee" is currently disabled only when the ocean variable is falsy, but it should also be disabled when the envs array is empty since this would result in an empty nodeAddress being passed to the GasFeeModal component. Update the disabled prop on the Button to include an additional check for whether envs is empty alongside the existing !ocean condition to prevent the modal from opening with invalid parameters.
🤖 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.
Nitpick comments:
In `@src/components/node-details/balance.tsx`:
- Around line 47-85: The Button component with the text "Send tokens for gas
fee" is currently disabled only when the ocean variable is falsy, but it should
also be disabled when the envs array is empty since this would result in an
empty nodeAddress being passed to the GasFeeModal component. Update the disabled
prop on the Button to include an additional check for whether envs is empty
alongside the existing !ocean condition to prevent the modal from opening with
invalid parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a7cfe129-f988-41f2-8c3a-2d8beb639252
📒 Files selected for processing (2)
src/components/node-details/balance.tsxsrc/components/node-details/node-info.tsx
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 (1)
src/components/run-job/payment-summary.tsx (1)
31-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix allowance insufficiency calculation when authorization data is missing.
At Line 31,
Number(authorizations?.maxLockedAmount) ?? 0can produceNaN, and?? 0won’t apply toNaN. That makes the check false and hides the “Insufficient allowance” state at Lines 66-70 when authorizations are absent/invalid.Suggested fix
- const insufficientAutorized = (Number(authorizations?.maxLockedAmount) ?? 0) < totalCost; + const maxLockedAmount = Number(authorizations?.maxLockedAmount ?? 0); + const insufficientAutorized = !Number.isFinite(maxLockedAmount) || maxLockedAmount < totalCost;Also applies to: 64-70
🤖 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 `@src/components/run-job/payment-summary.tsx` at line 31, The `insufficientAutorized` variable calculation on line 31 has a logic error where `Number(authorizations?.maxLockedAmount) ?? 0` can produce NaN when authorization data is invalid, and the nullish coalescing operator does not catch NaN values, causing the comparison to fail silently. Fix this by restructuring the expression to ensure a valid number is always produced, such as using `Number(authorizations?.maxLockedAmount || 0)` which handles undefined/invalid values before conversion, allowing the insufficient allowance message at lines 66-70 to display correctly when authorization data is missing or invalid.
🤖 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 `@src/components/run-job/payment-summary.tsx`:
- Line 31: The `insufficientAutorized` variable calculation on line 31 has a
logic error where `Number(authorizations?.maxLockedAmount) ?? 0` can produce NaN
when authorization data is invalid, and the nullish coalescing operator does not
catch NaN values, causing the comparison to fail silently. Fix this by
restructuring the expression to ensure a valid number is always produced, such
as using `Number(authorizations?.maxLockedAmount || 0)` which handles
undefined/invalid values before conversion, allowing the insufficient allowance
message at lines 66-70 to display correctly when authorization data is missing
or invalid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ceed499-62b5-450b-8c78-d85da05ba33b
📒 Files selected for processing (5)
src/components/escrow/escrow-token-panel.tsxsrc/components/run-job/payment-summary.module.csssrc/components/run-job/payment-summary.tsxsrc/lib/use-escrow-data.tssrc/services/nodeService.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/run-job/payment-summary.module.css
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/use-escrow-data.ts
- src/components/escrow/escrow-token-panel.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/lib/use-ocean-account.tsx`:
- Around line 176-179: The `ocean` instance in use-ocean-account.tsx is
initialized with a read-only JsonRpcProvider, but the methods
`authorizeTokensEoa`, `depositTokensEoa`, and `withdrawTokensEoa` all internally
call `this.provider.getSigner()` which fails on read-only providers. Fix this by
either: (1) modifying the ocean provider initialization to use the wallet's
BrowserProvider (the `provider` variable defined earlier in the hook) for write
operations while keeping the read-only provider for read-only calls, or (2)
refactor the three escrow write methods to use `sendTransaction` from the wallet
context with encoded contract calls instead of relying on the ocean instance's
signer-dependent methods.
🪄 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: a0f11014-02b2-4e50-a7c9-60d88dd57c5b
📒 Files selected for processing (1)
src/lib/use-ocean-account.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/node-details/node-info.tsx (1)
82-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard condition checks
node?.idbut operation usesnodeId.The guard checks
!node?.id, but the subsequent operations usenodeIdwhich is derived fromnode.id ?? node.nodeId. If a node has onlynodeId(notid), this function returns early even thoughnodeIdholds a valid value.The same pattern appears in
handlePushConfig(line 107) andhandleDownloadLogs(line 150).🐛 Proposed fix
async function handleFetchConfig() { if (!account.isConnected) { login(); return; } - if (!ocean || !node?.id) { + if (!ocean || !nodeId) { return; }Apply the same fix to
handlePushConfigandhandleDownloadLogs.🤖 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 `@src/components/node-details/node-info.tsx` around lines 82 - 84, The guard condition checks `!node?.id` but the function uses `nodeId` which is derived from `node.id ?? node.nodeId`. This causes the function to return early even when `nodeId` contains a valid value (when node only has nodeId but not id). Update the guard condition to check `!nodeId` instead of `!node?.id` to properly validate the value actually being used. Apply this same fix pattern to both the `handlePushConfig` method and the `handleDownloadLogs` method where the identical issue occurs.src/lib/use-authorize-tokens.ts (1)
50-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent stuck loading when the Ocean client is unavailable.
At Line 54, the EOA path returns early after
setIsAuthorizing(true)(Line 53), leavingisAuthorizingstucktrue. This can lock authorization UI indefinitely.Proposed fix
async ({ tokenAddress, peerId, spender, maxLockedAmount, maxLockSeconds, maxLockCount }: AuthorizeTokensParams) => { if (user?.type === 'eoa') { try { - setIsAuthorizing(true); - if (!ocean) return; + if (!ocean) { + setError('Ocean client not initialized'); + toast.error('Authorization failed'); + return; + } + setIsAuthorizing(true); const tx = await ocean.authorizeTokensEoa({ tokenAddress, spender,🤖 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 `@src/lib/use-authorize-tokens.ts` around lines 50 - 55, The early return statement in the EOA authorization path (when checking if !ocean) occurs after setIsAuthorizing(true) has been called, which leaves the isAuthorizing state stuck in true state indefinitely. Before returning early when the ocean client is unavailable, reset the isAuthorizing state to false by calling setIsAuthorizing(false) to ensure the loading state does not persist and lock the authorization UI.
🤖 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 `@src/components/escrow/create-authorization-modal.tsx`:
- Around line 58-75: The handleAuthorize function call in the onSubmit callback
is async but not being awaited, which allows Formik to clear the isSubmitting
state before the authorization transaction completes, potentially reopening the
submit path during latency. Add the await keyword before the handleAuthorize
call to ensure Formik keeps isSubmitting true until the authorization lifecycle
completes, preventing duplicate submissions during transaction processing.
- Around line 84-85: In the axios GET request where the node lookup is
performed, replace the manually constructed URL string with proper parameter
encoding. Instead of interpolating nodeId directly into the URL string in the
axios.get call for the nodes endpoint, use Axios's params configuration option
to pass all query parameters (page, size, and nodeId) as an object in the second
argument. This ensures proper URL encoding and prevents malformed requests.
---
Outside diff comments:
In `@src/components/node-details/node-info.tsx`:
- Around line 82-84: The guard condition checks `!node?.id` but the function
uses `nodeId` which is derived from `node.id ?? node.nodeId`. This causes the
function to return early even when `nodeId` contains a valid value (when node
only has nodeId but not id). Update the guard condition to check `!nodeId`
instead of `!node?.id` to properly validate the value actually being used. Apply
this same fix pattern to both the `handlePushConfig` method and the
`handleDownloadLogs` method where the identical issue occurs.
In `@src/lib/use-authorize-tokens.ts`:
- Around line 50-55: The early return statement in the EOA authorization path
(when checking if !ocean) occurs after setIsAuthorizing(true) has been called,
which leaves the isAuthorizing state stuck in true state indefinitely. Before
returning early when the ocean client is unavailable, reset the isAuthorizing
state to false by calling setIsAuthorizing(false) to ensure the loading state
does not persist and lock the authorization UI.
🪄 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: f26535d4-8344-4996-b37a-e828ba07da20
📒 Files selected for processing (5)
src/components/escrow/create-authorization-modal.tsxsrc/components/node-details/node-info.module.csssrc/components/node-details/node-info.tsxsrc/components/run-job/payment-authorize.tsxsrc/lib/use-authorize-tokens.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/run-job/payment-authorize.tsx
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This reverts commit 03ae23f.
Fixes #532 .
Changes proposed in this PR:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Removals