Skip to content

disconnect after topup#863

Open
techeng322 wants to merge 1 commit intomainfrom
techengme/myc-3490-topup-wallet-overridding-my-email-sign-in
Open

disconnect after topup#863
techeng322 wants to merge 1 commit intomainfrom
techengme/myc-3490-topup-wallet-overridding-my-email-sign-in

Conversation

@techeng322
Copy link
Copy Markdown
Collaborator

@techeng322 techeng322 commented Nov 19, 2025

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Improved external wallet security by properly revoking account permissions during disconnection.
    • Enhanced sequential execution of deposit operations and wallet interactions for more reliable flows.
  • Chores

    • Removed unused global event handlers from type definitions.

@vercel
Copy link
Copy Markdown

vercel bot commented Nov 19, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Updated (UTC)
in-process Ready Ready Preview Nov 19, 2025 6:42pm

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Nov 19, 2025

Walkthrough

The PR modifies wallet disconnection logic to support asynchronous permission revocation. The Deposit button's click handler now awaits the deposit operation before triggering disconnect. The connectDisconnect function converts to async and revokes external wallet permissions before disconnection. Unused global Window type augmentations are removed.

Changes

Cohort / File(s) Summary
Async Wallet Disconnect Logic
hooks/useTopup.ts
connectDisconnect converted to async function with external wallet permission revocation. Tightens wallet presence check to address property. When external wallet exists, attempts to revoke eth\_accounts permission via getEthereumProvider before disconnecting. When none exists, connects wallet as before.
Deposit Button Handler Update
components/TopupPage/Deposit.tsx
Deposit button onClick handler changed to async wrapper that awaits deposit completion before invoking connectDisconnect. Maintains existing isDepositing state handling.
Type Definition Cleanup
types/global.d.ts
Removes unused global Window interface augmentation containing onNodeLabelChange and onEdgeLabelChange callbacks. Removes module boundary export marker.

Sequence Diagram

sequenceDiagram
    autonumber
    actor User
    participant Deposit Button
    participant deposit()
    participant connectDisconnect()
    participant External Wallet
    participant Ethereum Provider

    User->>Deposit Button: Click
    Deposit Button->>deposit(): await
    deposit()->>Deposit Button: Complete
    Deposit Button->>connectDisconnect(): invoke (now async)
    alt External Wallet Exists
        connectDisconnect()->>External Wallet: getEthereumProvider()
        External Wallet-->>Ethereum Provider: provider
        connectDisconnect()->>Ethereum Provider: wallet_revokePermissions
        Ethereum Provider-->>connectDisconnect(): result
    end
    connectDisconnect()->>External Wallet: Disconnect
    External Wallet-->>connectDisconnect(): Done
    connectDisconnect()-->>User: Complete
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • hooks/useTopup.ts — Verify the external wallet permission revocation logic correctly handles provider availability and error cases
  • components/TopupPage/Deposit.tsx — Confirm async/await sequencing maintains proper error propagation and loading state
  • Cross-file interaction — Ensure connectDisconnect's signature change to Promise\<void\> is properly awaited throughout the codebase

Poem

🐰 A hop, a skip, permissions to revoke,
Disconnect with grace before the final stroke,
External wallets yield to cleanup's might,
Types dust away in the refactoring light! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'disconnect after topup' clearly summarizes the main change: modifying the deposit flow to disconnect the wallet after topup completes.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch techengme/myc-3490-topup-wallet-overridding-my-email-sign-in

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
components/TopupPage/Deposit.tsx (1)

73-76: Consider awaiting connectDisconnect for clearer async flow and error handling

This achieves the “disconnect after topup” behavior by awaiting deposit() before calling connectDisconnect(). Since connectDisconnect is now async, you may want to await connectDisconnect() as well so that:

  • the full deposit + disconnect sequence is explicit and completed within the handler, and
  • any errors from the disconnect flow are surfaced in the same place as deposit errors.

If you intentionally want disconnect to be fire‑and‑forget, this is fine as‑is, but a brief comment could help future readers understand that choice.

hooks/useTopup.ts (1)

33-47: Async connectDisconnect flow is solid; consider minor refinements

The new flow is robust:

  • When connected, you try to revoke eth_accounts via wallet_revokePermissions, and
  • externalWallet?.disconnect() in finally guarantees cleanup even if the revoke call fails or throws.

A few optional tweaks to consider:

  • If connectWallet() is async in @privy-io/react-auth, you might want to await connectWallet() so callers of connectDisconnect can reliably know when the connect flow has finished.
  • Depending on UX needs, you may want to catch/log non‑supported wallet_revokePermissions errors (some providers may not implement it) while still proceeding to disconnect.
  • If disconnect itself is async, consider awaiting it so that any downstream callers can rely on completion.

None of these are blockers; the current logic already meets the “disconnect after topup” goal with proper cleanup.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f440079 and ad8a9ad.

📒 Files selected for processing (3)
  • components/TopupPage/Deposit.tsx (1 hunks)
  • hooks/useTopup.ts (2 hunks)
  • types/global.d.ts (0 hunks)
💤 Files with no reviewable changes (1)
  • types/global.d.ts
🔇 Additional comments (1)
hooks/useTopup.ts (1)

18-18: Tighter hasExternalWallet check looks good

Switching to Boolean(externalWallet?.address) makes the presence check better aligned with an actually connected wallet rather than any non‑null object. This should reduce false positives where an externalWallet instance exists but is not yet connected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant