search

hicommonwealth / commonwealth

A platform for decentralized communities
https://commonwealth.im
GNU General Public License v3.0
66 stars 40 forks source link

Metamask not selecting the right address #5112

Open sachben91 opened 9 months ago

sachben91 commented 9 months ago

Steps to replicate

Loom

Expected behavior

sachben91 commented 9 months ago

This could possibly be user error because sometimes in Metamask, an address showing as selected need not mean its connected as shown in the below screenshot.

Image

ianrowan commented 9 months ago

Expanding on this issue. On first run of session keys locally I was signed into my Profile but the two addresses in the address switcher were wrong. Upon signing out And signing back in I anonymous but with the same two incorrect addresses. (My address is supposed to be 0xE64..)

image

Will investigate the whole metamask login as part of this

CC @jnaviask @CowMuon @sachben91

ianrowan commented 9 months ago

@sachben91 The behavior you mentioned in this ticket is expected given Metamask UX. I messed around with it on beta and it all worked as expected.

For context metamask requires manual confirmation to 'connect' a new address to any site when switching in the extension. Whichever account is 'active'(not necessarily the one selected in the extension) is where requests from our code will be sent to(ie if Account 1 is active but account 2 is selected in extension, a signature request will go to account 1). In other words our code isnt allowed to request an account that has not been explicitly connected in the extension.

I think this can close this issue. What I mentioned above seems related to some other changes on master that are not on beta right now, hence why I didnt have the same issue when trying to reproduce this. Above issue is probably related to the re-styling done to the address switch and would need context on how/who implemented the new style etc.

CC @jnaviask @CowMuon

jnaviask commented 9 months ago

@ianrowan I've seen Metamask flows that prompt me to connect -- is there any way we can leverage Metamask's API here to avoid requiring manual steps?

ianrowan commented 9 months ago

@jnaviask The flow described does prompt users to connect, the issue only occurs if they do not choose to connect upon switching their account(talking in the context of the extension UX here). The user must select connect when this prompt comes up upon selecting accounts

image

Connecting is required to allow our metamask sdk to have any information about a given account's existence. On the first time signing in to Common or any dapp a user is prompted with a checkbox list of accounts they want to connect and these are the accounts whose actions get exposed to our code from the extension. If a user would like to expose an account to common in the future having not checked the box the first time they must select the option above.

image

The above selection on first sign-in would not allow us to see any info or suggest sigs/txs to account 2 unless we select all

In the extension each domain has a set of permissions for each account with these actions

image

So in summary if an account hasnt been connected we don't know anything about it and can't even suggest a signature

zakhap commented 9 months ago

Above issue is probably related to the re-styling done to the address switch and would need context on how/who implemented the new style etc.

Can you explain a little bit more about what you're saying here? I don't think I follow how a restyle effects much in this case?

Connecting is required to allow our metamask sdk to have any information about a given account's existence.

Right, this makes sense. However, in the cases above, the user has already connected their address to Common, which is why it is present in their account dropdown menu to select from. If we know that to be true, that at some point the user has connected that particular metamask account to common, we should be able to prompt it. The case in which this is not possible is if the user for some reason disconnects/revokes the metamask account from Common (on purpose or not). This latter case should not prompt the user to sign with the wrong address, but should prompt the user to connect the correct address. Question: knowing the correct address, can we prompt metamask to connect the proper account (given the plugin has it)? if not, we will need to display to the user the address they need to connect and give them the "connect" button that opens metamask for selection.

I think it's important to note that within their account, we can never know of an address associated to their account without them previously connecting the address + signing a message. Given this pre-requisite, we should arrive at the ideal flow for signing.

Regarding the case detailed in the ticket, is it correct to say that this "problem case" arose out of the fact that when signing in with address 1 (which is associated to an account with address 2) the user connected address 1 but at no point reconnected address 2? The need for the reconnection here is because we are on a different url than production and that this bug in particular should not impact users of production that have not actively disconnected their metamask account from common. Is this correct?

ianrowan commented 9 months ago

@zakhap

I think we're talking about two different issues here.

  1. The restyling issue was something I noticed running master locally, this wasnt an issue on beta and I'm just saying it should probably be treated as another ticket as it doesnt have anything to do with metamask specifically. I mentioned restyling to allude to a hunch I had that cosmos address calculation logic might accidentally be being applied to evm addresses in the new style. Just a guess so far, but I'm not sure of another reason how evm addresses linked tot my profile would be totally wrong vs what is in my db.

  2. As for the issue in this ticket. Its unrelated to our address selector. The problem Sachen mentioned is related to switching metamask accounts.

To the first question on prompting a connection before we know the account but metamask isnt connected(either different domain or user revoked in metamask) - we can't do this because of the fact that the connection feature in metamask is a security feature to basically defend us/bad actors from doing just that. As noted in the permissions screenshot, unless an address is connected/approved the domain, we can't prompt against it.

And yes on the final point the reason for the issue in this ticket is simply because we're on a different domain(beta). So overall I'm just saying the issue in this ticket is just expected metamask behavior. The other issue mentioned with the address switcher is 100% an issue but also 100% separate from the issue in the ticket. I mainly would like to know the history of the switcher component and when it diverged(with session keys?) to determine the cause and if anyone is already working on that issue

CC @jnaviask

ianrowan commented 9 months ago

Per conversation in office hours, (ticket issue) the issue of this ticket specifically should only involve the resolution of a QA document about connecting metamask accounts to new testing domains.

(convo with Zak) Additionally on @zakhap point about prompting users to connect their other metamask account that is linked to their current logged in profile(either on a new machine or domain), we discussed how the main solution here is just a pop-up that prompts the user to ensure they click connect when switching accounts. This will mainly ensure the use of session keys when the edge case occurs.

(issue I accidentally flagged regarding the UI) I flagged an issue initially about incorrect addresses in the address selector. This is unrelated and does seem related to UI work for session keys. I am seeing the exact same addresses and dydx banner as mentioned here https://github.com/hicommonwealth/commonwealth/issues/4314#issuecomment-1665561702 which makes me assume they are some how hard coded to the flag. I also see these when signing into a totally different profile(via keplr). The conversation makes it looks like fixes have been made but with feature flag on via master I still see this, just fyi

CC @jnaviask

jnaviask commented 9 months ago

@ianrowan @zakhap I'm very confused about the scope of this ticket. What needs to be actually done here?

ianrowan commented 2 months ago

This was recently re-assigned. Requesting to close as this was decided a non-issue with common related to metamask UX(ie connection) and the solution I remember us throwing around was to add to 'User facing Docs'