-
Notifications
You must be signed in to change notification settings - Fork 984
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
[#20828] feat: display toast for all wallet connect actions #21534
Conversation
b90cdc4
to
1a4cdaa
Compare
Jenkins BuildsClick to see older builds (22)
|
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.
Looks good 👍
:on-success (fn [] | ||
(log/info "Wallet Connect session proposal rejected") | ||
(rf/dispatch [:toasts/upsert | ||
{:type :positive |
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.
Should this toast be of type :positive
if the session proposal was rejected?
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.
Since this isn’t an error and the user intended to reject the session, we can confirm it was successfully rejected by :positive
type IMO.
75% of end-end tests have passed
Failed tests (2)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Passed tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
|
Hey @mohsen-ghafouri ! Thanks for your PR!
disconnecting dapp Shouldn't it be like we see here in Decline section:
disconnecting dapp failed |
Hello @mariia-skrypnyk, thanks for testing! Manually disconnecting a dapp is different from declining a connection request—each has its own message. As for the 2nd point, it’s unlikely to trigger unless the WalletConnect library throws an error, which should be rare. Just thinking out loud—if you disconnect a dapp on the dapp side first, would trying to disconnect it again in Status throw any error? 🤔 |
@mohsen-ghafouri disconnecting from the dapp side would trigger a |
So @mohsen-ghafouri in my 1. point I see the correct toast when I decline connection request?
In practice if I disconnect connection on the dApp side then in app connection disconnects automatically and dissapears from the connection list. Did you have a chance to catch the disconnecting dapp failed toast on your dev side? I want to make sure it works in case I can’t emulate this situation manually. |
1a4cdaa
to
c670e7a
Compare
Hey @mariia-skrypnyk i tried to record video for different scenarios, i hope it helps. In first video :
Simulator.Screen.Recording.-.iPhone.13.-.2024-11-05.at.14.54.43.mp4in second video i could programmatically produce the fail situation that user can't disconnect the dapp and here is the result Simulator.Screen.Recording.-.iPhone.13.-.2024-11-05.at.14.56.55.mp4 |
Thanks @mohsen-ghafouri !
https://github.com/user-attachments/assets/0a4e4da4-5a02-41bd-b4f4-e84708e70cfc Everything else is fine all places you've mentioned in a PR. |
@mariia-skrypnyk About point 2, there were some conversation about tech limitation and other stuff that @clauxx did with design team, some are available in figma file https://www.figma.com/design/HOJ4lV2ko3jzMjorsDzaoP/Wallet-%2F-dApp-notifications?node-id=1-81335&t=yTFW0izl574K99Tp-4. In this PR i just added changes from #20870 with just made it compatible with latest code base changes. about point 3 there is no requirement that i'm aware about it. we just wanted to show toast for user action that they do inside the status app IMO. |
Thanks @mohsen-ghafouri !
Is it in a scope of your PR? |
Sure @mariia-skrypnyk i will update PR, also i couldn't find any reference about display account name when user disconnect from dApp, so i will update text to |
Hey @mohsen-ghafouri ! Please use this design https://www.figma.com/design/mLyiYPXqu8KetdQONOlZpV/dApp-Interactions?node-id=780-12729&node-type=frame&m=dev for disconnection case. Discussion is here https://discord.com/channels/1210237582470807632/1274068685266489434/1303689180643065887 |
c670e7a
to
ef6ab36
Compare
Hey @clauxx could you please my last commit? not sure if there is any other way to access to dApp name and account name. |
(defn get-account-by-session | ||
[db session] | ||
(let [accounts (get-in db [:wallet :accounts]) | ||
session-account-address (first (:accounts session)) | ||
[_ address] (network-utils/split-network-full-address session-account-address)] | ||
(get-account-by-address (vals accounts) address))) |
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 util seems specific to wallet-connect, since you're passing the session. You could either move it to the wallet-connect ns or pass the address instead of the session (but then you already have get-account-by-address
that does almost the same thing)
yeah looks good! left just one comment |
Hey @mariia-skrypnyk, I applied changes. but ios pipeline has issue. whenever they resolve the problem. it's ready for test again. |
@mohsen-ghafouri thanks ! Ping me please in case you found first that iOS build is ready. |
920c585
to
6870d86
Compare
Hey @mariia-skrypnyk it's ready for test. |
88% of end-end tests have passed
Expected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (7)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
|
Thanks @mohsen-ghafouri ! PR can be merged! |
6870d86
to
c04bbcf
Compare
fixes #20828
Summary
rel to this closed PR #20870
In this PR we show toasts when:
Areas that maybe impacted
status: ready