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

Allow setting mockWalletAddress via ui #578

Closed
1 of 5 tasks
sakulstra opened this issue Mar 25, 2022 · 13 comments · Fixed by #1178
Closed
1 of 5 tasks

Allow setting mockWalletAddress via ui #578

sakulstra opened this issue Mar 25, 2022 · 13 comments · Fixed by #1178
Assignees
Labels
feature New feature or request priority:medium

Comments

@sakulstra
Copy link
Collaborator

sakulstra commented Mar 25, 2022

Is your feature request related to a problem? Please describe.
There's a really cool feature in the app allowing you to "simulate" any address, but if you don't know it you don't find it as you manually have to enter things in the console ( localstorage.setItem('mockWalletAddress', '0x0') ).

Describe the solution you'd like
A input somewhere where i can enter & also remove the mock address.

  • Allow setting watched wallet address from UI
  • Handling for invalid or blank address
  • Remove "Switch Network" error from modals while in watch wallet mode
  • Block tx buttons, display warnings, and add ability to copy tx data
  • Adding Switch Wallet button for UX improvement
@sendra sendra added feature New feature or request design labels Mar 29, 2022
@defispartan defispartan assigned foodaka and unassigned iamanastasia Jul 20, 2022
@defispartan
Copy link
Collaborator

watchWallet2

watchWallet1

@defispartan
Copy link
Collaborator

Still needs:

  • Expected behavior on submitting transaction (new popup with to/from/data potentially linking to MetaMask or Tenderly)
  • Ability to share profile link
  • In current iteration it looks like the only way to disable watch mode is by disconnecting wallet, it might be helpful to have disconnect/update functionality in the profile header, so you don't have to constantly disconnect/reconnect to switch between wallets

Other notes:

  • When URL is passed with address query parameter ("&address=..."): switch app data provider to watch mode and remove parameter from URL

@sakulstra
Copy link
Collaborator Author

@defispartan I think that mockup on the wallet modal is super good.

Not convinced about the 👁️ though. At least to me it wouldn't be clear that it's a mock. Also as you wrote above would be nice to have a quick way to exit the mock.

@foodaka
Copy link
Collaborator

foodaka commented Jul 20, 2022

Actually there are a lot of screens left out of the design, mock mode would be shown the same way test mode is with a purple button where you can exit

@iamanastasia
Copy link
Contributor

@foodaka 
On your last comment — IMHO it’s not the best option to re-use Testnet label, because Watching mode can be applied to testnet too. So both of these features shouldn't be mixed via UI elements.


@sakulstra why you hate 👁 ?:) It's not about 'Mock address' more of 'Watch-only' mode. Eye -> watch.


@defispartan

  1. There is already an ability to share address via Profile -> Copy address. Or were you referencing URL thing?
  2. About quick disabling the Watch-mode. Do you mean a flow, when user connects to his personal wallet, then enables Watching mode and enters 3rd party address. Then when he is finished, he just disables watching mode and goes back to his connected account? Like the Testnet mode?
  3. Agree about tx

@iamanastasia
Copy link
Contributor

I imagine this flow would work like switching between accounts. And we may save 1 click for user if we add 'Switch wallet/Change' button:
Modal
wallet — connected
wallet watch mode

@MatthewCYLau
Copy link
Contributor

👋 Still new to the code base, and I'm happy try deliver this feature as it does not seem too difficult (of course unless I am missing something 😅 )

  • Add the Watch Address input field
  • On click, invoke localstorage.setItem('mockWalletAddress', '0x0')
  • Create a new context - isWatchOnlyMode; render warning / 👁️ icon accordingly

@iamanastasia
Copy link
Contributor

Hey @MatthewCYLau :) Maybe we also need to cover Profile dropdown (to switch between accounts) and decide on tx window: one option is to allow user open tx window to play around with options (but with disabled CTA).

And since nobody liked 👁️ icon let's just switch to alert exclamation. I'll send the designs later

@iamanastasia
Copy link
Contributor

Connect wallet window tooltip:
Connect wallet

TX window message
Borrow message

Mobile views
Mobile menu
Mobile dashboard

Profile dropdown (desktop)
Dashboard Profile dropdown

@MatthewCYLau let me know what you think :)

@MatthewCYLau
Copy link
Contributor

@iamanastasia thank you for the design above - lgtm 🎉 now let me try code it out 💪

@MatthewCYLau
Copy link
Contributor

Raised a draft PR. I'm happy to take feedback (on this thread?)

@defispartan defispartan linked a pull request Aug 26, 2022 that will close this issue
@defispartan
Copy link
Collaborator

Raised a draft PR. I'm happy to take feedback (on this thread?)

This is a great start! @iamanastasia Left feedback on the PR, I added these as tasks to this issue and added one more myself which is the ability to copy the approval and/or transaction data while in watch wallet mode.

Since this is quite a big change, don't feel like you have to do all of this yourself @MatthewCYLau 😅 , our team can pick up some of these tasks next week

@MatthewCYLau
Copy link
Contributor

Thank you for all the great feedback from the core Aave team 🙏I’m on a city break get away and will be back on Monday 5th September. Could I still own these tasks myself when I return? Thank you!

@drewcook drewcook linked a pull request Sep 27, 2022 that will close this issue
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request priority:medium
Projects
None yet
6 participants