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

wallet connect fix #5387

Merged
merged 9 commits into from Feb 8, 2024
Merged

wallet connect fix #5387

merged 9 commits into from Feb 8, 2024

Conversation

benisgold
Copy link
Member

@benisgold benisgold commented Feb 6, 2024

Fixes APP-1129

What changed (plus any additional context for devs)

  • prevent dapp connections that don't contain any compatible accounts in the session's namespace (https://specs.walletconnect.com/2.0/specs/clients/sign/namespaces)
  • added a migration that purges existing connections that are invalid according to the above requirement
  • added support for connections that don't contain any compatible chains in the session's namespace

Screen recordings / screenshots

UI for connections w/o supported networks
Simulator Screenshot - iPhone 15 Pro - 2024-02-06 at 11 31 09
Simulator Screenshot - iPhone 15 Pro - 2024-02-06 at 12 14 52

failed + successful dapp connections
https://github.com/rainbow-me/rainbow/assets/15272675/fa31a4fc-8890-46c8-9c74-7e4fe141b654
https://github.com/rainbow-me/rainbow/assets/15272675/25bf5524-8db3-4f54-a9e3-b8468577bdae

What to test

TF 1.9.15 (27)
try connecting to https://devnet.dymension.xyz/rollapps and then navigating to connected dapps list

Copy link

linear bot commented Feb 6, 2024

*************** Migration v19 ******************
Purge walletconnect v2 sessions with no accounts
*/
const v19 = async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add this to the new migrations flow that our king @estrattonbailey made? wanna deleeet these at some point

Copy link
Member Author

Choose a reason for hiding this comment

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

totally forgot that existed lol

Copy link
Contributor

Choose a reason for hiding this comment

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

am I having a stroke or did I respond to a notification like this months ago lmao

miss you guysssss ❤️

Copy link
Member Author

Choose a reason for hiding this comment

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

@estrattonbailey !!!!!

hope all is well over at bsky

Copy link
Contributor

Choose a reason for hiding this comment

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

ill never miss a chance to tag my king

Copy link
Contributor

@walmat walmat left a comment

Choose a reason for hiding this comment

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

Wasn't having issues before so maybe we can QA with someone who was before / after merging. But it looks good and there are no regressions on my end.

@benisgold
Copy link
Member Author

@walmat should have mentioned in PR description but you can test by connecting to this dapp and then going to connected dapps list. also the fix is available on TF 1.9.15 (27)

@@ -270,13 +270,8 @@ export default function WalletConnectApprovalSheet() {
const handleConnect = useCallback(() => {
handled.current = true;
goBack();
if (IS_IOS) {
Copy link
Member Author

Choose a reason for hiding this comment

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

moved this inside wc approval callback. i noticed that this was causing the "connection successful" modal to pop up even if there was an error

Copy link
Contributor

@skylarbarrera skylarbarrera left a comment

Choose a reason for hiding this comment

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

clean

@skylarbarrera skylarbarrera added the release for release blockers and release candidate branches label Feb 7, 2024
@skylarbarrera skylarbarrera merged commit 79b7f99 into develop Feb 8, 2024
5 of 6 checks passed
@skylarbarrera skylarbarrera deleted the @benisgold/wc-fix branch February 8, 2024 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release for release blockers and release candidate branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants