Group wallets by protocol and service #57
Conversation
Deploying with
|
| Latest commit: |
e3d7c31
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cbfaba7f.swim-ui.pages.dev |
There was a problem hiding this comment.
Looks great! Only commenting the result, not the code:
- The
ConnectButtonisn't working properly yet, claims e.g. Ethereum is not connected - The
MultiConnectButtonis now showing weird logos, might make sense to also work on this ticket at the same time
wormat
changed the title
|
Answering the question: it should disconnect for all EVM chains. |
Co-authored-by: Will <86618292+wormat@users.noreply.github.com>
…-to-divide-by-protocol Merging since otherwise I need to update files that are going to be removed
|
@arvakr your requested changes have been updated. I don't know if you have any suggestion on the "Decide if we want to keep displaying "4 wallets are connected" when we have only metamask connected" UI/UX concern I have. See screenshot below:
@wormat @cyphertrace @ramenuncle this PR is missing a few things (the unchecked checkboxes in the description, tests and selectors for the new store) but I would like to get some feedback on the store implementation. Random people on the internet say it's OK for one store to access the other directly and not via a hook so I went with this approach but since this is a first I would like to hear if you have any concerns. I also avoided a |
| ) => Promise<void>; | ||
| } | ||
|
|
||
| export const useWalletService = create<WalletServiceState>( |
There was a problem hiding this comment.
I thought also that these stores, that don't need persistence, should be just one of the slices of the global store.
Check this : https://github.com/pmndrs/zustand/wiki/Splitting-the-store-into-separate-slices
There was a problem hiding this comment.
Do we have a global store? Do we want one? Should we put notifications there too?
There was a problem hiding this comment.
I thought it could be convenient to have one store, sliced per domain, so we can access all data easily.
Just persisted part of the store, we had to put as a separate store, as it's not possible to slice it and have it in global.
For the notification, I took approach to separate it also, cause I didn't feel the need to be in global, just rendered on it's own on the fly, when one component, page needs it.. better said, I didn't thing that we're going to need it in the other stores. :)
There was a problem hiding this comment.
I think it probably makes sense to move the wallets context inside this one for sure but I would prefer to do it on a separate PR if that's OK since this is getting huge. Regarding one vs many stores, I think for now I would prefer to default to keep them separate unless they are coupled in some way but I definitely agree that many would be coupled.
So keeping it as is, adding tests for the stores and pinging you for a final review soon!
@nicomiicro It's all external to |
|
@arvakr @wormat thoughts on reverting the UI part of this to match the existing master until we reach a consensus with product so that we can merge this and move on with the remaining wallet refactoring (e.g. ConnectionProvider/WalletProvider)? The only difference in UX would be that all metamask wallets will appear connected once one of the is clicked/connected. I've also suggested another option today with this info icon button / popover
|
This is still work in progress, publishing so that I get some feedback on the general direction since I feel I don't know what I'm doing :)
Notion ticket: https://www.notion.so/exsphere/Rearrange-MultiWalletModal-to-divide-by-protocol-f46bf54422254ef5bbbf0e7552bd3b08
TODO
Questions
One theSince it's the same instance, it will disconnect all.ConnectButton, what should happen when someone disconnects for metamask/ethereum? Should all wallets with service metamask disconnect? Or just the ethereum wallet?