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

Support Phantom wallet #698

Open
wants to merge 54 commits into
base: dev
Choose a base branch
from
Open

Support Phantom wallet #698

wants to merge 54 commits into from

Conversation

maximgeerinck
Copy link

@maximgeerinck maximgeerinck commented Apr 7, 2023

Motivation and context

Add support for phantom wallet.

List of current features:

  • Connect wallet
  • Sign message
  • Cancel signing
  • Wallet setup
  • Copy eth address

EXTRA:

  • Multiple providers: Can use metamask and phantom together in 1 session using the PROVIDERS environment variable
  • Supports versions using @
PROVIDERS=phantom,[email protected] npx playwright test

TODO:

  • Phantom to provide public releases (Right now only accessible by providing strongly protected PATs)

Quality checklist

  • I have performed a self-review of my code.

Future Ideas

  • Would be nice if these providers can be classes that extend from an abstract class
  • We will look into more generic selectors (like proxying getByText, ... for accessing more generic things in dialogs)
  • Typescript support

@neuodev neuodev requested review from neuodev and drptbl April 7, 2023 22:52
@neuodev neuodev self-assigned this Apr 7, 2023
@neuodev
Copy link
Contributor

neuodev commented Apr 20, 2023

Hello, @maximgeerinck 👋

If I am getting it correctly this Phantom Wallet support PR is not finished because there is no way to download phantom wallet as it's not open-source. The plan from your side was to wait until it is open-sourced.

Just want to let you know that you can use this chrome extension to extract the source code from any chrome extension as ZIP file.

Also, you can use this website to download the source code.

Also, check this GitHub gist on how to download the source code for any Chrome extension.

CRX Viewer will allow you to download the source code for any Chrome extension (repo)

Thanks, please let me know if you need any help!

@maximgeerinck
Copy link
Author

We've added a way to download a phantom crx and install it

@maximgeerinck maximgeerinck marked this pull request as ready for review June 9, 2023 09:36
Copy link
Contributor

@neuodev neuodev left a comment

Choose a reason for hiding this comment

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

Many tests are failing when running PROVIDERS=phantom,[email protected] pnpm run test:e2e

plugins/index.js Outdated Show resolved Hide resolved
plugins/index.js Outdated Show resolved Hide resolved
plugins/index.js Outdated Show resolved Hide resolved
@@ -1332,13 +1490,14 @@
await metamask.goToAdvancedSettings();
}
}
if ((await playwright.metamaskWindow().locator(toggleOn).count()) === 0) {
await playwright.waitAndClick(toggleOff);
if (!(await playwright.windows(PROVIDER).locator(toggleOn).count()) === 0) {

Check warning

Code scanning / CodeQL

Comparison between inconvertible types Warning

This expression is of type boolean, but it is compared to
an expression
of type number.
@@ -0,0 +1,551 @@
const log = require('debug')('synpress:phantom');

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note

Unused variable log.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const log = require('debug')('synpress:phantom');

support/commands.js Outdated Show resolved Hide resolved
@neuodev
Copy link
Contributor

neuodev commented Jun 15, 2023

@maximgeerinck Tests are failing due to the recent changes (mostly adding new networks), can you please take a look? Many thanks for considering my request.

@neuodev neuodev mentioned this pull request Jun 15, 2023
2 tasks
package.json Outdated
Comment on lines 2 to 3
"name": "@phantom/synpress",
"version": "4.0.0-alpha.22",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"name": "@phantom/synpress",
"version": "4.0.0-alpha.22",
"name": "@synthetixio/synpress",
"version": "3.7.0",

@duckception duckception self-requested a review August 25, 2023 23:38
Copy link
Contributor

@duckception duckception left a comment

Choose a reason for hiding this comment

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

A little bit late, but here's the initial review 🫡

.eslintrc.js Outdated Show resolved Hide resolved
@@ -114,6 +115,9 @@
"ansi-regex": "5.0.1",
"@testing-library/dom": "8.20.0"
},
"overrides": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we override lodash here?

Copy link
Author

Choose a reason for hiding this comment

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

there were some instances were an insecure version of lodash is used in dependencies, this overrides it

package.json Show resolved Hide resolved
commands/metamask.js Outdated Show resolved Hide resolved
return browser.isConnected();
},
clearExtensionData: async provider => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason why this function was introduced?

Copy link
Author

Choose a reason for hiding this comment

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

for solana dapps this is needed as this is the only proper way to stop auto connecting

plugins/index.js Show resolved Hide resolved
);
arguments_.extensions.push(metamaskPath);
if (process.env.PROVIDERS) {
process.env.CYPRESS_PROVIDERS = process.env.PROVIDERS;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we set this here?

Copy link
Author

Choose a reason for hiding this comment

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

cause cypress only recognizes env vars that start with CYPRESS_

plugins/index.js Outdated Show resolved Hide resolved
helpers.js Outdated Show resolved Hide resolved
Comment on lines +238 to +241
activateShowCustomNetworkListInMetamask: async skipSetup => {
const activated = await getProvider(
selectedProvider,
).activateShowCustomNetworkList(skipSetup);
Copy link
Contributor

Choose a reason for hiding this comment

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

What happened here?

Copy link
Author

Choose a reason for hiding this comment

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

nothing much, just using providers now

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean that activateImprovedTokenAllowanceInMetamask function was replaced by activateShowCustomNetworkListInMetamask. Curious what happened here

Copy link
Author

Choose a reason for hiding this comment

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

ok, will put that back

@duckception
Copy link
Contributor

Two crucial things:

  • MetaMask tests are broken. They need to be fixed. Once this is done, I'll approve workflows on CI.
  • Add similar tests for Phantom.

Copy link
Contributor

@duckception duckception left a comment

Choose a reason for hiding this comment

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

Rebase looks good 👍🏻 I think we only need working tests for both of the wallets and we can merge this 🫡

Copy link

vercel bot commented Jan 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
synpress ❌ Failed (Inspect) Jan 26, 2024 3:15pm

rafinskipg and others added 2 commits January 26, 2024 15:57
fix: Fix preferences for default wallet navigation
Copy link

vercel bot commented Mar 1, 2024

@maximgeerinck is attempting to deploy a commit to the Synpress Team on Vercel.

A member of the Team first needs to authorize it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet