• test the port from metamask-extension code by wrapping the original controller

Many controllers accept a provider configuration property and internally create an EthQuery instance. We should make the NetworkControllers internal EthQuery instance public, and pass that into each controller instead of the provider.

This may be too restrictive, but it should be investigated.

There is a condition in the PreferencesController that checks if the RPC URL is
http://localhost:8545, presumably to check if the node is a local testing node. However, a user may
conceivably run their node on a different port, which would fail this check.

stripHexPrefix isn't a documented function from ethereumjs-util v5.2.0,^[1] but there are a few usages of it in src/utils.ts:

https://github.com/MetaMask/gaba/blob/7237cc605c0432f3a2298ec64a790bb3339aed59/src/util.ts#L70-L79

https://github.com/MetaMask/gaba/blob/7237cc605c0432f3a2298ec64a790bb3339aed59/src/util.ts#L81-L97

https://github.com/MetaMask/gaba/blob/7237cc605c0432f3a2298ec64a790bb3339aed59/src/util.ts#L172-L190

This code works in 5.x and in 6.x accidentally because all of ethjs-util is assigned to exports but we should either submit a patch upstream or rely on ethjs-util directly.^[2][3]

Address book entries are defined as:

interface AddressBookEntry {
  address: string;
  name: string;
  chainId: number;
  memo: string;
}

This leads us to believe that, perhaps, the user should be able to have entries with identical address fields and different chainId fields.

However, we store AddressBookEntrys by normalized addresses, and we normalize the address using toChecksumAddress(address), which returns the same address regardless of the chainId.

According to the ethereumjs-util documentation, we can add the chainId as an optional parameter to generate unique checksummed addresses by chainId. It appears they have yet to publish this version of the library, however, see: ethereumjs/ethereumjs-util#218

Question: if we compute checksummed addresses with chainId, can we still use those addresses directly for sending transactions? In other words, will they be different from what's currently displayed in the UI?

Currently the method BaseController.putState(state: object, forceUpdate: bool) is synchronous, and merely emits a state update, leaving persistence as an external concern, not coupled with controller logic.

I am working on a feature that will require guaranteed persistence to be coupled with some types of state updates, so I would like a way to update state that returns a promise and resolves once state has been persisted, so that it will be restored in the case of a system interruption.

Example use case

Let's say we add a login permission that enforces a rule like "This site may update my profile 5 times."

In order to enforce this rule, we need to ensure that portions of the state, namely:

• update counter
• outbound profile update messages

Are always persisted atomically, so that we never broadcast a profile update without also accounting for the site's limit.

This is about enforcing smart-contract-like guarantees within our own controllers, and if we want to decouple our security from constant user nagging, I believe this is a critical feature to pursue.

This will probably require gaba to become somewhat aware of its persistence strategy.

The existing Jest config only outputs the default text report. The HTML report is useful during development—we should output that as well.

We should look to remove require statements and use imports for any NPM modules with available typings.

There's something funky going on with the state of the KeyringController. Specifically with the property keyrings.

Once the property is updated via fullUpdate which internally calls update, the new value is not accessible via state (still has the default value) but it's fine if you access it via context.

How to reproduce from the app:
1 - Log in
2 - To verify that the value has been set, log Engine.context.KeyringController.state.keyrings => [{ type: 'HD Keyring', accounts: ['0x...']}]
3 - Now try to access it through state, logEngine.state.KeyringController.keyrings => []

Would be nice if handle returns some object which represents running request with a method abort which aborts the request, like XMLHttpRequest.abort()

Test coverage is incorrectly reported at 100% due to the lack of importing and testing of the
KeyringController. The “heavy lifting” of this controller is handled in another package, but it’s still
important to test this controller to ensure proper usage.

See https://help.github.com/en/github/managing-security-vulnerabilities/adding-a-security-policy-to-your-repository

The current test suite doesn't mock any endpoints during unit testing. Instead, they either hit real endpoints or skip request entirely. We should be setting nock.disableNetConnect(); in the unit test setup to ensure that none of our tests will send real network requests, but first we'll have to migrate each test that relies upon the network to use mocks.

EDITED TO ADD:

Here are a list of controllers that have tests in which we need to mock network requests:

• #770
• #769
• #771
• #772
• #773

Gaba often makes use of a utility function called safelyExecute. The purpose of this function is to call a supplied function without handling any exceptions or errors that may occur.

The safelyExecute function accepts a function that returns a Promise and then calls it inside of a try...catch block. The catch block does not handle exceptions, provide a way for the caller to handle them, or even log the error. Because of this failures and critical issues can go unnoticed.

We should port the existing balance controller to GABA.

GABA currently uses the Balanc3 pricing API for token rates. Unfortunately, this API is being shut down soon. We should transition the TokenRatesController to the CoinGecko pricing API (assuming its rate limit is per IP.)

We should port the existing network controller to GABA.

As of fcdf82f, npm ci && npm t fails with a lot of test files producing the following error:

> [email protected] test
> jest --coverage

[...]

    Cannot find module 'bignumber.js' from 'utils.js'
      
      at Resolver.resolveModule (node_modules/jest-resolve/build/index.js:169:17)
      at Object.<anonymous> (node_modules/web3/lib/utils/utils.js:37:17)

https://github.com/MetaMask/gaba/blob/a30160a68e304559c551d9477d5683df5f61f557/src/TransactionController.ts#L353

https://npmfs.com/package/gaba/1.6.0/index.js.map references "../src/index.ts" which is not included in the package distribution. Either sources should be inlined or the source should be included in the package

