Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: optimistic disconnect #1871

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: optimistic disconnect #1871

wants to merge 4 commits into from

Conversation

magiziz
Copy link
Contributor

@magiziz magiziz commented Mar 19, 2024

Changes

  • Added optimistic disconnect behavior by checking isDisconnecting in RainbowKitWagmiStateProvider. If isDisconnecting is true, the useConnectionStatus hook is set to the disconnected state.

Screen recordings / screenshots

optimistic_disconnect.mov

@magiziz magiziz requested a review from a team as a code owner March 19, 2024 04:24
Copy link

vercel bot commented Mar 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
rainbowkit-example ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 8, 2024 10:01pm
rainbowkit-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 8, 2024 10:01pm

@DanielSinclair
Copy link
Contributor

@magiziz For that WalletConnect scenario, I think what might work better is an "optimistic disconnect". When the user taps disconnect, we put the RainbowKit modal into the disconnected state and then trigger the useDisconnect hook internally. This way the user is never blocked; they don't need to return to their wallet to disconnect anyway. We may need a retry to ensure Wagmi's state is in sync if the websocket were to fail

@magiziz magiziz changed the title chore: disable disconnect buttons when isDisconnecting is true feat: optimistic disconnect behaviour Mar 19, 2024
@@ -19,6 +19,7 @@ export const YourApp = () => {
<ConnectButton.Custom>
{({
account,
disconnecting,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This technically won't be a breaking change. If we were to merge this the only thing that would impact is if disconnecting is true, then openConnectModal with just return a noop function. Here is an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For injected wallets it should happen super fast, but for WalletConnect in some scenarios might take time. Of course depending on their socket server.

@DanielSinclair
Copy link
Contributor

DanielSinclair commented Apr 4, 2024

I started to look into a refactor here:

  • I don't think RainbowKitWagmiStateContext is necessary as we have the existing useConnectionStatus hook
  • useDisconnect also comes with a status prop that we could listen to globally and reflect in useConnectionStatus
  • Throughout the codebase we use isConnected and isConnecting from useAccount; we would want to consolidate this to only use useConnectionStatus
  • As long as the disconnect status flips to pending after a user was in a connected status, then we know they're in the disconnecting state
  • We would just want to treat disconnecting like existing connection status disconnected throughout the uses of that hook (as we want the UI to reflect disconnected, even if it is internally processing)

chore: remove unnecessary code

chore: remove unnecessary code
Comment on lines 13 to 19
const { isConnected } = useAccount();
const { isDisconnecting } = useRainbowKitWagmiState();

if (!isConnected) {
if (!isConnected || isDisconnecting) {
return 'disconnected';
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the most important part out of all which makes this work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know this is not ideal especially in the long term, but wagmi's useDisconnect hook doesn't provide a global scope state support. It only supports at a component level since they use TanStack react query mutation directly.

Maybe we can look at having isDisconnecting state in useAccount hook in the future, but for now this approach works.

As long as we mark isDisconnecting to disconnected mode in useConnectionStatus that'd work for now 🙏

Comment on lines +126 to 131
// may mark it as 'disconnected', we don't want the user to open the modal as the wagmi state
// still considers it 'connected'
!isDisconnecting &&
(connectionStatus === 'disconnected' ||
connectionStatus === 'unauthenticated')
? openConnectModal
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You technically can still open the connect modal if your connectionStatus is returning disconnected.

If we allow users to open the connect modal by the time WalletConnect takes time to disconnect, they'll face the issue with shim disconnection which is a known issue in wagmi rn. Ofc only if they start connecting different wallets at once, but we'll make sure to disable it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants