We would like to insert TypeScript into the ESLint configuration, and
because of the way that the current config is organized, that is not
easy to do.
Most files are assumed to be files that are suited for running in a
browser context. This isn't correct, as we should expect most files to
work in a Node context instead. This is because all browser-based files
will be run through a transpiler that is able to make use of
Node-specific variables anyway.
There are a couple of important ways we can categories files which our
ESLint config should be capable of handling well:
* Is the file a script or a module? In other words, does the file run
procedurally or is the file intended to be brought into an existing
file?
* If the file is a module, does it use the CommonJS syntax (`require()`)
or does it use the ES syntax (`import`/`export`)?
When we introduce TypeScript, this set of questions will become:
* Is the file a script or a module?
* If the file is a module, is it a JavaScript module or a TypeScript
module?
* If the file is a JavaScript module, does it use the CommonJS syntax
(`require()`) or does it use the ES syntax (`import`/`export`)?
To represent these divisions, this commit removes global rules — so now
all of the rules are kept in `overrides` for explicitness — and sets up
rules for CommonJS- and ES-module-compatible files that intentionally do
not overlap with each other. This way TypeScript (which has its own set
of rules independent from JavaScript and therefore shouldn't overlap
with the other rules either) can be easily added later.
Finally, this commit splits up the ESLint config into separate files and
adds documentation to each section. This way sets of rules which are
connected to a particular plugin (`jsdoc`, `@babel`, etc.) can be easily
understood instead of being obscured.
ESLint rules have been added to enforce our JSDoc conventions. These
rules were introduced by updating `@metamask/eslint-config` to v9.
Some of the rules have been disabled because the effort to fix all lint
errors was too high. It might be easiest to enable these rules one
directory at a time, or one rule at a time.
Most of the changes in this PR were a result of running
`yarn lint:fix`. There were a handful of manual changes that seemed
obvious and simple to make. Anything beyond that and the rule was left
disabled.
The ESLint config has been updated to v8. The breaking changes are:
* The Prettier rule `quoteProps` has been changed from `consistent` to
`as-needed`, meaning that if one key requires quoting, only that key is
quoted rather than all keys.
* The ESLint rule `no-shadow` has been made more strict. It now
prevents globals from being shadowed as well.
Most of these changes were applied with `yarn lint:fix`. Only the
shadowing changes required manual fixing (shadowing variable names were
either replaced with destructuring or renamed).
The dependency `globalThis` was added to the list of dynamic
dependencies in the build system, where it should have been already.
This was causing `depcheck` to fail because the new lint rules required
removing the one place where `globalThis` had been erroneously imported
previously.
A rule requiring a newline between multiline blocks and expressions has
been disabled temporarily to make this PR smaller and to avoid
introducing conflicts with other PRs.
# Permission System 2.0
## Background
This PR migrates the extension permission system to [the new `PermissionController`](https://github.com/MetaMask/snaps-skunkworks/tree/main/packages/controllers/src/permissions).
The original permission system, based on [`rpc-cap`](https://github.com/MetaMask/rpc-cap), introduced [`ZCAP-LD`](https://w3c-ccg.github.io/zcap-ld/)-like permissions to our JSON-RPC stack.
We used it to [implement](https://github.com/MetaMask/metamask-extension/pull/7004) what we called "LoginPerSite" in [version 7.7.0](https://github.com/MetaMask/metamask-extension/releases/tag/v7.7.0) of the extension, which enabled the user to choose which accounts, if any, should be exposed to each dapp.
While that was a worthwhile feature in and of itself, we wanted a permission _system_ in order to enable everything we are going to with Snaps.
Unfortunately, the original permission system was difficult to use, and necessitated the creation of the original `PermissionsController` (note the "s"), which was more or less a wrapper for `rpc-cap`.
With this PR, we shake off the yoke of the original permission system, in favor of the modular, self-contained, ergonomic, and more mature permission system 2.0.
Note that [the `PermissionController` readme](https://github.com/MetaMask/snaps-skunkworks/tree/main/packages/controllers/src/permissions/README.md) explains how the new permission system works.
The `PermissionController` and `SubjectMetadataController` are currently shipped via `@metamask/snap-controllers`. This is a temporary state of affairs, and we'll move them to `@metamask/controllers` once they've landed in prod.
## Changes in Detail
First, the changes in this PR are not as big as they seem. Roughly half of the additions in this PR are fixtures in the test for the new migration (number 68), and a significant portion of the remaining ~2500 lines are due to find-and-replace changes in other test fixtures and UI files.
- The extension `PermissionsController` has been deleted, and completely replaced with the new `PermissionController` from [`@metamask/snap-controllers`](https://www.npmjs.com/package/@metamask/snap-controllers).
- The original `PermissionsController` "domain metadata" functionality is now managed by the new `SubjectMetadataController`, also from [`@metamask/snap-controllers`](https://www.npmjs.com/package/@metamask/snap-controllers).
- The permission activity and history log controller has been renamed `PermissionLogController` and has its own top-level state key, but is otherwise functionally equivalent to the existing implementation.
- Migration number 68 has been added to account for the new state changes.
- The tests in `app/scripts/controllers/permissions` have been migrated from `mocha` to `jest`.
Reviewers should focus their attention on the following files:
- `app/scripts/`
- `metamask-controller.js`
- This is where most of the integration work for the new `PermissionController` occurs.
Some functions that were internal to the original controller were moved here.
- `controllers/permissions/`
- `selectors.js`
- These selectors are for `ControllerMessenger` selector subscriptions. The actual subscriptions occur in `metamask-controller.js`. See the `ControllerMessenger` implementation for details.
- `specifications.js`
- The caveat and permission specifications are required by the new `PermissionController`, and are used to specify the `eth_accounts` permission and its JSON-RPC method implementation.
See the `PermissionController` readme for details.
- `migrations/068.js`
- The new state should be cross-referenced with the controllers that manage it.
The accompanying tests should also be thoroughly reviewed.
Some files may appear new but have just moved and/or been renamed:
- `app/scripts/lib/rpc-method-middleware/handlers/request-accounts.js`
- This was previously implemented in `controllers/permissions/permissionsMethodMiddleware.js`.
- `test/mocks/permissions.js`
- A truncated version of `test/mocks/permission-controller.js`.
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
This PR adds one LavaMoat background script policy or each build type. It also renames the build system policy directory from `node` to `build-system` to make its purpose more clear. Each build type has the original `policy-override.json` for `main` builds. The `.prettierignore` file has been updated to match the locations of the new auto-generated policy files.
We need to maintain separate policies for each build type because each type will produce different bundles with different internal and external modules.
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