We should verify what happens to controllers when connectivity goes down and make sure they can resume peacefully. Is this something we handle higher up int he stack, like at the app level?

The GABA engine is a collection of platform-agnostic modules for creating secure data models for cryptocurrency wallets.

What does "GABA" mean?

Compilation with the latest 3.9 version results in the following errors:

src/user/PreferencesController.ts:186:5 - error TS2783: 'address' is specified more than once, so this usage will be overwritten.

186  			address,
     			~~~~~~~

  src/user/PreferencesController.ts:188:5
    188  			...(oldIdentities[address] || {})
         			~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    This spread always overwrites this property.

src/user/PreferencesController.ts:187:5 - error TS2783: 'name' is specified more than once, so this usage will be overwritten.

187  			name: `Account ${index + 1}`,
     			~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  src/user/PreferencesController.ts:188:5
    188  			...(oldIdentities[address] || {})
         			~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    This spread always overwrites this property.


Found 2 errors.

The https://api.infura.io/v2/blacklist endpoint is just a proxy of https://raw.githubusercontent.com/409H/EtherAddressLookup/master/blacklists/domains.json and https://raw.githubusercontent.com/MetaMask/eth-phishing-detect/master/src/config.json. We should request these files from GitHub directly to reduce our Infura traffic.

We should port the existing transaction controller to GABA.

By having an interface EthereumAddress we can forget about all the type related issues (string vs hex, etc).

If we have an EthereumAddress obj also we could call toChecksumAddress inside of the constructor and have some utility function like toShortString (which could return 0x1234...1234), etc.

cc: @danfinlay

We should update the current KeyringController implementation to be a true GABA controller instead of wrapping metamask/KeyringController internally. Note that this also involves testing the controller fully.

npm install gaba fails to install on an empty nodejs project with

npm ERR! code 128
npm ERR! Command failed: /usr/bin/git checkout e307359ca9ea6be4ded6ac58ac7e16f192ae4e2a
npm ERR! fatal: reference is not a tree: e307359ca9ea6be4ded6ac58ac7e16f192ae4e2a

the following dependency can not be resolved
https://github.com/MetaMask/gaba/blob/b0cbc49b5ccf311207a7e79fd57b420f722c0fc0/package.json#L76
it references an nonexistent commit in the respective repository

Gaba contains a number of classes that initiate a polling mechanism for querying remote services for
various information. Due to the way in which these timers are written, certain network conditions could
lead to “stacking” connections, thus causing an exhaustion of sockets/file descriptors.

A requested permission should permit an optional justification field that is passed to the user confirmation function, to enhance the context of the prompt.

Should support text, but also be forward-extensible to other justification formats.

Because all the data in state is not relevant anymore

This for loop (https://github.com/MetaMask/gaba/blob/master/src/KeyringController.ts#L302) is assuming accounts and testAccounts are objects, but in reality it's an array,.

See example:

This project should be using CircleCI, for consistency with the rest of the organization.

Hi, @estebanmino!
Will be a lot of thanks for the advice, how can be received a list of tokens by Web3 from AssetsState?

https://github.com/MetaMask/controllers/blob/82c29fac98e3073d15f93d59d6da8f2f616115a7/src/AssetsController.ts#L64

So the state can only expose the list of transactions for the selected address and current network

The state member variable exposed by the BaseController is mutable and can be modified externally. Direct state modification shouldn't be allowed since it bypasses notification. The state variable should be made readonly so that it's immutable and can only be modified by calling update or an exposed API method that modifies state internally.

We should port the existing computed balance controller to GABA.

The current state of develop will be 2.0.0:

• Remove web3 (#108)
• Reuse collectible contract info (#165, optional)
• Rename & scope package (#226)

The AddressBookController in Gaba does not perform any validation on the input and allows invalid
addresses.

for removing token management methods from extension preferences controller MetaMask/metamask-extension#6258

We should add a require parameter to permissions requests to allow domains to require specific permissions. These permissions cannot be deselected in the UI, and the entire request will fail if any require === true permissions are rejected.

This project could use the shared ESLint config

Allows lazier consumption of the resulting API: permissions could be requested as-needed.

Maybe added as an option?

As discussed in MetaMask/metamask-extension#6110

better if we use https://github.com/ethjs/ethjs-contract and https://github.com/ethjs/ethjs-query

The validateTransaction utility function in Gaba is not sufficient in preventing hostile input.

The value is converted to a string and then checked for “-” and “.” to determine if it is a negative or floating point number respectively. There are no other checks.

The current incarnation of this function allows the numerical values Infinity and NaN as well as unsafe
numbers like 10000000000000000 and even strings like “one million dollar$”. This can
potentially lead to a class of bugs like 9007199254740992 === 9007199254740993; // true

Instead of type casting and checking for substrings, the best approach here is to check
Number.isFinite(), !Number.isNaN(), and Number.isSafeInteger(). This will provide the
validation needed and perform better.

The notify method is exposed publicly on the BaseController, but it should be protected. It's an anti-pattern for notification to be triggered externally since it should already be triggered on every supported state change.

We should also triage all member visibility on the BaseController.

After trying to debug why transactions are not showing up as confirmed on the mobile app (See MetaMask/website#161) I found out the BlockHistoryController is holding the same last block 40 times instead of the last 40 blocks.

Looks like this no longer gets updated via CI, gh-pages hasn't been updated in 6 months.

(I imagine this is because of a token issue.)