Flip updated dApp connections flag #3492
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Shadowfiend
force-pushed
the
dapp-connection-flip
branch
from
0e134f5
to
76d8172
Compare
|
Couple of non blocking small bits. Happy to have these as a ticket for next sprint.
It doesn't need to be clickable
|
The verbiage is updated to refer to websites instead of dApps regardless, while the default toggle is updated only if the feature flag is enabled.
|
This fellow should be ready. |
|
Trying to fix the failing e2e tests here led me to an issue that @hyphenized caught @ #3462 (comment) . Need to fix that one as it will trigger anytime the user tries to connect to Taho as Taho (rather than as default) if they have Taho-as-default turned off (by auto-closing the dApp connection popup whoops). |
This runs a path that hides the default-displayed dApp default connection popover that would otherwise block normal dApp connection flow automation.
Also fix the regular dApp Connection test by dismissing the popover for that flow. The first-time dApp Connect test ensures that the popover is displayed the first time, and is not after it is dismissed the first time.
Previously we were auto-dismissing if isDefaultWallet was ever false. This meant if the user toggled the wallet to being not-default, then tried to connect to a dApp, it would immediately dismiss—for example when Taho-as-default is disabled and the user tries to connect to Taho as Injected or as Taho on a website. Now, the initial value when the popup was rendered is also checked, so that the auto-dismissal only occurs if the popup was rendered when the Taho-as-default was true, and then switched to false. This makes the auto-dismissal work when the user changes the default setting in the popup.
In certain cases width in e2e test-triggered popups can be oddly wide, and width/height get out of sync with internal elements, which ultimately leads to test failures.
|
Ok, this should be ready again. |
|
@Shadowfiend, what about that removal of the connection banner from the swap screen? Are we leaving this at it is? |
|
Yep, remaining for now and left for future cleanup. |
|
@hyphenized since you are feature buddy here, can you please review this PR? |
hyphenized
suggested changes
Jul 3, 2023
e2e-tests/dapp-connect.spec.ts
Outdated
| await page.locator('text="Settings"').click() | ||
| await page.locator("text=Connected websites").click() | ||
| // Clear the popover. | ||
| const bgLocator = popupPage.locator(".bg") |
There was a problem hiding this comment.
Can we target the "void space" button instead? Something like this should work:
popupPage
.getByRole("button", { name: "Background close" })
.click()There was a problem hiding this comment.
I think that was my original approach and then I ran into issues that I thought were related but ultimately weren't, and then never changed it back. I'll poke at it.
| await walletPageHelper.hideDappConnectPopup() | ||
|
|
||
| const dappPage = await context.newPage() | ||
| await dappPage.goto("https://swap.cow.fi/") |
There was a problem hiding this comment.
Probably worth running one small dApp along hardhat for some standard tests of this kind
There was a problem hiding this comment.
Would really love to avoid more stuff we have to maintain tbh, but agree this is a pretty kludgey way to do it.
|
Updated the last bullet to reference app.eigenlayer.xyz instead of .com. |
Previously toggling default to off with only Taho installed would try to route to the previous provider, which was also Taho. This would lead to a second popup from Taho appearing. We now check that the new provider is not the same as the Taho provider and, if it is, then we skip rerouting and error out.
When the wallet started as non-default and was toggled to default, the fact that the wallet was now default wasn't tracked. This meant that the auto-close behavior if default was toggled back off was not triggering. This is a fairly niche edge case, but should now work correctly.
This instead of clicking on the background element itself, which is supported for user convenience but isn't really semantic.
This is fixed. I also made it so the wallet will correctly auto-close in this scenario. The remaining edge case here is:
This seems vanishingly unlikely, and I suggest we don't spend any time trying to fix it unless we get a lot of user confusion, even though my engineering brain wants to solve it. |
hyphenized
approved these changes
Jul 8, 2023
kkosiorowska
pushed a commit
that referenced
this pull request
Jul 13, 2023
…cases (#3546) ## 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](https://discord.com/channels/808358975287722045/888500174685614090/1127318749746311258)). - 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](https://github.com/tahowallet/extension/suites/14241880950/artifacts/799754754) (as of Wed, 12 Jul 2023 12:06:52 GMT).
Merged
kkosiorowska
pushed a commit
that referenced
this pull request
Jul 14, 2023
## 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cc @VladUXUI for design QA
e2e test was also added for the initial display of the Connect to Taho hint popover.
Testing
Fixes #3430.
Fixes #2500. (Sort of—Taho is now directly an option, but if we hack
window.tally.isTally = falseand try with Taho set as default, Taho works correctly. Only important insofar as the bug is no longer relevant.)Fixes #3453.
Latest build: extension-builds-3492 (as of Sat, 08 Jul 2023 07:08:43 GMT).