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

fix: collisions with other wallet providers #162

Conversation

meandavejustice
Copy link
Collaborator

@meandavejustice meandavejustice commented Feb 17, 2024

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • Refactor (non-breaking change that updates existing functionality)

@meandavejustice
Copy link
Collaborator Author

Getting some peculiar errors in this pr that I'm not seeing on master.

When running pnpm test:
After playwright's webServer is started on port 8081 in packages/snap, getting logs for 404s on http://localhost:8081 and http://localhost:8081/index.html until the timeout is triggered.

No tests are run as it seems to be waiting on the server to start.

I can confirm that the server is running by fetching a file from that directory: curl http://localhost:8081/snap.manifest.json.

@hugomrdias Hoping you can help me out a bit here, I'm not understanding what/where is checking localhost:8081. Is this a playwright thing? This feels like a thing you'll probably know the answer to since you set these tests up.

@meandavejustice
Copy link
Collaborator Author

Digging in a bit more on this webserver failure, after updating packages in hugomrdias/metamask#32 and updating some packages locally to get rid of the peer dependency issue, I'm seeing an actual error message:

j@Davids-MacBook-Air ~/Code/filsnap % pnpm test              

> root@ test /Users/dj/Code/filsnap
> pnpm -r --if-present run test

Scope: 5 of 6 workspace projects
packages/snap test$ playwright test
│ [WebServer] Analyzing[WebServer] 
│ [WebServer] [!] Error: Cannot find module '/Users/dj/Code/filsnap/node_modules/.pnpm/@metamask+key-tree@9.0.0/node_modules/@metamask/key-tree/dist/esm/BIP44Node' imported from /Users/dj/Code/f…
│ file:///Users/dj/Code/filsnap/node_modules/.pnpm/@metamask+key-tree@9.0.0/node_modules/@metamask/key-tree/dist/esm/BIP44Node
│     at finalizeResolution (node:internal/modules/esm/resolve:264:11)
│     at moduleResolve (node:internal/modules/esm/resolve:917:10)
│     at defaultResolve (node:internal/modules/esm/resolve:1130:11)
│     at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:396:12)
│     at ModuleLoader.resolve (node:internal/modules/esm/loader:365:25)
│     at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:240:38)
│     at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:85:39)
│     at link (node:internal/modules/esm/module_job:84:36)
│ [WebServer] 
│ ❌ [build] exited with exit code 1.
│ [WebServer] ❌ 1 script failed.
│ Error: Process from config.webServer was not able to start. Exit code: 1
└─ Failed in 1s at /Users/dj/Code/filsnap/packages/snap
/Users/dj/Code/filsnap/packages/snap:
 ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL  filsnap@1.0.1 test: `playwright test`
Exit status 1
 ELIFECYCLE  Test failed. See above for more details.

There is something bizarre happening when importing this line in snap/src/account.ts

import { getBIP44AddressKeyDeriver } from '@metamask/key-tree'

I've verified locally that these files do exist in node_modules/.pnpm/ {ETC ETC}. 🤔 So not sure what could be causing this issue.

@hugomrdias I did notice that you had run into previous issues with this library's imports as referenced here MetaMask/key-tree#144, but it looks like this issue was resolved in 9.0.0. I know tests have been failing for a while, perhaps this is still an issue in @metamask/key-tree

@hugomrdias
Copy link
Collaborator

Digging in a bit more on this webserver failure, after updating packages in hugomrdias/filsnap-testing#32 and updating some packages locally to get rid of the peer dependency issue, I'm seeing an actual error message:

j@Davids-MacBook-Air ~/Code/filsnap % pnpm test              

> root@ test /Users/dj/Code/filsnap
> pnpm -r --if-present run test

