-
Notifications
You must be signed in to change notification settings - Fork 5k
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
feat: upgrade assets-controllers to v38.2.0 #27629
Changes from 9 commits
160176d
87953a1
99887e1
4054769
d106c7f
16a47b6
900419c
55bb920
f856efb
14a7051
c32292c
a6a230a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,36 @@ | ||||||
diff --git a/dist/assetsUtil.cjs b/dist/assetsUtil.cjs | ||||||
index e90a1b6767bc8ac54b7a4d580035cf5db6861dca..2662df7869e9079f9379c4dd1ded40b5af4e71f2 100644 | ||||||
--- a/dist/assetsUtil.cjs | ||||||
+++ b/dist/assetsUtil.cjs | ||||||
@@ -3,6 +3,7 @@ var __importDefault = (this && this.__importDefault) || function (mod) { | ||||||
return (mod && mod.__esModule) ? mod : { "default": mod }; | ||||||
}; | ||||||
Object.defineProperty(exports, "__esModule", { value: true }); | ||||||
+function _interopRequireWildcard(obj) { if (obj && obj.__esModule) { return obj; } else { var newObj = {}; if (obj != null) { for (var key in obj) { if (Object.prototype.hasOwnProperty.call(obj, key)) { newObj[key] = obj[key]; } } } newObj.default = obj; return newObj; } } | ||||||
exports.fetchTokenContractExchangeRates = exports.reduceInBatchesSerially = exports.divideIntoBatches = exports.ethersBigNumberToBN = exports.addUrlProtocolPrefix = exports.getFormattedIpfsUrl = exports.getIpfsCIDv1AndPath = exports.removeIpfsProtocolPrefix = exports.isTokenListSupportedForNetwork = exports.isTokenDetectionSupportedForNetwork = exports.SupportedTokenDetectionNetworks = exports.formatIconUrlWithProxy = exports.formatAggregatorNames = exports.hasNewCollectionFields = exports.compareNftMetadata = exports.TOKEN_PRICES_BATCH_SIZE = void 0; | ||||||
const controller_utils_1 = require("@metamask/controller-utils"); | ||||||
const utils_1 = require("@metamask/utils"); | ||||||
@@ -221,7 +222,7 @@ async function getIpfsCIDv1AndPath(ipfsUrl) { | ||||||
const index = url.indexOf('/'); | ||||||
const cid = index !== -1 ? url.substring(0, index) : url; | ||||||
const path = index !== -1 ? url.substring(index) : undefined; | ||||||
- const { CID } = await import("multiformats"); | ||||||
+ const { CID } = await Promise.resolve().then(() => _interopRequireWildcard(require("multiformats"))); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Going back to the change itself: The await on an empty promise before running synchronous require seems unnecessary. Does anything depend on this step being asynchronous? Doesn't seem so.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think in v37 this change was introduced: This patch update matches the generated build after the V37 update; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The @naugtur's suggestion looks good to me, although I'd feel better with another manual check on whether CID is sourced at runtime. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you @MajorLift and @naugtur 🙏 i have removed the empty promise; could you please approve again and we merge this 🙏 |
||||||
// We want to ensure that the CID is v1 (https://docs.ipfs.io/concepts/content-addressing/#identifier-formats) | ||||||
// because most cid v0s appear to be incompatible with IPFS subdomains | ||||||
return { | ||||||
diff --git a/dist/token-prices-service/codefi-v2.mjs b/dist/token-prices-service/codefi-v2.mjs | ||||||
index e7eaad2cfa8b233c4fd42a51f745233a1cc5c387..416dab35e83334f0cafe23c9bd34d0a0380145b2 100644 | ||||||
--- a/dist/token-prices-service/codefi-v2.mjs | ||||||
+++ b/dist/token-prices-service/codefi-v2.mjs | ||||||
@@ -12,8 +12,8 @@ var __classPrivateFieldGet = (this && this.__classPrivateFieldGet) || function ( | ||||||
var _CodefiTokenPricesServiceV2_tokenPricePolicy; | ||||||
import { handleFetch } from "@metamask/controller-utils"; | ||||||
import { hexToNumber } from "@metamask/utils"; | ||||||
-import $cockatiel from "cockatiel"; | ||||||
-const { circuitBreaker, ConsecutiveBreaker, ExponentialBackoff, handleAll, retry, wrap, CircuitState } = $cockatiel; | ||||||
+ | ||||||
+import { circuitBreaker, ConsecutiveBreaker, ExponentialBackoff, handleAll, retry, wrap, CircuitState } from "cockatiel"; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fix makes sense, but I don't understand why we're getting a default import here in the first place. @Mrtenz Is it possible that this is an artifact from This is the original code: import {
circuitBreaker,
ConsecutiveBreaker,
ExponentialBackoff,
handleAll,
type IPolicy,
retry,
wrap,
CircuitState,
} from 'cockatiel'; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not related to type imports, those are supported by
There are several options here:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yet another CJS/ESM interop scenario to keep an eye out for. :( Really appreciate the breakdown and your looking into this. I had a couple of questions:
According to @sahar-fehri, this causes the following error:
Is storybook an outlier here, or will we always see this error if there is no default export?
Also, for this issue, is an upstream update pretty much the only possible fix? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess you could say Webpack is the outlier because it looks at the import $cockatiel from 'cockatiel';
const { circuitBreaker } = $cockatiel;
console.log(circuitBreaker); // [Function: circuitBreaker] Because Node.js loads the CommonJS version, but Webpack loads the ESM version.
I'm leaning towards configuring Webpack to stop using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could apply this as patch temporarily until it's updated, rather than patching There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Really like the idea of patching closer to the root cause. @sahar-fehri To clarify, we'd want to remove this section of the The Webpack issue is really... not great. I also think configuring Webpack would be the preferable solution here. Fixing or patching every upstream package that triggers this issue doesn't seem advisable or feasible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you guys for looking into this 🙏 🙏 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just realised we need the So I think the easiest way forward for now is to keep this patch as it is, and hope that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like a plan! Technically, anything downstream of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using modules across ESM/CJS boundaries differs a lot across tools and environments too. Here's some examples: https://github.com/endojs/endo-e2e-tests/blob/main/matrix/table.md |
||||||
/** | ||||||
* The list of currencies that can be supplied as the `vsCurrency` parameter to | ||||||
* the `/spot-prices` endpoint, in lowercase form. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
await import()
call being left in there was intentional. Not only isawait import()
available to commonjs in node v12+, passing an ESM-only module likemultiformats
intorequire
also crashes node or otherwise may cause undefined behavior.The
import
call causing failed test runs is also expected, because dynamic import is only available behind aNODE_OPTIONS=--experimental-vm-modules
flag. This is why we've added that to all of our core test scripts (https://github.com/MetaMask/core/blob/main/packages/assets-controllers/package.json#L44-L47). We'll probably need to do the same in extension and mobile.I guess the question here is whether we can be sure that
_interopRequireWildcard
is actually executing theCID
import correctly, or if it's masking broken behavior.Another question would be how to handle similar scenarios (upstream ESM-only modules) going forward. Consistently using
await import()
is the only way that makes sense to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MajorLift thnx a lot for looking into this 🙏 I have added some logs with this current patch and it looks like its working fine, its able to fetch CID and display the NFT media;
Without the patch, i can see the error
__import__ is not defined
in console again and its not displaying the images correctly;Thanks for referencing the related test script update on core 🙏 this is not only related to tests, correct? since im able to see the error when running the app locally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The browser uses only ESM, so that's a really weird error message. Maybe it's coming from selenium or some other node process? I tried adding the flag to the e2e test build scripts (#27675) but I'm still getting the same error.
Since you verified that the import is working correctly I think we can move forward with the current patch. Thanks for being on top of that.
Would be nice to figure this error out for the future though. I'll do some more digging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is running under lavamoat and dynamic import is deliberately not supported - hence a transform changing all functions named
import
into__import__
We currently can't have dynamic import work inside compartments. It's not feasible to intercept it the way we'd need to. Since the bundler we use doesn't support it, the dynamic import would be called directly in the browser and "multiformats" is not a URL to a script file, so it would fail with a different error even if LavaMoat transforms weren't there to prevent it.
I recommend addressing this somehow in a minor refactor of the assets controller if it's intended for use in MetaMask.
When we switch to webpack with LavaMoat webpack plugin it's possible webpack will transform the dynamic import into its own syntax if configured for that, but that's the only hope for using dynamic require I see for now. Didn't check how that behaves yet. I can get back to you later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@naugtur Thanks for the insight! I wasn't aware that Lavamoat did this. I'll have to look into what else it blocks.
Unfortunately this a language-level CJS/ESM interop issue that isn't specific to
assets-controllers
andmultiformats
-- it applies to any ESM-only module in our dependency tree.Most of our modules are natively CJS. We use ESM syntax because we write in TypeScript, but we expose dual CJS/ESM builds and our manifests do not specify the
"module": "type"
property. Because of this, the only way for our modules to handle ESM imports in a CJS-compatible manner is with dynamic import syntax i.e.require
doesn't work, and neither does static import syntax that transpiles down torequire
.This issue was previously masked by our usage of the outdated
{ module: "commonjs", moduleResolution: "node10" }
tsconfig options. However, the core and snaps repos recently updated to"node16"
for both options, which enforces sound module resolution restrictions and therefore prohibits ESM-only modules from being passed intorequire
calls. This update is expected to propagate to all of our TypeScript repos.In this specific case, we can use the patch as is because
multiformats
happens to work withrequire
calls without breaking things, but this is a coincidence we can't rely on for all ESM packages in general.To preserve the ban on dynamic import syntax I see three possibilities:
@metamask/superstruct
).All of these would be major, if not prohibitive, undertakings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 and 2 are not options we're fond of.
With the exception of: until we have a lavamoat-node that supports ESM (we're working on it) we might sometimes have to eliminate esm-only packages from builds that run under lavamoat.
We should be able to switch to webpack in the foreseeable future. LavaMoat Webpack plugin is reaching 1.0 by the end of this year and we're wery close to getting a working MetaMask build out of it. 3rdparty audit is finishing too.
Meanwhile,
Joyee has made it possible to
require(esm)
in Node.jshttps://nodejs.org/api/esm.html#interoperability-with-commonjs
https://joyeecheung.github.io/blog/2024/03/18/require-esm-in-node-js/
So we're looking for a solution to the typescript situation that we can live with for limited time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that 1, 2 aren't the most ideal options. ;)
I wasn't aware of
--experimental-require-module
, but it looks like a great stopgap. Fingers crossed that we don't run into an ESM package with top-levelawait
.Based on anecdotal experience from applying
Node16
to core and snaps, our usage of CJS-incompatible packages is uncommon enough that we should be able to handle them on a case-by-case basis, at least for the time being.However, the few exceptions can have an outsized impact. Migrating from
superstruct
to@metamask/superstruct
ended up being a massive undertaking because it required us to update and release basically all transitive dependents. Removing the imports for such a widely used package will probably present difficulties as well. Hopefully we don't come across such a scenario again any time soon.Anyway, it's very good to know that there are fundamental fixes on the way, and thank you for your work on that! Especially excited to potentially have lavamoat-node directly applied to our upstream libraries.