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.

Sign up for GitHub

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

Jump to bottom

Sticky Defaults: Make Taho-as-default replace MetaMask in almost all cases #3546

Merged
merged 8 commits into from
Jul 13, 2023
Merged

Sticky Defaults: Make Taho-as-default replace MetaMask in almost all cases #3546

merged 8 commits into from
Jul 13, 2023

Conversation

Shadowfiend
Copy link
Contributor

@Shadowfiend Shadowfiend commented Jul 11, 2023 •
edited by github-actions bot
Loading

Background

Up until now, Taho's approach to being marked as default was meant to
be roughly:

  • If Taho is marked as default, and the dApp supports showing a non-MetaMask
    provider at window.ethereum, make Taho that provider.
  • If Taho is marked as default, and the dApp doesn't support showing a non
    MetaMask provider, have Taho impersonate MetaMask so that users can still
    connect using Taho.
  • If Taho is marked as not-default, leave everything untouched, and only
    show Taho for dApps that specifically look for it (typically at window.tally,
    but sometimes in window.ethereum.providers).

For this to work, the window provider was tracking an allowlist of sites to
impersonate MetaMask on, and it was also tracking an allowlist of sites to
do additional tricks on (e.g. returning a blank providers array for sites
that detect MetaMask and don't look for any other wallet). There were other
tricks also being done to try to discern between scenarios, etc.

Other wallets just throw their hands up and attempt to impersonate MetaMask
most/all the time, leading to escalating wars with dApp frameworks to detect
these behaviors.

Revised Approach

This PR throws all of that away. It was confusing for users (do you click on
MetaMask? Browser/Injected? What happens if Taho isn't default? What about
the times when MetaMask impersonation isn't hardcoded for a site?), and it
was difficult to maintain (allowlists of specific sites are not realistically
a scalable long-term approach).

Instead, having Taho set as default (a reminder that as of #3492, this is
represented in the UI as a toggle between “connect with MetaMask” and “connect
with Taho”) should mean two things:

  • Trying to connect to MetaMask will always use Taho.
  • For dApps that also specifically detect Taho, they may also allow connecting
    with Taho. If Taho is set as default, attempting to connect with MetaMask will
    still connect with Taho.

On the flip side, if Taho is not set as default, it should mean two things:

  • Trying to connect to MetaMask will always use whatever was presenting
    as MetaMask before Taho got involved.
  • For dApps that also specifically detect Taho, they will allow connecting
    with Taho still.

Technical Details

How we do this is straightforward: any provider on the providers array
that presents as MetaMask (our heuristic: isMetaMask returns true AND
there are no other is* methods on the provider) is wrapped in a one-time
proxy. That proxy passes everything through to the wrapped provider, unless
Taho has been set to present as MetaMask by the user (an explicit choice).
In this case, a subset of functions are always redirected to Taho, including
account and RPC requests. Additionally, any calls to isMetaMask on
window.ethereum return true if Taho is set to default, even though the
provider presented on window.ethereum is the Taho provider.

We do two more small things:

  • When Taho is set as default, it is set as the first provider in the providers
    array. When it is not, it is removed from the front of the providers list. This
    allows MetaMask or other wallet usage for dApps that use a framework like wagmi,
    which especially in older versions only used the first wallet in the providers
    array for connections. Examples of dApps that show this behavior include Lens
    and Vela (e.g. see this bug report in Discord).
  • When Taho is set as default by the user and MetaMask is not installed, a mock
    MetaMask is presented to the dApp that reroutes calls to Taho. If Taho is
    switched back to not default, the mock is removed. This allows dApps like
    bitcoinbridge.network that only allow connecting via MetaMask to work with
    Taho even if MetaMask is not installed.

The net result is that the “act as MetaMask”/“Taho as default” toggle should
behave consistently across all dApps. Famous last words :)

Finally, we fix a small issue with eth_requestAccounts handling on certain dApps.
Not related to MetaMask impersonation really, but knockin' bugs out.

Testing

  • This is going to require doing some relatively extensive testing in our
    release QA dApps to make sure that MetaMask is replaced when Taho is set
    as default and not when
    it is not.
  • All these should also be replicated with Taho as default and not and MetaMask
    disabled (to ensure the dApp registers MetaMask as uninstalled when Taho is
    not default, and allows interaction as MetaMask when Taho is default).
  • We should also check Lens (https://claim.lens.xyz/) to make sure the Browser
    connection uses Taho when default, and MetaMask when not.
  • Finally, the dApps in the issues below should be tested to make sure they're
    all working!

Fixes #3433, fixes #3412, fixes #3368, fixes #3242, fixes #3231, fixes #2506, fixes #2706, fixes #2818.

Latest build: extension-builds-3546 (as of Thu, 13 Jul 2023 09:38:23 GMT).

@Shadowfiend
Make TallyWindowProvider compatible with TahoProvider
13589ff 
Also do some internal renaming from Tally to Taho in a few places that
aren't externally-facing.
@Shadowfiend
Wrap MetaMask-like providers with a proxy for default handling
4279eb8 
Rather than try to intercept and swap out or filter provider lists, we
now always wrap any provider that looks like MetaMask (right now we
look at isMetaMask + make sure there are no is* properties on the
provider) with a proxy. That proxy always forwards requests to Taho if
the user has set Taho as default, meaning they've explicitly opted into
replacing MetaMask with Taho.
@Shadowfiend
Masquerade isMetaMask without breaking Taho detection
af75805 
When Taho is set as default, the wallet router now reports isMetaMask =
true, while the Taho provider always reports isMetaMask = false. This
allows dApps that support Taho as a first-class provider to see it
as itself. In general the Taho provider is less concerned with
masquerading as MetaMask, leaving that to the MetaMask wrapper proxy and
the wallet router.

Additionally, the wrapper only routes an allowlist of methods to Taho to
minimize divergence of behavior from MetaMask when it is installed.
@Shadowfiend
Rearrange ethereum.providers based on Taho default setting
5463bc5 
Some frameworks, like Wagmi, always display the first item in the
providers list as the injected/browser wallet. Some sites that do this
also don't check the providers list for other wallets, so that even if
MetaMask is in the providers list, the MetaMask button shown to the user
doesn't actually bring up MetaMask, instead showing the MetaMask mobile
connection option.

To get around this, the providers list is now rearranged when Taho is
switched to or from default. When it's set as default, it is placed at
the front of the providers list; when it's set as not default, the
previous provider it replaced is set as first.
@Shadowfiend
Add MetaMask mock when Taho is default and no MetaMask is present
189eba8 
For the specific case where Taho is set as default but MetaMask does not
seem to be installed, we now inject a MetaMask-alike in the providers
list to allow dApps that only look for MetaMask to work correctly.

When Taho is cleared from being default, the MetaMask-alike is removed
and the fact that MetaMask is not installed is left as-is for the dApp
to detect normally.
@Shadowfiend Shadowfiend requested a review from a team July 11, 2023 21:20
@Shadowfiend Shadowfiend mentioned this pull request Jul 12, 2023
Open
@Shadowfiend
Fix this references in proxy forwarding
ada7635 
While no observed behaviors broke on this, the existing invocation
strategy could lead to failure if provider-private variables were being
accessed, due to a shift in the `this` reference.
@Shadowfiend
Force params for eth_requestAccounts calls from dApps
285ebe1 
Some dApps were sending an extraneous parameter up front as if they were
expecting to call wallet_requestPermissions. We now discard that
parameter when attaching the dApp title and favicon in the content
script.

wallet_addEthereumChain remains unchanged.
jagodarybacka
jagodarybacka reviewed Jul 12, 2023
View reviewed changes
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.

Looks good, will take a second look at it tomorrow

window-provider/index.ts
@@ -141,7 +143,7 @@ export default class TallyWindowProvider extends EventEmitter {
}

if (isTallyConfigPayload(result)) {
const wasTallySetAsDefault = this.tallySetAsDefault
const wasTallySetAsDefault = this.tahoSetAsDefault
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we try to rename variables and functions with tally as much as possible here?

Suggested change
const wasTallySetAsDefault = this.tahoSetAsDefault
const wasTahoSetAsDefault = this.tahoSetAsDefault

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I agree. Let's do it next steps.

kkosiorowska
kkosiorowska reviewed Jul 13, 2023
View reviewed changes
Copy link
Contributor

@kkosiorowska kkosiorowska 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 good, I'm getting down to testing.

@kkosiorowska
Copy link
Contributor

kkosiorowska commented Jul 13, 2023 •
edited
Loading

Tests notes:

Following issues:

dApps:

kkosiorowska
kkosiorowska approved these changes Jul 13, 2023
View reviewed changes
Copy link
Contributor

@kkosiorowska kkosiorowska left a comment

Choose a reason for hiding this comment

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

Everything seems to work well with the exception of two issues (#2706 and #2818). Let's not block this and include it in the upcoming release because it makes for a big bang improvement for our users. 🚀

@kkosiorowska kkosiorowska merged commit 644c936 into main Jul 13, 2023
5 checks passed
@kkosiorowska kkosiorowska deleted the sticky-defaults branch July 13, 2023 09:32
This was referenced Jul 13, 2023
Merged
Open
kkosiorowska pushed a commit that referenced this pull request Jul 14, 2023
v0.42.0 (#3549)
2edd8ea 
## What's Changed
* Add private key onboarding flow by @jagodarybacka in
#3119
* Private key JSON import by @jagodarybacka in
#3177
* Allow export of private keys and mnemonics by @jagodarybacka in
#3248
* Export private key form by @jagodarybacka in
#3255
* Unlock screen for the account backup by @kkosiorowska in
#3257
* Show mnemonic menu by @jagodarybacka in
#3259
* Fix background blur issue by @jagodarybacka in
#3265
* Account backup UI fixes by @jagodarybacka in
#3270
* Fix unhiding removed accounts by @jagodarybacka in
#3282
* New error for incorrectly decrypted JSON file by @jagodarybacka in
#3293
* Export private keys from HD wallet addresses by @jagodarybacka in
#3253
* Refactor keyring redux slice to remove `importing` field by
@jagodarybacka in #3309
* 📚 Accounts backup by @kkosiorowska in
#3252
* Catch Enter keypress on Unlock screen by @jagodarybacka in
#3355
* Rename `keyring` to `internal signer` and other improvements by
@jagodarybacka in #3331
* 🗝 QA - Accounts backup and private key import by @jagodarybacka in
#3266
* Remove private key signers if they are replaced by accounts from HD
wallet by @jagodarybacka in
#3377
* RFB 4: One-Off Keyring Design by @Shadowfiend in
#3372
* Copy to clipboard warning by @kkosiorowska in
#3488
* Allow setting custom auto-lock timer by @hyphenized in
#3477
* Use Argon2 for encrypted vaults by @jagodarybacka in
#3502
* 👑 Private keys import and accounts backup by @jagodarybacka in
#3089
* Untrusted assets should not block the addition of custom tokens by
@kkosiorowska in #3491
* Flip updated dApp connections flag by @Shadowfiend in
#3492
* v0.41.0 by @Shadowfiend in
#3531
* Switch to a given network if adding a network that is already added.
by @0xDaedalus in #3154
* Remove waiting for Loading Doggo component in E2E tests by
@jagodarybacka in #3541
* Squeeze content to better fit on Swaps page by @jagodarybacka in
#3542
* Refactor of terms for verified/unverified assets by @kkosiorowska in
#3528
* Fix ChainList styling by @fulldecent in
#3547
* Update release checklist by @jagodarybacka in
#3548
* Fix custom asset price fetching by @hyphenized in
#3508
* Sticky Defaults: Make Taho-as-default replace MetaMask in almost all
cases by @Shadowfiend in
#3546

## New Contributors
* @fulldecent made their first contribution in
#3547

**Full Changelog**:
v0.41.0...v0.42.0

Latest build:
[extension-builds-3549](https://github.com/tahowallet/extension/suites/14268975651/artifacts/801826435)
(as of Thu, 13 Jul 2023 09:51:56 GMT).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jagodarybacka jagodarybacka jagodarybacka left review comments

@kkosiorowska kkosiorowska kkosiorowska approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@Shadowfiend @kkosiorowska @jagodarybacka