Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
New dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No new dependency issues detected in pull request Bot CommandsTo ignore an alert, reply with a comment starting with Ignoring: Pull request alert summary
📊 Modified Dependency Overview:
|
Currently the extension has explicit ts import files (example - import * 'test.ts'). This is not a good pattern for node, as the ts build will output js files. And thus during the runtime the node process breaks as it cannot find the imported file. On the Extension it does not have any issues because of the bundling that we have in place. It creates a reference to the path and as so it can have ts and js files beig explicitly imported, but in reality it is only running js. The fix that we are doing here, is just to remove the .ts file extension from any import (as the ts file itself is already transpiled into js and has the js file extension)
cf5018a to
6b79e22
Compare
3a6c8d8 to
15785e7
Compare
|
@SocketSecurity ignore chromedriver@111.0.0 fsevents@1.2.13 playwright@1.32.3 |
There's a bug on snaps node execution services that prevents the Math to be used within a snap env. Issue: MetaMask/snaps#1347
This is in line with the node version that electron 23 is using
The NFTController is always fetching the NFT image, even if the image is a data base64 encoded. Fetching anything other than http/https is supported in the browser but not by node-fetch (which uses the underlying http/https node native modules). To fix this we will use the native node fetch (available by default since node 18) to perform any fetch actions that are not http/https.
Update the node browser runtime id whenever we get an internal connection and the sender id does not match the browser id
There's an issue with node 18 where dns will resolve using ipv6 and not fallback to ipv4. This was causing the tests to fail, because localhost resolves to ::1:<port> instead of 127.0.0.1:<port>. This was preventing from running the tests locally
When running the extension e2e tests it is expected that the infura project id is set to 00000000000000000000000000000000. If not we might not be hitting the mockserver
Firefox generates a different runtime id for the extension. In order to get the actual runtime id, we can parse the url from an internal connection created by either the popup, notification or fullscreen extension
This method is not async in the browser. Due to that on the Extension we are calling it synchronously to go back to the home page (from the phishing blocked script). When on Desktop we proxy this requests to the actual browser, meaning that the request will be async. This was causing the phishing detection to not return to the extension home page when clicking on the back to safety button. Also, it is worth mentioning that the protocol for chrome and firefox extensions are different. So added a method to update the browser info accordingly and route to the correct url.
snaps 0.32.2 updated the bundled from webpack to browserify, so we need to reintroduce this patch (now adpated to browserify)
trezor connect and isomorphic fetch were overriding global.fetch. Open a topic with lavamoat team to clarify why setting the policy to write would not identify that the global.fetch was previously set.
Mock connectRemote env type constants
|
@SocketSecurity ignore @metamask/eth-json-rpc-provider@1.0.0 @metamask/message-manager@2.1.0 @metamask/swappable-obj-proxy@2.1.0 react-syntax-highlighter@15.5.0 fault@1.0.4 format@0.2.2 |
This test is skiped in the extension. There is a bung in the snaps controller.
|
@SocketSecurity ignore @metamask/eth-json-rpc-middleware@11.0.0 |
| "dotenv-cli": "^6.0.0", | ||
| "duplexify": "^4.1.1", | ||
| "electron": "^23.0.0", | ||
| "electron": "^23.2.2", |
| Icon, | ||
| ICON_NAMES, | ||
| } from '../../../../submodules/extension/ui/components/component-library'; | ||
| } from '../../../../submodules/extension/ui/components/component-library/icon/deprecated'; |
There was a problem hiding this comment.
| } from '../../../../submodules/extension/ui/components/component-library/icon/deprecated'; | |
| } from '../../../../submodules/extension/ui/components/component-library/icon'; |
There was a problem hiding this comment.
There are breaking changes between the icons/deprecated and the new icons... I decided to keep the deprecated one, otherwise I would need to do more changes to the Desktop UI components. We can do that on subsequent PR
vinistevam
left a comment
There was a problem hiding this comment.
Amazing PR @cryptotavares, so many challenges overcome like the ip6, congrats!
Just a minor suggestion that might be worth double-checking.
|
|
||
| if (['https:', 'http:'].includes(requestURL.protocol)) { | ||
| // Use node-fetch which supports proxying with global-agent rather than built-in fetch provided by Node 18 | ||
| return nodeFetch(url, options); |
Overview
Update submodule to latest Extension master. This is a preparatory PR for a silent release.
Changes
Root
Pipeline
Common
None
App
writedoes not includeread.browser.runtime.getURLto synchronously create the URL manually for if the path that we are getting is home.html (also needed to figure out if the Extension connecting is running on chrome or firefox). In Desktop, due to proxying the request to the browser,browser.runtime.getURLis an async method. This was preventing going back from the Phishing blocker page back into the Extension home page.