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

revert: revert remove wallet connect page #8466

Merged
merged 5 commits into from
Jan 6, 2025

Conversation

NeOMakinG
Copy link
Collaborator

Description

This PR has been reverted because when someone clear cache on the wallet page, then try to connect to a native wallet, it loads indefinitely

This issue was always existing but appeared when we removed the connect wallet page

It seems there is a race condition somehow where deviceId is set but wallet is undefined, in order to avoid trying to find the root cause because the WalletProvider is a pita, I would prefer to wait for wallet to be defined

Also remove the connect wallet page dead code as it was initially done and tested

Issue (if applicable)

spotted in https://discord.com/channels/554694662431178782/1324583740000698420/1325624884415565844

Risk

Medium (related to market data and portfolio data loading along with wallet)

High Risk PRs Require 2 approvals

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

Testing

  • Go to wallet page
  • Clear cache
  • Connect to a native wallet, market and portfolio data should load
  • Also try to smoke test along the app but shouldn't do so much side effects

Engineering

n/a

Operations

n/a

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)

Screenshots (if applicable)

https://jam.dev/c/eefa5a79-dff8-4ede-96a5-ae23beec5177

@NeOMakinG NeOMakinG requested a review from a team as a code owner January 6, 2025 07:19
@gomesalexandre gomesalexandre self-requested a review January 6, 2025 09:47
Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Tested locally, confirmed this does what it says on the box! One q: re mobile splash flow to ensure this gets tested as well, but happy to get this one in otherwise.

https://jam.dev/c/2dff74bd-5eeb-4109-a3a2-9d353a2a1c22

@NeOMakinG NeOMakinG force-pushed the revert-revert-remove-splash branch from db28038 to 3eb1eae Compare January 6, 2025 10:25
@NeOMakinG NeOMakinG enabled auto-merge (squash) January 6, 2025 12:39
@NeOMakinG NeOMakinG merged commit 0c8668c into develop Jan 6, 2025
3 checks passed
@NeOMakinG NeOMakinG deleted the revert-revert-remove-splash branch January 6, 2025 12:47
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.

2 participants