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

Proxy Wars: More QoL improvements for dApp connections #3613

Merged
merged 15 commits into from
Nov 3, 2023
Merged

Conversation

Shadowfiend
Copy link
Contributor

@Shadowfiend Shadowfiend commented Sep 1, 2023

This PR:

Additionally, it introduces logging for MetaMask-impersonated requests that includes timing information and parameter/return value/error value information:

Screenshot 2023-09-01 at 00 36 14

This is similar to existing request/response logging in dApps, but it:

  • Only happens on development builds.
  • Distinguishes between calls handled by Taho and calls handled by MetaMask or the MetaMask mock (if MetaMask is not installed).
  • Intercepts all function calls and return values, not just JSON-RPC requests, allowing us to identify diverging behaviors with MM more easily.
  • Highlights errored calls.
  • Includes call and return value in one line.
  • Includes how long the call took to complete.
  • Only reports the call without the return value if it takes >1s to complete.
  • Reports long-running calls every second with a note on how long the call has been pending.

Latest build: extension-builds-3613 (as of Thu, 02 Nov 2023 22:01:18 GMT).

This continues to be managed in the window provider's mocking paths,
but the cases where this is pulled from the metaMaskMock are expanded.
When Taho is set as default, MetaMask-specific properties are pulled
from the mock. This was already effectively happening for `isMetaMask`,
but is now expanded to include _metamask and _state, which are also
sometimes used to recognize MetaMask.
When a pending request is detected, new permission requests are
immediately rejected, preventing cascading request popups for the user.

Additionally, allow for a permanent permission denial to be recorded in
the database. This isn't currently exposed in the UI, but follows from
the implementation of disallowing parallel permission requests.
Two core changes here:

- When proxying MetaMask calls, properties that resolve to functions are
  wrapped such that they maintain their `this` pointer at the underlying
  object rather than the proxy.
- In development builds, all calls intercepted by the MetaMask wrapper
  are logged. This logging includes a timer log every 1s for longer-
  running calls, and applies both when Taho is set as default and when
  MetaMask is being called directly.
@Shadowfiend Shadowfiend marked this pull request as draft September 1, 2023 04:39
The `showExtensionPopup` helper previously returned the created window
object, but offered no easy way to monitor for the closing of the
window. We monitor for such a closing in more global ways for signature
requests, mediated by the Main service; however, there are use cases
where we need to manage internal state in the ProviderBridge service in
a self-contained way.

To allow this, a new `onClose` parameter is added to
`showExtensionPopup`; it is a function that will be called when the
popup window created by the function call is closed, no matter how it
was closed.
This effectively rejects the permission request, and allows for future
permission requests to come through normally. Previously, because this
state was not cleared, the request was not treated as rejected from the
dApp perspective (meaning the dApp would stay in a pending state that
required a reload to recover from), and due to recent changes to
disallow multiple parallel permission requests, future requests were
immediately denied.
Previously, `#pendingPermissionRequests` had its value set in
`requestPermission` but not cleared there, so it was hard to follow
where that field was being managed. `requestPermission` now manages the
full lifecycle of that value. Other places that were handling clean now
only handle resolving the promise.
The actual implementation is a little more subtle: permissions are for
an origin AND a network. If the permission being cleared is for the
origin's current network, as tracked in the `InternalEthereumProvider`,
then that current network is cleared so that future interactions can set
it.
@beemeeupnow
Copy link
Contributor

anything needed for this? testing?

wallet_requestPermissions is defined by EIP-2255, but does not seem to
have much use outside of an alternative to `eth_accounts` and a way to
re-prompt the user for access to more/different accounts.

For the time being, `wallet_requestPermissions` is implemented exactly
like `eth_requestAccounts`, and `wallet_getPermissions` exactly like
`eth_accounts`, except that the return value is structured somewhat
differently.

If and when Taho supports connecting multiple accounts to the same dApp
simultaneously, we can extend this model with two things:

- Always prompting the user to update their connected accounts when
  `wallet_requestPermissions` is invoked.
- Returning more than one account in the `wallet_getPermissions`
  `restrictedReturnedAccounts` caveat.
@Shadowfiend
Copy link
Contributor Author

anything needed for this? testing?

Testing, yes! Hoping to ship this fella next week.

