Escrow contract switch#554
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a legacy escrow contract selector allowing users to view balances/locks and withdraw funds from a previous escrow deployment via an env-configured address, while restricting deposits and authorization actions in legacy mode. Also adds a dismissible homepage banner linking to the legacy escrow flow. ChangesLegacy Escrow Contract Selector
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant EscrowPage
participant useEscrowData
participant OceanProvider
participant EscrowContract
User->>EscrowPage: select "Legacy contract"
EscrowPage->>EscrowPage: compute escrowAddress from LEGACY_ESCROW_ADDRESS
EscrowPage->>useEscrowData: useEscrowData(escrowAddress)
useEscrowData->>OceanProvider: getUserFundsDetailed(token, address, escrowAddress)
useEscrowData->>OceanProvider: getAllAuthorizations(token, payer, escrowAddress)
useEscrowData->>OceanProvider: getLocks(token, payer, payee, escrowAddress)
OceanProvider->>EscrowContract: query balances/locks/authorizations at escrowAddress
EscrowContract-->>OceanProvider: data
OceanProvider-->>useEscrowData: tokens/spenders/locks
useEscrowData-->>EscrowPage: updated state
EscrowPage-->>User: renders read-only authorizations, withdraw-only actions
User->>EscrowPage: submit withdraw
EscrowPage->>useEscrowData: handleWithdraw({..., escrowAddress})
useEscrowData->>OceanProvider: withdrawTokensEoa({tokenAddresses, amounts, escrowAddress})
OceanProvider->>EscrowContract: withdraw from escrowAddress
EscrowContract-->>OceanProvider: tx result
OceanProvider-->>EscrowPage: withdrawal confirmed
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
bogdanfazakas
requested review from
alexcos20,
andreip136,
dnsi0,
giurgiur99 and
mihaisc
as code owners
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
This PR is exceptionally well-implemented. It handles the legacy escrow contract redeployment seamlessly by adding a fallback selector and safe read-only/withdraw modes. The approach used to handle React race conditions during data fetching is highly commendable. All edge cases regarding disabled deposits and authorizations on the legacy contract have been thoroughly accounted for.
Comments:
• [INFO][bug] It is a good practice to wrap localStorage access in a try...catch block. In some environments (like Safari Private Browsing or restricted iframes), accessing localStorage throws a SecurityError or QuotaExceededError, which could cause the React component to crash during mount or user interaction.
Consider wrapping the logic safely:
- setVisible(localStorage.getItem(DISMISSED_KEY) !== 'true');
+ try {
+ setVisible(localStorage.getItem(DISMISSED_KEY) !== 'true');
+ } catch (err) {
+ setVisible(true);
+ }And in your dismiss handler:
- localStorage.setItem(DISMISSED_KEY, 'true');
- setVisible(false);
+ try {
+ localStorage.setItem(DISMISSED_KEY, 'true');
+ } catch (err) {
+ // ignore
+ }
+ setVisible(false);• [INFO][style] The way you handled race conditions with loadIdRef and escrowAddressRef is excellent! Ensuring that late-arriving data from an old contract fetch doesn't overwrite the current state is a very robust pattern for data fetching in React.
• [INFO][other] Great job on preventing deposits to the legacy contract right at the UI level while keeping the hook usage clean.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/escrow/escrow-token-panel.tsx (1)
220-254: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWithdraw/deposit form values persist across contract switch.
EscrowTokenPanelisn't remounted whenescrowAddresschanges (no key change), and there's no effect resettingdepositForm/withdrawFormon that change. A previously-entered amount from the current contract's view carries over when switching to the legacy view (or back), which can confuse users even though submission still correctly targets the resolvedescrowAddress.💡 Proposed fix: reset forms when escrowAddress changes
const EscrowTokenPanel = ({ token, spenders, loadingSpenders, onChange, escrowAddress }: EscrowTokenPanelProps) => { const [isCreateOpen, setIsCreateOpen] = useState(false); const isLegacy = !!escrowAddress; + + useEffect(() => { + depositForm.resetForm(); + withdrawForm.resetForm(); + }, [escrowAddress]);🤖 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/escrow/escrow-token-panel.tsx` around lines 220 - 254, The deposit and withdraw amounts in EscrowTokenPanel are persisting when escrowAddress changes because the component is reused and the Formik state is never reset. Add an effect in EscrowTokenPanel that watches escrowAddress and calls both depositForm.resetForm() and withdrawForm.resetForm() whenever the contract view switches, so the amount fields clear when moving between legacy and current escrow views.
🧹 Nitpick comments (2)
src/components/homepage/legacy-escrow-banner.module.css (1)
8-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffMagic-number coupling to
hero-section.module.cssbreakpoints.The negative
margin-topvalues and breakpoints are hand-copied to mirror another file's layout, per the comment. Any future change to hero padding/breakpoints silently desyncs this file since there's no shared constant/variable enforcing the relationship.🤖 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/homepage/legacy-escrow-banner.module.css` around lines 8 - 24, The banner spacing in the legacy escrow CSS is hard-coupled to hero-section layout values via copied breakpoints and negative margins. Refactor the .banner rules to use shared design tokens or CSS custom properties sourced from the hero layout, and update the `@media` thresholds to reference the same centralized values so both files stay in sync. Use the existing .banner selector and the hero-section.module.css breakpoint/padding definitions as the single source of truth.src/components/homepage/legacy-escrow-banner.tsx (1)
20-31: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard localStorage access against exceptions.
localStorage.getItem/setItemare called unguarded. In restrictive environments (e.g., private browsing modes, storage disabled by policy), these calls can throw and crash the component since there's no error boundary/try-catch here.🛡️ Proposed defensive fix
useEffect(() => { - setVisible(localStorage.getItem(DISMISSED_KEY) !== 'true'); + try { + setVisible(localStorage.getItem(DISMISSED_KEY) !== 'true'); + } catch { + setVisible(true); + } }, []); const dismiss = () => { - localStorage.setItem(DISMISSED_KEY, 'true'); - setVisible(false); + try { + localStorage.setItem(DISMISSED_KEY, 'true'); + } catch { + // ignore storage errors; still hide for this session + } + setVisible(false); };🤖 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/homepage/legacy-escrow-banner.tsx` around lines 20 - 31, The localStorage calls in LegacyEscrowBanner are unguarded and can throw in restricted environments. Update the useEffect initializer and the dismiss handler to wrap localStorage.getItem and localStorage.setItem in defensive try/catch logic, and fall back to a safe default if storage is unavailable. Keep the fix localized to LegacyEscrowBanner and its dismiss function so the component still renders without crashing when storage access fails.
🤖 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/escrow/escrow-token-panel.tsx`:
- Around line 220-254: The deposit and withdraw amounts in EscrowTokenPanel are
persisting when escrowAddress changes because the component is reused and the
Formik state is never reset. Add an effect in EscrowTokenPanel that watches
escrowAddress and calls both depositForm.resetForm() and
withdrawForm.resetForm() whenever the contract view switches, so the amount
fields clear when moving between legacy and current escrow views.
---
Nitpick comments:
In `@src/components/homepage/legacy-escrow-banner.module.css`:
- Around line 8-24: The banner spacing in the legacy escrow CSS is hard-coupled
to hero-section layout values via copied breakpoints and negative margins.
Refactor the .banner rules to use shared design tokens or CSS custom properties
sourced from the hero layout, and update the `@media` thresholds to reference the
same centralized values so both files stay in sync. Use the existing .banner
selector and the hero-section.module.css breakpoint/padding definitions as the
single source of truth.
In `@src/components/homepage/legacy-escrow-banner.tsx`:
- Around line 20-31: The localStorage calls in LegacyEscrowBanner are unguarded
and can throw in restricted environments. Update the useEffect initializer and
the dismiss handler to wrap localStorage.getItem and localStorage.setItem in
defensive try/catch logic, and fall back to a safe default if storage is
unavailable. Keep the fix localized to LegacyEscrowBanner and its dismiss
function so the component still renders without crashing when storage access
fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 729b9975-ff6d-4e15-b507-9bbf9e81ae74
📒 Files selected for processing (12)
README.mdsrc/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/homepage/hero-section.tsxsrc/components/homepage/legacy-escrow-banner.module.csssrc/components/homepage/legacy-escrow-banner.tsxsrc/constants/escrow.tssrc/lib/ocean-provider.tssrc/lib/use-escrow-data.tssrc/lib/use-withdraw-tokens.ts
3 participants
Fixes #552 .
Changes proposed in this PR:
Summary by CodeRabbit
New Features
Bug Fixes