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

refactor: walletconnect prefetch #2124

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

refactor: walletconnect prefetch #2124

wants to merge 3 commits into from

Conversation

magiziz
Copy link
Contributor

@magiziz magiziz commented Jul 30, 2024

Changes

Optimised our wallets that use WalletConnect connector. It should now be fast and improve the UX.

Screen recordings / screenshots

Prefetching WalletConnect URI

wc-desktop.mp4

(Me spamming cancel button on Safari mobile)

wc-mobile.mov

What to test

Desktop:

  • QR code for wallets should be generated pretty fast because we prefetch WalletConnect URI when connect modal is opened. If let's we don't get the URI before clicking on the wallet then the behaviour should be the same as it's right now. In this case we'll wait until we get the URI before displaying QR code.
  • When scanning the QR code using a wallet and then clicking "Cancel" or "Reject" there should be a new QR code displayed.
  • Disconnect flow should be much faster and take around 0.5 - 1 second. This is because we appended two WalletConnect connectors in connectorsForWallets which resulted into having the same Wagmi reconnect bug that we had with EIP-1193 connectors.

Mobile:

  • Make sure on Safari mobile whenever you click a wallet -> "Cancel" (safari popup) -> then click on the same wallet again you should not get the default iOS forbidden popup.
  • Whenever you click on any wallet and then reject the connection request you can go back to the browser and still get a new deep link with a new WalletConnect URI. This means you can reject as many times you want, but have the ability to connect without refeshing the page.

@magiziz magiziz requested a review from a team as a code owner July 30, 2024 18:06
Copy link

vercel bot commented Jul 30, 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 Aug 5, 2024 11:49am
rainbowkit-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 5, 2024 11:49am

Copy link

socket-security bot commented Jul 30, 2024

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring: npm/@bundled-es-modules/cookie@2.0.0, npm/@bundled-es-modules/statuses@1.0.1, npm/@bundled-es-modules/tough-cookie@0.1.6, npm/@inquirer/confirm@3.1.17, npm/@inquirer/confirm@3.1.21, npm/@inquirer/core@9.0.5, npm/@inquirer/core@9.0.9, npm/@inquirer/figures@1.0.5, npm/@inquirer/type@1.5.1, npm/@inquirer/type@1.5.2, npm/@mswjs/interceptors@0.29.1, npm/@open-draft/deferred-promise@2.2.0, npm/@open-draft/logger@0.3.0, npm/@open-draft/until@2.1.0, npm/@polka/url@1.0.0-next.25, npm/@testing-library/dom@10.4.0, npm/@types/mute-stream@0.0.4, npm/@types/node@20.14.12, npm/@types/node@22.1.0, npm/@types/statuses@2.0.5, npm/@types/tough-cookie@4.0.5, npm/@types/wrap-ansi@3.0.0, npm/@vitest/browser@2.0.4, npm/cli-width@4.1.0, npm/fsevents@2.3.2, npm/graphql@16.9.0, npm/headers-polyfill@4.0.3, npm/is-node-process@1.2.0, npm/mrmime@2.0.0, npm/msw@2.3.4, npm/msw@2.3.5, npm/mute-stream@1.0.0, npm/outvariant@1.4.3, npm/path-to-regexp@6.2.2, npm/playwright-core@1.45.3, npm/playwright@1.45.3, npm/sirv@2.0.4, npm/strict-event-emitter@0.5.1, npm/totalist@3.0.1, npm/type-fest@4.23.0, npm/undici-types@6.13.0, npm/yoctocolors-cjs@2.1.2

View full report↗︎

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/foo@1.0.0 or ignore all packages with @SocketSecurity ignore-all

@magiziz
Copy link
Contributor Author

magiziz commented Jul 30, 2024

@SocketSecurity ignore npm/msw@2.3.4

Copy link

socket-security bot commented Jul 31, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher

View full report↗︎

@DanielSinclair
Copy link
Collaborator

@magiziz Pushed up a little change to avoid mismatch jsdom and playwright together. I'm not quite sure why globals is still necessary in the vitest config though; thought that would be bundled with the playwright runner.

I did notice the component tests have become a bit noisy. Any idea how to silence some of these warnings?

Warning: An update to %s inside a test was not wrapped in act(...).

When testing, code that causes React state updates should be wrapped into act(...):

act(() => {
  /* fire events that update state */
});
/* assert on the output */

