-
Notifications
You must be signed in to change notification settings - Fork 992
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
Restructure wallet-connect namespaces #21167
Conversation
Jenkins BuildsClick to see older builds (19)
|
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.
Overall looks good to me. Thank you for the amazing work 🚀
src/status_im/contexts/wallet/wallet_connect/utils/data_transformations.cljs
Outdated
Show resolved
Hide resolved
@status-im/mobile-qa ready for testing |
86% of end-end tests have passed
Failed tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (6)Click to expandClass TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
|
@clauxx thanks for the PR. Please, take a look at the issue ISSUE 1 No account is selected by default in connection modalSteps:
Actual result: no account selected. Expected result: default account should be selected |
I will take a look to this issue |
83cbb86
to
91831e3
Compare
91831e3
to
d87d8f6
Compare
Hey @mariia-skrypnyk could you please test again. thanks |
Hi @mohsen-ghafouri ! I see one more issue relates to account selection: ISSUE 2: user is navigated to Wallet Account without a connection Steps:
Actual result: User is redirected to Wallet account connection screen on Account 1 without a connection: IMG_2330.MP4Expected result: User should be redirected to Wallet account connection of chosen Account 2. Design with navigation example is here https://www.figma.com/design/mLyiYPXqu8KetdQONOlZpV/dApp-Interactions?node-id=47-29943&node-type=frame&t=YPrT5xiOglqRZdyE-0 |
Thank you @mariia-skrypnyk for checking this, could you please check if it's related to this PR or a general issue on dev, if so we can handle it in a follow up issue, as current PR is big enough to merge. also i'm not sure when user exactly is in this account we should let they change the selection or not. |
Thanks @mohsen-ghafouri ! This behaviour also exist on nightly. |
57% of end-end tests have passed
Failed tests (3)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestWalletMultipleDevice:
Class TestCommunityOneDeviceMerged:
Passed tests (4)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
Failed tests are not related to PR, ready to merge thanks @mariia-skrypnyk and @mohsen-ghafouri |
67% of end-end tests have passed
Failed tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestWalletMultipleDevice:
|
d87d8f6
to
bb548da
Compare
Hi @mariia-skrypnyk, I created this issue #21243 for ISSUE 2 |
fixes #21166
Summary
Following the initial WalletConnect implementation and the rush to release, a restructuring became necessary to stabilize the future development of the feature.
Review notes
There are no functional changes other than the namespace names (which cause formatting changes sometimes) and moving of functions into different namespaces.
Testing notes
Need to make sure the main flows work as expected, as all the wallet-connect code was touched. There are no functional changes though, so things should work as they did before.
Platforms
Areas that maybe impacted
Functional
status: ready