This is a continuation of #9726, which did not fix the problem
described.
If the initial network when the extension is started is something other
than Mainnet, the swaps controller will never successfully retrieve
swap quotes. This is because `ethers` will continue to communicate
with whichever network the provider was initially on.
We tried fixing this by hard-coding the `chainId` to Mainnet's
`chainId` when constructing the Ethers provider, but this did not work.
I suspect this failed because the `provider` we pass to `ethers` is not
compliant with EIP 1193, as `ethers` doubtless expects it to be.
Instead the entire `ethers` provider is now reconstructed each time the
network changes. This mirrors the approach we take in some other
controllers.
If the initial network when the extension is started is something other
than Mainnet, the swaps controller will never successfully retrieve
swap quotes. This is because the `ethers` provider used by the swaps
controller doesn't allow network changes by default - it assumes that
the network remains the same as when the provider was initialized.
This was fixed by hard-coding Mainnet as the initial chain ID for this
`ethers` provider used by the swaps controller.
Some adjustments needed to be made to the `provider` stub to allow
setting `1` as the network ID and chain ID in unit tests.
Refs #9663
See [`node/no-deprecated-api`][1] for more information.
This change enables `node/no-deprecated-api` and fixes the issues raised by the rule.
[1]:https://github.com/mysticatea/eslint-plugin-node/blob/v11.1.0/docs/rules/no-deprecated-api.md
The change to the way that `punycode` is imported is to address the fact that
third-party module is hidden by the built-in. This is a silly hack but it works.
Our ENS resolver for the browser address bar was incorrectly resolving
addresses that included query strings. We were concatenating the `path`
property with the `search` property, despite the fact that the `path`
property already contains `search`. As a result, `search` was
duplicated in the resolved addresses.
For example, if an IPFS content ID was found for this address, the
resolved address for `metamask.eth/?foo=bar` would have the path
`/?foo=bar?foo=bar`
The original intent was likely to use `pathname` in place of `path`.
The resolver has been updated to use `pathname`, and the query string
now appears only once in the resolved address.
Consolidates the background and UI segment implementations into a shared solution.
This results in the introduction of our first shared module.
Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>
`@metamask/eslint-config` has been updated to v4.1.0. This update
requires that we update `eslint` to v7 as well, which in turn requires
updating most `eslint`-related packages.
Most notably, `babel-eslint` was replaced with `@babel/eslint-parser`,
and `babel-eslint-plugin` was replaced by `@babel/eslint-plugin`. This
required renaming all the `babel/*` rules to `@babel/*`.
Most new or updated rules that resulted in lint errors have been
temporarily disabled. They will be fixed and re-enabled in subsequent
PRs.
* Calculate savings per swap relative to median values
* Update test mock quotes, add getMedian tests
* Identify assets by sourceToken and destinationToken
The account tracker had one doc comment above the constructor that
partially served to document the constructor, but mostly contained a
type definition for the class itself. It has been split into two
blocks; one for the class, one for the constructor. The constructor doc
comment has also been expanded to document all constructor options.
The `chainId` is now used by the account tracker to identify the
current network, instead of the `networkId`. This should have no
functional impact, aside from that different chains with the same
`networkId` will now be correctly distinguished from each other.
An attempt to safely release the `nonceLock` upon failure has instead
made failure worse by masking it with a new error. If the call to get
the `nonceLock` throws an exception, then the `finally` block here
would attempt to call `releaseLock` on the `nonceLock` variable, which
is guaranteed to be `undefined` if the previous call failed. The
attempt to call a method on `undefined` throws another error, masking
the original error.
It is safer to obtain the `nonceLock` and release it without using any
`try` or `finally` block. The `nonceLock` is synchronously released
immediately after it is obtained, and any errors bubble up correctly
without being masked. There is no case where the lock is left
unreleased.
If the `signTypedData` background function threw an exception, it would
return `undefined` to the UI, which would throw another exception in
the UI. It now re-throws the error if an error is thrown, which
allows the UI to handle the error.
I'm not sure why this might fail, and I'm not sure we're handling this
failure well, but this is an improvement at least.
* Add data point to 'Swaps Completed' segment event: estimated vs used gas
* Linted
* Correct property name for estimated gas on swapMetaData in _trackSwapsMetrics()
* Set estimated_gas property on swapMetaData to a hex string
* Correct base when dividing by estimated_gas
Co-authored-by: Dan Finlay <dan@danfinlay.com>
* Delete CachedBalancesController.cachedBalances
* Migrate provider to Rinkeby instead of deleting it
* Convert hex transaction metamaskNetworkId values to decimal
* Don't migrate provider state in e2e tests
* Don't kick custom RPC users to Rinkeby unnecessarily
* Use provider.chainId for address book chainId values
* Add address book migration
* Fix failing unit test
* fixup! Merge branch 'develop' into address-book-use-chainId
* Select address book entries for display by chainId
* Merge all address book entry keys
* fixup! Merge all address book entry keys
* Delete localhost provider type
* Use ganache-cli default chain ID for tests
* Delete unused test firstTimeState variable
* Migrate default ganache-cli network to frequentRpcListDetail
* Add default test provider state
* Add test functionality to createJsonRpcClient
* Lint locales
* Update test middleware creation
* fixup! Update test middleware creation
* Update txMeta after postTxBalance has been retrieved
* Use gas used from txReceipt to calculate eth received
* Return null from getSwapsTokensReceivedFromTxMeta in tokenSymbol is ETH and txReceipt is missing
* Get latest txMeta before updating it with postTxBalance in case of a swaps tx in confirmTransaction
* Lint fix
* Use initial transaction for settings swap transaction title params, and remove addition of swap properties to cancel transcations
* Update unit test data
* Use token symbol properties from initial transaction for filitering in transaction list
* call this.txStateManager.setTxStatusConfirmed before async call in confirmTransaction in the transactions controller
* Clone txMeta before setTxStatusConfirmed in confirmTransaction
* Correctly updateTx in confirmTransaction
* Track swaps event only if it is a swap transaction
* Create wrapper function for segment events
* Extract transaction controller metrics calls into own function
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
* Stop passing a gas param to the estimateGas call initiated in the swaps controller timedoutGasReturn
* Stop passing gas params to timedoutGasReturn
* Lint fix
* Stop passing no longer used param to setInitialGasEstimate
When the `chainId` for a custom RPC endpoint is edited, we now migrate
the corresponding address book entries to ensure they are not orphaned.
The address book entries are grouped by the `metamask.network` state,
which unfortunately was sometimes the `chainId`, and sometimes the
`networkId`. It was always the `networkId` for built-in Infura
networks, but for custom RPC endpoints it would be set to the user-set
`chainId` field, with a fallback to the `networkId` of the network.
A recent change will force users to enter valid `chainId`s on all
custom networks, which will be normalized to be hex-prefixed. As a
result, address book contacts will now be keyed by a different string.
The contact entries are now migrated when this edit takes place.
There are some edge cases where two separate entries share the same set
of contacts. For example, if two entries have the same `chainId`, or if
they had the same `networkId` and had no `chainId` set. When the
`chainId` is edited in such cases, the contacts are duplicated on both
networks. This is the best we can do, as we don't have any way to know
which network the contacts _should_ be on.
The `typed-message-manager` unit tests have also been updated as part
of this commit because the addition of `sinon.restore()` to the
preferences controller tests ended up clearing a test object in-between
individual tests in that file. The test object is now re-constructed
before each individual test.
The `_fetchAndSetSwapsLiveness` was accidentally passed to
`setInterval` without being bound first, so the `this` reference was
not defined when it was called. It is now bound before being passed to
`setInterval`.
* Remove network config store
* Remove inline networks variable in network controller
* Re-key network controller 'rpcTarget' to 'rpcUrl'
* Require chainId in lookupNetwork, implement eth_chainId
* Require chain ID in network form
* Add alert, migrations, and tests
* Add chainId validation to addToFrequentRpcList
* Update public config state selector to match new network controller
state
* Use network enums in networks-tab.constants
* Ensure chainId in provider config is current
* Update tests
Right now when editing an address in "Settings > Contact", the contact
is lost after saving. This is because the code awaits
`removeFromAddressBook()` before creating the new contact but
`removeFromAddressBook()` never resolves. This change fixes this bug.
The web3 usage metrics added in #9144 assumed that all web3 properties
were strings. When a `Symbol` property is accessed, our `inpage.js`
script crashes because the `Symbol` cannot be serialized correctly.
A check has been added for non-string property access. The metric event
in these cases is set to the string "typeof ", followed by the type of
the key. (e.g. `typeof symbol` for a `Symbol` property).
Fixes#9234
The usage metrics for the injected web3 instance were being sent upon
each use, which exceeded the limits of our Matomo plan. These metrics
are now only being sent upon the first usage, for each origin and
property.
* Fix require-unicode-regexp issues
See [`require-unicode-regexp`](https://eslint.org/docs/rules/require-unicode-regexp) for more information.
This change enables `require-unicode-regexp` and fixes the issues raised by the rule.
* Remove case-insensitive flag from regexps
The "Background" metrics category was being set in the
`backgroundMetaMetricsEvent` function. This function might not
necessarily include any event at all though, so setting it here seemed
inappropriate. It would also crash if `eventData.eventOpts` was not
set, which is not great since that property is optional.
The background category is now set in the `sendBackgroundMetaMetrics`
function in `metamask-controller`. This method is used solely for event
data, so it would make sense for this category to be always set.
There is no functional difference, since `backgroundMetaMetricsEvent`
is called solely by `sendBackgroundMetaMetrics`.
The `currentPath` parameter passed to our metrics utility had been
passed the full URL rather than just the path, contrary to what the
name would imply. We only used the path portion, so passing the full
URL did lead to complications.
Now just the `pathname` is passed in, rather than the full URL. This
simplifies the metrics logic, and it incidentally fixes two bugs.
The main bug fixed is regarding Firefox metrics. Previously we had
assumed the `currentPath` would start with `chrome-extension://`, which
of course was not true on Firefox. This lead to us incorrectly parsing
the `currentPath`, so path tracking was broken for Firefox events.
This broken parsing is now bypassed entirely, so metrics should now
work the same on Firefox as on Chrome.
The second bug was that we were incorrectly setting the tracking URL
for background events during tests. As a result, we were incorrectly
detecting ourselves as an internal site that had referred the user to
us. But this was not of major concern, since it only affected test
metrics (which get sent to the development Matomo project).
Lastly, this change let us discard the `pathname` parameter used in
the `overrides` parameter of the `metricsEvent` function. Now that
`currentPath` is equivalent to `pathname`, the `pathname` parameter is
redundant.
* Remove `url` parameter from `metricsEvent`
The `url` parameter was used to override the `currentPath`, but it
never worked correctly. It was supposed to be used for setting the
`url` query parameter that was sent to Matomo, but `currentPath` was
always used even if it `url` was set and `currentPath` was empty.
Instead, `currentPath` is now always used. There was never a need to
provide an "override" for `currentPath` when it can be set directly.
The metrics provider does set `currentPath` automatically by default,
but this can be overwritten already by passing a second parameter to
`metricsEvent`.
There were two places this `url` parameter was being used: background
events, and path changes. Background events were submitted with no
`currentPath`, so because of the bug with the `url` parameter, the
metrics utility would crash upon each event. So those were never
actually sent. This commit will fix that crash.
The `currentPath` parameter was supplied as an empty string for the
path change events, so those never crashed. They just had the `url`
query string parameter set incorrectly (to an empty string). It should
now be correctly populated, which should mean we'll be capturing all
path changes now. Previously we were only capturing path changes to
pages that happened to include an event, because of this blank `url`
problem.
* Use `url` query parameter as fallback for generating `pv_id`
The `pv_id` parameter currently isn't generated correctly on Firefox,
as the generation assumes that the current URL starts with
`chrome-extension://`. The `url` query parameter is still unique for
each path, so it's probably good enough for generating an id for each
page.
This is just a temporary fix; it will be removed in a future PR, where
Firefox will be properly supported.
Background events are now sent in the `Background` category, rather
than `backend`. Conventionally we use the term "background" over
"backend", as it's not really a "backend" in the normal sense since
it's a client background process. Also it's capitalized because all of
the other event categories are capitalized as well.
The metrics URL has been updated to use `background` instead of
`backend` as well, for consistency.
Luckily we don't have to worry about our metrics being disjointed due
to this name change, because the background metrics never worked to
begin with! So there will be none under the old name. The metrics will
be made functional in a separate PR.
The Sentry DSN is now expected to be provided via environment variable
for production builds. The build script will fail if it is missing, and
an error will be thrown at runtime if it is missing.
The `SENTRY_DSN` environment variable has been set in CI to the old
value for `SENTRY_PROD_DSN`. We can migrate to a new DSN at some point
in the future.
In a non-production environment, Sentry was configured to send error
reports to a "test" MetaMask project. It will still do this during e2e
tests, but in development Sentry is now disabled completely.
In practice this was never useful in development.
* Fix popup/notification when browser is in fullscreen, primarily on OSX.
The issue was reported internally via Slack. User was running Mac OSX Chrome in fullscreen mode where Chrome is created in a new Desktop workspace.
The issue reproduced on OSX Chrome in fullscreen/maximized view overrides the explicitly set width and height for `windows.create()`. Possibly not overrides, but creates a window based off of the window that it was created from. Found a related [Chromium bug](https://bugs.chromium.org/p/chromium/issues/detail?id=263092&q=window%20create%20width%20os%3DMac&can=2).
The fullscreen `popup.left` pixel will calculate the window position incorrectly since we set and assume the width of the created window. The incorrect `left` position the window and transition the focus Desktop/Workspace incorrectly and make is seem to lose focus of the new window/workspace. Incidentally this will make the popup full width/height, and create a new workspace for the view, which we have no control over until Chrome
fixes it.
This will check if the popup is 'fullscreen', which it gets passed from the origin window, if so then don't reposition the window. If Chrome fixes the issue we can revert this change.
* Feedback commit
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
The `extra` property of errors sent to Sentry is sometimes not
initialized when we add the application state. A check has been added
to initialize it if it's missing.
I suspect that this changed with v5 of `@sentry/browser`, though I
can't find any explicit confirmation of this in their changelog.
The state snapshot that was attached to Sentry errors was removed
recently in #8794 because it had become too large. The snapshot has
now been restored and reduced in size.
A utility function has been written to reduce the state object to just
the requested properties. This seemed safer than filtering out state
that is known to be large or to contain identifiable information.
This is not a great solution, as now knowledge about the state shape
resides in this large constant, but it will suffice for now. I am
hopeful that we can decorate our controllers with this metadata in the
future instead, as part of the upcoming background controller refactor.
A separate `getSentryState` global function has been added to get the
reduced state, so that the old `getCleanAppState` function that we used
to use could remain unchanged. It's still useful to get that full state
copy while debugging, and in e2e tests.
This migration had referred to the non-existent
`TransactionsController` instead of `TransactionController`, so it
effectively did nothing. Now it should work.
This migration hasn't been included in any release yet, so we can fix
it in-place instead of adding an additional corrected migration.
The migration comment has also been updated, as it was inaccurate.
This method was accidentally broken with the introduction of the
permissions controller, as this was missing from the list of safe
methods.
It is now included in the list of safe methods.
Fixes#8993
The currency rate controller is updated upon each network change, as
the "native currency" is network-dependent and might have changed.
However, any thrown errors were being caught and passed to an empty
callback.
The errors are now re-thrown in the callback. As a result, the errors
will now be printed to the console and sent to Sentry.
It seems that this blocklist checker never worked correctly. Ever since
the initial commit, it was comparing the Number `1` to the `networkId`,
which is a string. Additionally, even if it did throw, the transaction
continued unhindered. The user could still approve it, and there was no
indication shown to the user that anything went wrong. Also some of the
blocklist entries were incorrectly mixed-case, and were never hit.
We can remove this for now, and re-add it later on after we rewrite the
transaction controller.
The `metamaskNetworkId` property in the `txMeta` for incoming
transactions was incorrectly set as a Number instead of a String. This
change was made accidentally as part of #8627.
As a result incoming transactions were being excluded from the
transaction list, as they didn't have a matching network ID.
`metamaskNetworkId` is now set to a string, and a migration has been
added to ensure `metamaskNetworkId` is converted to a string for any
incoming transactions in state.
An optimization in `account-tracker.js` was being skipped consistently
due to a type error (a number was being compared to a string).
The optimization in this case was to update the balances for all
accounts with a single request, rather than one request per account.
The `activeTab.id` property is relied upon in the connected sites modal
to prevent the user from manually connecting to the MetaMask extension
itself. Unfortunately the `id` property was never set.
`id` is now set on the `activeTab` state, so manually connecting to the
extension UI is now impossible.
The `activeTab` state is now set to an empty object if the `origin` of
the active tab is missing or invalid. It can be invalid if the URL
passed to the `URL` constructor is missing a scheme (e.g.
`about: blank`).
There are currently no cases where the rest of the data in `activeTab`
is useful in the absence of an `origin`. This will make upcoming UI
logic changes a bit simpler than they would be otherwise. Now we can
assume that if any property is set on `activeTab`, it must have a valid
`origin`.
There were three cases where execution unintentionally continued after
an error was encountered. These cases likely are impossible to
encounter in practice due to recent validation improvements in the
`eth-json-rpc-middleware/wallet` module, but they were broken
nonetheless.
Execution inside the Promise constructor now halts immediately after
`reject` is called.
* Use over the whole stringified error object which doesn't show the actual error message that is set as the
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
* Feedback commit
The code for checking whether a transaction was dropped or not was
refactored in #8398, but in the process an off-by-one error was
introduced.
The old version of `_checkIfTxWasDropped` would query for an updated
transaction count from the network, and would consider the pending
transaction to be dropped if the count was above the nonce. However,
the version introduced in #8398 considers the transaction to be dropped
if the count is above *or equal to* the nonce.
The pending transaction nonce is expected to be equal to the
transaction count, because the nonce starts at zero. The transaction
count is equal to the expected next nonce.
The variable name has been updated to make this more clear
(`networkNextNonce` is how the `nonce-tracker` refers to this value).
`parseInt` is now called with an explicit radix of `16` as well, to
ensure both nonce strings are always parsed as hex. In all cases I am
aware of, these nonce strings were prefixed by `0x`, meaning that
`parseInt` would default to a radix of `16`, so this likely doesn't
constitute a functional change.
Fixes#8688
* update connected accounts appearance
* consolidate account alerts
* UnconnectedAccountAlert: use ConnectedAccountsList
* move switch account action out of menu in all views
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Imported accounts can be removed, but the permissions controller is not
informed when this happens. Permissions are now removed as part of the
account removal process.
Additionally, the `getPermittedIdentitiesForCurrentTab` selector now
filters out any non-existent accounts, in case a render occurs in the
middle of an account removal.
This was resulting in a render crash upon opening the popup on a site
that was connected to the removed account.
The state snapshot we were attaching to Sentry errors was too large.
As a temporary solution, it has been removed completely. We can re-add
it later after reducing its size.
* restore and enhance the time est feature
background: we had a feature for showing a time estimate on pending txs
that was accidently removed during the redesign implementation. This PR
restores that feature and also enhances it:
1. Displays the time estimate on all views instead of just fullscreen
2. Uses Intl.RelativeTimeFormat to format the time
3. Adds a way to toggle the feature flag.
4. Uses a hook to calculate the time remaining instead of a component
* Update app/_locales/en/messages.json
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
* do not display on test nets
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
The `TokenRatesController` was accidentally broken in #8744, when the
logic for starting and stopping polling was moved from the `isActive`
property to start/stop functions.
A reference to the now-obsolete `isActive` property was accidentally
left behind, resulting in no exchange rate updates.
Each "message" requiring a user confirmation has a unique `type`
property. These `type` properties have all been added as enums, and the
enum is now used wherever the literal string was used previously.
A new page has been created for viewing assets. This replaces the old
`selectedToken` state, which previously would augment the home page
to show token-specific information.
The new asset page shows the standard token overview as seen previously
on the home page, plus a history filtered to show just transactions
relevant to that token.
The actions that were available in the old token list menu have been
moved to a "Token Options" menu that mirrors the "Account Options"
menu.
The `selectedTokenAddress` state has been removed, as it is no longer
being used for anything.
`getMetaMetricState` has been renamed to `getBackgroundMetaMetricState`
because its sole purpose is extracting data from the background state
to send metrics from the background. It's not really a selector, but
it was convenient for it to use the same selectors the UI uses to
extract background data, so I left it there for now.
A new Redux store has been added to track state related to browser history.
The most recent "overview" page (i.e. the home page or the asset page) is
currently being tracked, so that actions taken from the asset page can return
the user back to the asset page when the action has finished.