This ensures that you're testing the behavior the user would see in the browser. Learn more at https://reactjs.org/link/wrap-tests-with-act%s Dialog 
    at Dialog (http://localhost:5173/packages/rainbowkit/src/components/Dialog/Dialog.tsx:14:26)
    at ChainModal (http://localhost:5173/packages/rainbowkit/src/components/ChainModal/ChainModal.tsx:19:30)
    at ModalProvider (http://localhost:5173/packages/rainbowkit/src/components/RainbowKitProvider/ModalContext.tsx:31:33)
    at ShowBalanceProvider (http://localhost:5173/packages/rainbowkit/src/components/RainbowKitProvider/ShowBalanceContext.tsx:7:39)
    at TransactionStoreProvider (http://localhost:5173/packages/rainbowkit/src/transactions/TransactionStoreContext.tsx:14:3)
    at ModalSizeProvider (http://localhost:5173/packages/rainbowkit/src/components/RainbowKitProvider/ModalSizeContext.tsx:13:3)
    at I18nProvider (http://localhost:5173/packages/rainbowkit/src/components/RainbowKitProvider/I18nContext.tsx:10:32)
    at WalletButtonProvider (http://localhost:5173/packages/rainbowkit/src/components/RainbowKitProvider/WalletButtonContext.tsx:7:40)
    at WalletConnectStoreProvider (http://localhost:5173/packages/rainbowkit/src/components/RainbowKitProvider/WalletConnectStoreProvider.tsx:21:3)
    at RainbowKitChainProvider (http://localhost:5173/packages/rainbowkit/src/components/RainbowKitProvider/RainbowKitChainContext.tsx:8:3)
    at RainbowKitProvider (http://localhost:5173/packages/rainbowkit/src/components/RainbowKitProvider/RainbowKitProvider.tsx:38:3)
    at QueryClientProvider (http://localhost:5173/node_modules/.vite/deps/chunk-7HXWSVET.js?v=5ac11689:3616:3)
    at Hydrate (http://localhost:5173/node_modules/.vite/deps/wagmi.js?v=5ac11689:360:11)
    at WagmiProvider (http://localhost:5173/node_modules/.vite/deps/wagmi.js?v=5ac11689:384:11)
    at wrapper (http://localhost:5173/packages/rainbowkit/test/index.tsx:55:17)
stderr | packages/rainbowkit/src/components/ChainModal/ChainModal.test.tsx > <ChainModal /> > Custom chain metadata passed from <RainbowKitProvider>
Warning: An update to %s inside a test was not wrapped in act(...).

When testing, code that causes React state updates should be wrapped into act(...):

act(() => {
  /* fire events that update state */
});
/* assert on the output */

This ensures that you're testing the behavior the user would see in the browser. Learn more at https://reactjs.org/link/wrap-tests-with-act%s ChainModalWithConnectButton 
    at ChainModalWithConnectButton (http://localhost:5173/Users/daniel/Desktop/rainbowkit/packages/rainbowkit/src/components/ChainModal/ChainModal.test.tsx?import&browserv=1722407134988:8:40)
    at div
    at ModalProvider (http://localhost:5173/packages/rainbowkit/src/components/RainbowKitProvider/ModalContext.tsx:31:33)
    at ShowBalanceProvider (http://localhost:5173/packages/rainbowkit/src/components/RainbowKitProvider/ShowBalanceContext.tsx:7:39)
    at TransactionStoreProvider (http://localhost:5173/packages/rainbowkit/src/transactions/TransactionStoreContext.tsx:14:3)
    at ModalSizeProvider (http://localhost:5173/packages/rainbowkit/src/components/RainbowKitProvider/ModalSizeContext.tsx:13:3)
    at I18nProvider (http://localhost:5173/packages/rainbowkit/src/components/RainbowKitProvider/I18nContext.tsx:10:32)
    at WalletButtonProvider (http://localhost:5173/packages/rainbowkit/src/components/RainbowKitProvider/WalletButtonContext.tsx:7:40)
    at WalletConnectStoreProvider (http://localhost:5173/packages/rainbowkit/src/components/RainbowKitProvider/WalletConnectStoreProvider.tsx:21:3)
    at RainbowKitChainProvider (http://localhost:5173/packages/rainbowkit/src/components/RainbowKitProvider/RainbowKitChainContext.tsx:8:3)
    at RainbowKitProvider (http://localhost:5173/packages/rainbowkit/src/components/RainbowKitProvider/RainbowKitProvider.tsx:38:3)
    at QueryClientProvider (http://localhost:5173/node_modules/.vite/deps/chunk-7HXWSVET.js?v=5ac11689:3616:3)
    at Hydrate (http://localhost:5173/node_modules/.vite/deps/wagmi.js?v=5ac11689:360:11)
    at WagmiProvider (http://localhost:5173/node_modules/.vite/deps/wagmi.js?v=5ac11689:384:11)
    at wrapper (http://localhost:5173/packages/rainbowkit/test/index.tsx:55:17)

@magiziz
Copy link
Contributor Author

magiziz commented Jul 31, 2024

@DanielSinclair I can look into refactoring the tests and suspress those warnings in another PR ? Apparently those warnings were already there since the @testing-library/jest-dom wants us to use their act method when testing user behaviours. Here is example of one we had earlier https://github.com/rainbow-me/rainbowkit/actions/runs/10116065175/job/27978109985.

@DanielSinclair DanielSinclair changed the title chore: WalletConnect optimization refactor: walletconnect prefetch Jul 31, 2024
@magiziz
Copy link
Contributor Author

magiziz commented Aug 5, 2024

@SocketSecurity ignore-all

@bujoralexandru
Copy link

bujoralexandru commented Oct 1, 2024

@DanielSinclair @magiziz hey guys, thank you for your work on rainbow ❤️ . Is this PR a long way from being merged? I've seen quite a few projects depending on rainbow that are impacted by the issues fixed in this PR. Can we at least get an ETA regarding how much time you still need to "cook" this PR?

@subhod-i
Copy link

subhod-i commented Oct 7, 2024

I agree this PR fixes some major issues and improves the user experience by a big margin, especially on IOS.

@generatorpoint
Copy link

@magiziz can this PR be merged? Safe wallet users are having trouble connecting via Rainbowkit using Walletconnect and we're hoping these optimizations might fix 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
5 participants