-
Notifications
You must be signed in to change notification settings - Fork 975
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
Fix: MWA Select but No Connect Bug #1049
Fix: MWA Select but No Connect Bug #1049
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, thanks! Just one clarifying question about a change in the other PRs that isn't in this one.
const adapter = useMemo( | ||
() => adaptersWithMobileWalletAdapter.find((a) => a.name === walletName) ?? null, | ||
[adaptersWithMobileWalletAdapter, walletName] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is different to #960 and #1013 which change this to:
const adapter = useMemo(
() =>
adaptersWithMobileWalletAdapter.find((a) => a.name === walletName) ??
(adaptersWithMobileWalletAdapter.length === 1 && adaptersWithMobileWalletAdapter[0] === mobileWalletAdapter
? mobileWalletAdapter
: null),
[adaptersWithMobileWalletAdapter, walletName, mobileWalletAdapter]
);
Do we not need that change any more?
It looks like that change was motivated by this comment: #960 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, we do not want this change. If it was added I would propose a new PR to remove it as it would bring back the bug we are trying to fix here 😅
This change was added originally to try to preserve the existing behavior with MWA in the (rare) case that MWA is the one and only adapter available on Mobile. But we are aiming to remove the existing behavior, since it causes the bug with react state leading to MWA being non-functional for mobile users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its an edge case that will likely never happen anyways, MWA being the one and only adapter available. So between that and the fact that it can reintroduce the bug we are trying to fix, we think its better to remove this special treatment for MWA.
If at some point we decide to somehow change the react state handling in the UI layer to alleviate this issue caused by default selection of the adapter, we could then reintroduce special handling on MWA.
See this comment from, Jordan on the original pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks! Glad we're finally getting this fixed!
This PR fixes an issue with MWA where the Solana Mobile wallet adapter gets stuck in a state that prevents the user from connecting to thier installed wallet. The issue is fully described with root cause and a video on our repo here.
This fix is nearly identical to the changes in #1013. All tests have been fixed to accommodate the new behavior. The #1013 PR should be closed in favor of this PR. Alternatively I can rebase my changes on that branch if you prefer @jordaaash.