Scope: 5 of 6 workspace projects
packages/snap test$ playwright test
│ [WebServer] Analyzing[WebServer] 
│ [WebServer] [!] Error: Cannot find module '/Users/dj/Code/filsnap/node_modules/.pnpm/@metamask+key-tree@9.0.0/node_modules/@metamask/key-tree/dist/esm/BIP44Node' imported from /Users/dj/Code/f…
│ file:///Users/dj/Code/filsnap/node_modules/.pnpm/@metamask+key-tree@9.0.0/node_modules/@metamask/key-tree/dist/esm/BIP44Node
│     at finalizeResolution (node:internal/modules/esm/resolve:264:11)
│     at moduleResolve (node:internal/modules/esm/resolve:917:10)
│     at defaultResolve (node:internal/modules/esm/resolve:1130:11)
│     at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:396:12)
│     at ModuleLoader.resolve (node:internal/modules/esm/loader:365:25)
│     at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:240:38)
│     at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:85:39)
│     at link (node:internal/modules/esm/module_job:84:36)
│ [WebServer] 
│ ❌ [build] exited with exit code 1.
│ [WebServer] ❌ 1 script failed.
│ Error: Process from config.webServer was not able to start. Exit code: 1
└─ Failed in 1s at /Users/dj/Code/filsnap/packages/snap
/Users/dj/Code/filsnap/packages/snap:
 ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL  filsnap@1.0.1 test: `playwright test`
Exit status 1
 ELIFECYCLE  Test failed. See above for more details.

There is something bizarre happening when importing this line in snap/src/account.ts

import { getBIP44AddressKeyDeriver } from '@metamask/key-tree'

I've verified locally that these files do exist in node_modules/.pnpm/ {ETC ETC}. 🤔 So not sure what could be causing this issue.

@hugomrdias I did notice that you had run into previous issues with this library's imports as referenced here MetaMask/key-tree#144, but it looks like this issue was resolved in 9.0.0. I know tests have been failing for a while, perhaps this is still an issue in @metamask/key-tree

yeah its a metamask problem they have no extensions in the import and also are importing json files without attribute

@hugomrdias
Copy link
Collaborator

its too deep i tried fixing this on our side, but can't find a quick fix

@meandavejustice
Copy link
Collaborator Author

@hugomrdias I see, we are using the newer style imports with ts and esm node.

We should be able to just use the same module rules they are using in our tsconfig

"module": "CommonJS",
"moduleResolution": "node",

and switch away from using esm at the package.json level.

That way we still get to use import statements in all of the actual codebase, but use requires in the rollup.js config, and we shouldnt have any issues with how dependencies are doing their packaging/importing.

@meandavejustice meandavejustice force-pushed the 160-filsnap-adaptor-multiple-wallets branch 2 times, most recently from 33d3803 to cec6eac Compare March 4, 2024 18:58
@meandavejustice
Copy link
Collaborator Author

@hugomrdias Scratch that last comment. These test failures are happening on the master branch, and out of scope of this pr. Those test should be fixed elsewhere. I cleaned up the commit history to reflect the actual changes here.

@hugomrdias
Copy link
Collaborator

@hugomrdias
Copy link
Collaborator

metamask broke its own rollup plugin (kinda it now support proper esm but key-tree is not package right for esm) , and the cli serve cmd also doesnt work anymore it defaults to a full webpack build and serve, and they updated most snap packages to a proper esm packaging but forget key-tree....

@hugomrdias
Copy link
Collaborator

check this PR #163

@meandavejustice
Copy link
Collaborator Author

#163 accomplishes the updates and fixes weirdness around metamask packaging. Will rebase and fix up any remaining issues here once that lands :)

@meandavejustice meandavejustice force-pushed the 160-filsnap-adaptor-multiple-wallets branch from 81055b3 to 11d8cd2 Compare March 4, 2024 21:05
@meandavejustice meandavejustice force-pushed the 160-filsnap-adaptor-multiple-wallets branch from 11d8cd2 to 46cef2f Compare March 4, 2024 21:08
Copy link
Collaborator

@hugomrdias hugomrdias left a comment

Choose a reason for hiding this comment

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

LGTM

@meandavejustice meandavejustice merged commit a166ddb into filecoin-project:master Mar 6, 2024
3 checks passed
@meandavejustice meandavejustice deleted the 160-filsnap-adaptor-multiple-wallets branch March 6, 2024 16:19
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.

[bug]: Filsnap adapter fails to connect when multiple wallet providers are installed
2 participants