…App permissions (#3616)

Draft until I fill in some testing details.

The actual implementation is a little more subtle: permissions are for
an origin AND a network. If the permission being cleared is for the
origin's current network, as tracked in the `InternalEthereumProvider`,
then that current network is cleared so that future interactions can set
it.

Latest build:
[extension-builds-3616](https://github.com/tahowallet/extension/suites/15762123848/artifacts/899755021)
(as of Sat, 02 Sep 2023 20:14:11 GMT).
@Shadowfiend Shadowfiend marked this pull request as ready for review October 30, 2023 13:57
@andreachapman
Copy link
Contributor

@Shadowfiend - some notes below from testing:
Curve DOA:

  1. Tested without MM installed and with TAHO not set to default - See Tally as a connect wallet option: connected without issue - https://github.com/tahowallet/extension/assets/135037801/4a1765ea-ecb0-4e9a-b80f-393ff721f615 - but it seemed weird that I saw "Tally" - is that right?

  2. Testing without MM installed and TAHO as default: don't see Tally as an option, see MM, can use that to connect - https://github.com/tahowallet/extension/assets/135037801/b0aea4b3-96e8-42f8-9645-028d102bf70b

  3. MM enabled and Taho as Default: I see the option to connect via MM and clicking that opens Taho and I can connect normally.

  4. With MM enabled and Taho not as default: I see the MM option for connecting the wallet and when clicked, the MM extension comes up to connect - as expected

guild.xyz:
After connecting a wallet to the site in a different browser, when I return to the first browser I connected on, I do see a message that my session expired and it's asking me to verify again Screenshot 2023-10-30 at 10 48 34 AM

So this one seems to behave as expected now.

Support EIP 2255:
I didn't get definitive proof that this is working. It looks like you mentioned that "Ethermail no longer requires wallet_requestPermissions" and so I checked the other user-reported site with this issue, skurpytown.com. Here's what I saw:

  1. With Taho not as default - the Taho extension won't come up, it just tells me that I need to install MM
  2. With Taho set as default - it recognizes it and bring up the extension to connect and it connects fine
    Screenshot 2023-10-30 at 10 57 22 AM

What I'm not sure about though is if that site really tests the wallet_requestPermissions either. I didn't see that in the console, I saw: content: inpage > background: {"id":"4","target":"tally-provider-bridge","request":{"method":"eth_chainId"

"#3599 by clearing current network state when a disconnect on a matching account and network occurs. This can be tested by connecting to a dApp on a network, disconnecting, switching network, and attempting to connect again. Previously, the new connection would be on the original network, now it should be on the currently-selected network.":

I connected to opensea.io on Polygon. I then disconnected in opensea.io and in the extension switched to Ethereum. I connected in opensea.io again but it connected on Polygon instead of Ethereum. I did essentially the same tests except on https://skurpytown.com/ in Brave. I connected on Optimism, disconnected, switched to Polygon in the extension, actually closed the tab for https://skurpytown.com/ and opened it in a new tab and connected again - it was still on Optimism:

Screen.Recording.2023-10-30.at.11.17.18.AM.mov

However, I was finally able to get it to behave as expected when I went to background inspector > indexed db > internal-ethereum-provider > currentNetwork and I deleted skurpytown.com. It's still getting indexed that first time and then once I clear it from there, it doesn't happen again. I recreated it in this video with opensea.io. You can see it indexed when I start, that's from doing the test the first time. Then I delete the entry and run the test again and you can see the network correctly switch from Polygon to Optimism when I reconnect. The indexing isn't happening again after that first time, I can see that in the background page - it happens once then after I clear it, it seems to be good.

opensea.mov

@beemeeupnow
Copy link
Contributor

"#3599 by clearing current network state when a disconnect on a matching account and network occurs. This can be tested by connecting to a dApp on a network, disconnecting, switching network, and attempting to connect again. Previously, the new connection would be on the original network, now it should be on the currently-selected network.":

I connected to opensea.io on Polygon. I then disconnected in opensea.io and in the extension switched to Ethereum. I connected in opensea.io again but it connected on Polygon instead of Ethereum.

Did you disconnect via the lightning bolt button in the wallet? That is the action that should clear the association since it is revoking the site permissions. Disconnecting in the dapp UI isn't expected to clear it, from what I understand.

@andreachapman
Copy link
Contributor

"#3599 by clearing current network state when a disconnect on a matching account and network occurs. This can be tested by connecting to a dApp on a network, disconnecting, switching network, and attempting to connect again. Previously, the new connection would be on the original network, now it should be on the currently-selected network.":

I connected to opensea.io on Polygon. I then disconnected in opensea.io and in the extension switched to Ethereum. I connected in opensea.io again but it connected on Polygon instead of Ethereum.

Did you disconnect via the lightning bolt button in the wallet? That is the action that should clear the association since it is revoking the site permissions. Disconnecting in the dapp UI isn't expected to clear it, from what I understand.

Yea, should have noted that. I tried disconnecting from within the extension and from within the dapp (just to try it all) - neither case gave me the results I expected until I cleared the entry from the currentNetwork index and then when I do that, it works like a charm.

@Shadowfiend
Copy link
Contributor Author

I might need to ride shotgun with you to understand this one better. Clearing the index is what should be happening in the first place (if, as beem notes, you disconnect from the lightning bolt in the extension), so having to do it manually first is puzzling.

@andreachapman
Copy link
Contributor

I might need to ride shotgun with you to understand this one better. Clearing the index is what should be happening in the first place (if, as beem notes, you disconnect from the lightning bolt in the extension), so having to do it manually first is puzzling.

I have to leave soon but here's another video showing me using the lightning bolt to disconnect so hopefully that helps clarify to some degree and tomorrow I can definitely sit together to look at it. In this video, you can see that after I manually delete the indexed network, the dapp switches from Optimism to Polygon automatically. Polygon is what is was on when I connected the 2nd time and what it should have connected on. But instead, I had to manually delete that entry before it switched.

lightningbolt.mov

And I think this will make it less confusing: it's still always doing the indexing for me. I had said it only does it once but now I can't recreate that scenario. Now, it's always indexing and the automatic switch when connecting on a different network won't happen unless I delete that entry.

Should it be storing anything here at all after this change? Because these are all ones that have been stored since I started testing on this version today (ones that I haven't deliberately cleared yet). Or is it supposed to be storing them but then deleting/re-creating when we disconnect? I'm trying to understand if I should or should not see anything there with this change.
Screenshot 2023-10-30 at 12 51 05 PM

@Shadowfiend
Copy link
Contributor Author

Yeah on disconnect, it should be deleting them. I'll poke at it on my end as well. Odd that we're not seeing it…

background/services/internal-ethereum-provider/db.ts Outdated Show resolved Hide resolved
background/services/provider-bridge/index.ts Outdated Show resolved Hide resolved
background/services/provider-bridge/index.ts Show resolved Hide resolved
src/window-provider.ts Show resolved Hide resolved
The property in the db was at network.chainID, looking at chainID was
referencing an earlier version of the schema.
jagodarybacka
jagodarybacka previously approved these changes Nov 2, 2023
Copy link
Contributor

@jagodarybacka jagodarybacka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks ok, @andreachapman up to you if you prefer to merge it and do release testing or test it here again.

@Shadowfiend
Copy link
Contributor Author

Looks like the latest build here doesn't include the fix from main for some reason…

@Shadowfiend
Copy link
Contributor Author

Rolling this in due to positive feedback from a couple of testers. We'll test the whole shebang together in a release PR.

@Shadowfiend Shadowfiend merged commit 0afb307 into main Nov 3, 2023
5 of 6 checks passed
@Shadowfiend Shadowfiend deleted the proxy-wars branch November 3, 2023 01:33
@Shadowfiend Shadowfiend mentioned this pull request Nov 3, 2023
Shadowfiend added a commit that referenced this pull request Nov 7, 2023
## What's Changed
* v0.50.0 by @jagodarybacka in
#3643
* Unsettled Networks: Clear an origin's current network when revoking
dApp permissions by @Shadowfiend in
#3616
* Sepo-Ledger: Patch @ledgerhq/hw-app-eth for Arbitrum Sepolia chain id
handling by @Shadowfiend in
#3650
* Proxy Wars: More QoL improvements for dApp connections by @Shadowfiend
in #3613


**Full Changelog**:
v0.50.0...v0.51.0

Latest build:
[extension-builds-3652](https://github.com/tahowallet/extension/suites/17870144186/artifacts/1025746882)
(as of Fri, 03 Nov 2023 02:06:50 GMT).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants