Introduction

In our previous two essays, we mainly talked about the vision and the architecture behind the Metamask browser extension. In this essay, we guide you through the code quality and the quality culture that exists in this project. We have already emphasized the importance of enforcing the key quality attributes of this project, so we hope you enjoy reading our analysis.

Overall quality processes

• Bugs and feature requests can be submitted through the community forum which may be converted to GitHub issues by ConsenSys’ employees, or by submitting a GitHub issue directly. Afterwards, these GitHub issues are labeled and triaged, awaiting to be claimed by a developer. Note that MetaMask has a separate process for reporting vulnerabilities to ensure the confidentiality of the vulnerability.
• The MetaMask contributor guidelines only state guidelines for submitting pull requests, like closing the issue your PR fixes. Note that these guidelines do not enforce testing, or specify any coding conventions. Fortunately, the repository does have a linter, which enforces the use of some coding conventions, and forbids committing code with linter errors. We will go into more detail on this linter in the sections below.
• Continuous Integration (CI) also enhances the software quality. We will go into more detail in the sections below.

Key quality attributes

In our previous essays we concluded that the vital quality attributes of the MetaMask extension are security, privacy, and correctness. In the previous section, we noted that MetaMask does not have explicit code quality attributes, because they do not advise or enforce any conventions besides linting.

Security

In the last essay we noted that most security issues are caused by misunderstandings from, often non-technical, users as can be seen in The Chrome Web Store and the MetaMask community forum. We do note that MetaMask is actively working on more preventive security, like blacklisting phishing addresses or warning the user when copy-pasting their passphrase. Unfortunately, other security issues like vulnerabilities are not made public, so we have no way of further measuring the degree of this quality attribute.

Privacy

Since the introduction of Privacy Mode each exposure of personal information causes a “Connect Request”, and users need to explicitly confirm to share their data. The mode is enabled by default, and disabling this mode comes with explicit warnings on the privacy concerns. As a result, privacy is ensured at all times.

Correctness

Besides a secure and privacy-aware application, we obviously need a correctly functioning one as well. As of now, there are roughly 1300 open issues. Fortunately, we can filter on labels indicating the severity of an issue, and find that no urgent issues are open (or have been open), but there are a few high-severity issues. An example of such an issue is that the extension sometimes sends transactions to a cached address instead of the one entered. Given that this directly affects people’s funds, we conclude that correctness has not yet reached a satisfactory degree.

MetaMask Continuous Integration (CI)

MetaMask has implemented CI in various ways, all of them ultimately revolving around the Pull Request made by one or more developers for a certain feature or bugfix. Once a PR is opened, certain teams are automatically requested for review, depending on the files touched. One of the team members can then pick up the review.

Contributor License Agreement signing

The first time a new contributor pushes code to a branch and opens a PR, they must sign the MetaMask Individual Contributor License Agreement before they are allowed to merge their changes into the codebase. The contributor can sign the CLA by commenting on their opened PR. This whole process is handled by a code in a separate open-source repository.

Crowdin

A second step in the CI pipeline is the Crowdin GitHub action. Crowdin is a cloud-based solution that helps open source projects to localize their software projects to different languages. In other words, it is a way of automating the translation of your software project into multiple languages.

Codeql analysis

The last non-test related CI step is the static and semantic analysis of the pushed code for vulnerabilities, using CodeQL. This is done by querying the code as if it were data to target specific types of vulnerabilities, and has a very extensive set of defaults for JavaScript, which is used in the MetaMask extension.

Automated testing

As explained in the previous essay, security, privacy and correctness are key quality attributes of MetaMask. As such, a rigorous test suite is implemented to test every aspect of the extension. These tests range from simple and small unit tests all the way to fully integrated end-to-end tests. For example, MetaMask has simple unit tests for the conversion utils script that handles converting numbers to nicely formatted currencies. On the other hand, the end-to-end tests launch a browser and simulate a user interacting with the extension, to test if the extension behaves as expected. These tests are twofold. First of all, they test whether the implemented functionality has the expected result, including the interaction of the extension with the blockchain. Secondly, it also tests whether the code works in the supported browser, as some functionalities may not always work in all browsers.¹

Unit tests and linting

The unit test suite also automatically includes a linter step. The linter ensures that all the code is up to spec with the required standards, defined in the .eslint configuration files and the included MetaMask eslint-config repository dependency. An example of an eslint configuration rule is the no-array-constructor rule. This rule basically disallows array constructions like new Array(1, 2, 3) and forces the developer to first create an array and then populate it, or to use a primitive array like [1, 2, 3].

The role of test coverage

There are no strict and explicit coverage requirements for submitted pull requests to the repository. There is, however, a strong recommendation to add tests to new programmatic functionality. When an open-source contribution did not add tests, the MetaMask team adds tests on the develop branch before the next release wherever possible. Naturally, this depends on the functionality added and what parts of the codebase are modified. In some cases, when the added functionality touches or changes key functionalities of the project, the pull request is scrutinized heavily and simply not approved if not tested. Due to the fact that tests are still required in functional pull requests, the code coverage is relatively high. An overview of code coverage in the largest folders can be seen in the table below:

Folder	Line Coverage %
shared/	89
shared/constants	98
shared/modules	82
shared/notifications	50
ui/	48
ui/components	48
ui/contexts	36
ui/ducks	58
ui/helpers	66
ui/hooks	82
ui/pages	40
ui/selectors	58
ui/store	41

Hotspot components

To find hotspot components we used CodeScene. Unsurprisingly, the files that change most often are the files with the most lines of code. Most of them are the connector scripts that connect the UI and the controllers, as discussed in the previous essay. It looks like this is a deliberate design choice as all calls are routed through these classes. This inherently makes them hotspot components, as they need to be updated every time functionality is added, removed or a name is changed.

Although the files are changed often, we found the changes came merely down to additions and name changes. It is debatable whether having all these calls go through one large class is good design, but at least it standardizes the communication between the UI and the controllers.

Filename	LoC
ui/store/actions.js	2956
app/scripts/metamask-controller.js	2678
ui/pages/confirm-transaction-base/confirm-transaction-base.component.js	1023

Moreover, the current hotspot components will likely stay junction points in the functionality of the app, at least until a major redesign of the system architecture happens. The maintenance of MetaMask is predominantly “adaptive” to unpredictable external factors, so we can not specifically point out other components that might become hotspots in the future.

Code quality

Although the average lines of code per file is 104, there are 14 files with over a 1000 lines. This looks to be a conscious choice, but does lead to some very complex functions in the aforementioned hotspots. For instance, the cyclomatic complexity of the MetamaskController constructor (in app/scripts/metamask-controller.js) is 35 and has over 800 lines of code, which can evidently be ascribed to the high number of additions to the file. Other long files, like actions.js, suffer from the same issues.

We do note there is a baseline quality of the code because of the strict linter rules.

Quality culture

In order to analyze the quality culture, we found major pull requests that made it all the way to production and had a significant impact on the product. We did this by starting at the ‘releases’ page and looking at the rounded numbers.

An example of an important PR was #11376 which added the ability to buy crypto with FIAT currency straight from Metamask. The author of the PR did a good job, as there was only one comment by a fellow contributor proposing a minor adjustment after which he accepted the PR. Finally, a senior developer from Metamask also accepted the PR and merged it into the develop branch.

In contrast, large UI-related pull requests often get more thoroughly examined, like #13306. Multiple reviewers built and tested the branch, and repeatedly requested changes. Evidently, the reviewers highly value the look of the application.

Technical debt

While analyzing the codebase behind the browser extension, we noticed that it seems like Metamask is growing out of its ‘clothes’ so to speak. As with many Javascript projects, code must continually be refactored in order to suit the size of the codebase.

A good indicator for technical debt is the amount of open bugs on GitHub. For a project that many other dApps rely on, this is a serious concern. This post already describes that large crypto projects were leaving Metamask because of the compatibility issues and bugs that existed in 2019. It is extremely difficult to maintain compatibility with older versions, especially at the rate at which Metamask is growing.

One example of this is the amount of react components that exist within Metamask. These are currently all unordered in a single directory, with no clear distinction between the various types of components. One thing the team did to relieve the pain of having this many components is maintain a solid Storybook, which is a tool to build, test and document your react components.

Another thing we would consider technical debt is the fact that Metamask is using Vanilla Javascript. Typescript is becoming the standard in modern Web development since it improves the code quality and development process, for example by having typing, especially in large projects with many contributors. The larger the codebase becomes, the more work it is to convert everything to Typescript. Since Metamask is already one of the heavier projects, we consider this serious technical debt.

Conclusion

This rounds up our take on the code quality and quality culture in Metamask. Next week we will discuss the scalability issues that the project faces.

References

---

1. Overview of functionalities that work in different browsers, caniuse.com. ↩︎