All transaction status updates were moved into a `setTimeout` callback
and wrapped in a `try...catch` block in #4131, apparently in an attempt
to prevent failures in event subscribers from interrupting the
transaction logic. The `try...catch` block did accomplish that, but by
putting the status update in a `setTimeout` callback the operation was
made asynchronous.
Transaction status updates now happen unpredictably, in some future
event loop from when they're triggered. This creates a race condition,
where the transaction status update may occur before or after
subsequent state changes. This also introduces a risk of accidentally
undoing a change to the transaction state, as the update made to the
transaction inside the `setTimeout` callback uses a reference to
`txMeta` obtained synchronously before the `setTimeout` call. Any
replacement of the `txMeta` between the `setTxStatus` call and the
execution of the timeout would be erased. Luckily the `txMeta` object
is more often than not mutated rather than replaced, which may explain
why we haven't seen this happen yet.
Everything seems to work correctly with the `setTimeout` call removed,
and now the transaction logic is easier to understand.
The unconnected account alert can now be disabled. A "don't show this
again" checkbox has been added to the alert, which prevents that alert
from being shown in the future.
An alert settings page has been added to the settings as well. This
page allows the user to disable or enable any alert.
This controller was not used. It was used by the
`ComputedBalancesController`, which was removed in #7057 (as it was
also unused).
The pending balances calculator was only used by the balances
controller.
The inactive timer was being reset upon any change to the preferences
store. The intent was only to update the timer when the auto-lock
timeout had changed, so the subscription was updated to only update in
those cases.
There are no indications that this had any effect upon the user. It
looks like the preferences store never updates while the extension is
unattended, so in practice this may have been harmless. It was still
pointless however. This also protects against the possibility of the
preferences store being updated while unattended at some point in the
future.
A race condition exists where after adding an unapproved transaction,
it could be mutated and then replaced when the default gas parameters
are set. This happens because the transaction is added to state and
broadcast before the default gas parameters are set, because
calculating the default gas parameters to use takes some time.
Once they've been calculated, the false assumption was made that the
transaction hadn't changed.
The method responsible for setting the default gas now retrieves an
up-to-date copy of `txMeta`, and conditionally sets the defaults only
if they haven't yet been set.
This race condition was introduced in #2962, though that PR also added
a loading screen that avoided this issue by preventing the user from
interacting with the transaction until after the gas had been
estimated. Unfortunately this loading screen was not carried forward to
the new UI.
* Remove `estimatedGas` property from `txMeta`
The `estimatedGas` property was a cache of the gas value estimated for
a transaction when the default gas limit was set. This property wasn't
used anywhere. It may have been useful for debugging purposes, but the
same gas estimate is already stored on the `history` property so it
should be present in state logs regardless.
* Remove `gasLimitSpecified` txMeta property
The `gasLimitSpecified` property of `txMeta` wasn't used for anything.
It might have been useful for debugging purposes, but whether or not
the gas limit was specified can also be determined from looking at the
transaction history, so it's not a huge loss.
* Remove `gasPriceSpecified` txMeta property
The `gasPriceSpecified` property of `txMeta` wasn't used for anything.
It might have been useful for debugging purposes, but whether or not
the gas price was specified can also be determined from looking at the
transaction history, so it's not a huge loss.
* Remove `simpleSend` txMeta property
The `simpleSend` property of `txMeta` was used to ensure a buffer was
not added to the gas limit during gas estimation for simple send
transactions. It was made redundant by #8484, which accomplishes this
without the use of this property.
Previously a transaction would get assigned a default value during the
`addTxGasDefaults` function, after the transaction was added and sent
to the UI.
Instead the transaction is assigned a default value before it gets
added. This flow is simpler to follow, and it avoids the race condition
where the transaction is assigned a value from the UI before this
default is set. In that situation, the UI-assigned value would be
overridden, which is obviously not desired.
`analyzeGasUsage` now returns the results of the analysis rather than
setting them directly on `txMeta`. The caller is now responsible for
mutating `txMeta` instead. Functionally this should be identical to
before.
The simple send gas estimation has been moved out of the gas estimation
module, and into the transaction controller. This was done in an effort
to limit the number of places where `txMeta` is mutated while the
default gas parameters are being set.
Backport #8445 to v7.7.9. Original commit description:
* Don't updatePendingTxs outside of block updates
Refs #8377
Reverts 507397f6c (#5431)
* Check for new block data on unlock
Co-authored-by: Whymarrh Whitby <whymarrh.whitby@gmail.com>
Backport #8363 to v7.7.9. Note that this uses `clone` instead of
`cloneDeep`, because `clone` hadn't yet been replaced by `cloneDeep` on
`master`.
Backporting that change as well would have been very disruptive, so
I've updated this to use `clone` instead to minimize conflicts. It is
functionally equivalent.
Co-authored-by: Whymarrh Whitby <whymarrh.whitby@gmail.com>
This backports the deletion of the Dai/Sai migration notification
(#8418). Note that the migration to delete the now unused background
state has not been included, as it is non-essential and would have been
more difficult to backport. The migration to delete the unused state
will be included in the next major release instead.
This state has been removed from the background. It was used for the
old UI, and has been unused for some time. A migration has been added
to delete this state as well.
The action creator responsible for updating this state has been removed
from the UI as well, along with the `callBackgroundThenUpdateNoSpinner`
convenience function, which was only used for this action.
* Add popover for informing user about the connected status indicator
* Ensure user only sees connected status info popover once
* Default connectedStatusPopoverHasBeenShown to true and set it to false in a migration
* Add unit test for migration 42
* Initialize AppStateController if it does not exist in migration 42
* Update connect indicator popup locale text
* Code cleanup for connected-indicator-info-popup
* Code cleanup for connected-indicator-info-popup
This method adds the given account to the given origin's list of
exposed accounts. This method is not yet used, but it will be in
subsequent PRs (e.g. #8312)
This method has been added to the background API, and a wrapper action
creator has been written as well.
Now that identities are available synchronously in the permissions
controller, accounts can be validated synchronously as well. Any
account the user wants to give permissions to should already be tracked
as an identity in the preferences controller.
* Fix order of accounts in `eth_accounts` response
The accounts returned by `eth_accounts` were in a fixed order - the
order in which the keyring returned them - rather than ordered with the
selected account first. The accounts returned by the `accountsChanged`
event were ordered with the selected account first, but the same order
wasn't used for `eth_accounts`.
We needed to store additional state in order to determine the correct
account order correctly on all dapps. We had only been storing the
current selected account, but since we also need to determine the
primary account per dapp (i.e. the last "selected" account among the
accounts exposed to that dapp), that wasn't enough.
A `lastSelected` property has been added to each identity in the
preferences controller to keep track of the last selected time. This
property is set to the current time (in milliseconds) whenever a new
selection is made. The accounts returned with `accountsChanged` and by
`eth_accounts` are both ordered by this property.
The `updatePermittedAccounts` function was merged with the internal
methods for responding to account selection, to keep things simpler. It
wasn't called externally anyway, so it wasn't needed in the public API.
* Remove caveat update upon change in selected account
The order of accounts in the caveat isn't meaningful, so the caveat
doesn't need to be updated when the accounts get re-ordered.
* Emit event regardless of account order
Now that we're no longer relying upon the caveat for the account order,
we also have no way of knowing if a particular account selection
resulted in a change in order or not. The notification is now emitted
whenever an exposed account is selected - even if the order stayed the
same.
The inpage provider currently caches the account order, so it can be
relied upon to ignore these redundant events. We were already emiting
redundant `accountsChanged` events in some cases anyway.
Selecting a new account now results in all domains that can view this
change being notified. Previously only the dapp in the active tab was
being notified (though not correctly, as the `origin` was accidentally
set to the MetaMask chrome extension origin).
This handling of account selection has been moved into the background
to minimize the gap between account selection and the notification
being sent out. It's simpler for the UI to not be involved anyway.
Previously all browser globals were allowed to be used anywhere by
ESLint because we had set the `env` property to `browser` in the ESLint
config. This has made it easy to accidentally use browser globals
(e.g. #8338), so it has been removed. Instead we now have a short list
of allowed globals.
All browser globals are now accessed as properties on `window`.
Unfortunately this change resulted in a few different confusing unit
test errors, as some of our unit tests setup assumed that a particular
global would be used via `window` or `global`. In particular,
`window.fetch` didn't work correctly because it wasn't patched by the
AbortController polyfill (only `global.fetch` was being patched).
The `jsdom-global` package we were using complicated matters by setting
all of the JSDOM `window` properties directly on `global`, overwriting
the `AbortController` for example.
The `helpers.js` test setup module has been simplified somewhat by
removing `jsdom-global` and constructing the JSDOM instance manually.
The JSDOM window is set on `window`, and a few properties are set on
`global` as well as needed by various dependencies. `node-fetch` and
the AbortController polyfill/patch now work as expected as well,
though `fetch` is only available on `window` now.
The tests for the detect-tokens controller were nearly all broken. They
have been fixed, and a few improvements were made to controller itself
to help with this.
* The core `detectNewTokens` method has been updated to be async, so
that the caller can know when the operation had completed.
* The part of the function that used `Web3` to check the token balances
has been split into a separate function, so that that part could be
stubbed out in tests. Eventually we should test this using `ganache`
instead, but this was an easier first step.
* The internal `tokenAddresses` array is now initialized on
construction, rather than upon the first Preferences controller update.
The `detectNewTokens` function would have previously failed if it ran
prior to this initialization, so it was failing if called before any
preferences state changes.
Additionally, the `detectTokenBalance` function was removed, as it was
no longer used.
The tests have been updated to ensure they're actually testing the
behavior they purport to be testing. I've simulated a test failure with
each one to check that it'd fail when it should. The preferences
controller instance was updated to set addresses correctly as well.
* Connect screen popup redesign
* Open permission request in notification instead of tab
* Remove no longer user locales
* Update permissions unit test mock to accout for change of opts passed to permissions controller
* Lint fix
* Inline broken line svg in permission-page-container-content.component.js for faster loading
* Add back button to second screen on connect flow
* Add xOfY locale and use for the page count in the connect flow
* Lint fix for svgs permission-page-container-content.component.js
* Fix rebase error
* Lint fix
* Clean up styles on the connect-screen-into-popup branch
* Use closeCurrentWindow to close window on cancel when in full screen connect flow
* Handle errors in rejectPermissionsRequest
* Full screen styles for connect flow
* Lint fixed in permissions-connect and actions.js
* Redirect screen now shows metamask icon instead of users identicon
* Fix subtitle spacing in permissions-connect-header'
* Use window.close instead of closeCurrentWindow() in cancelPermissionsRequest
* Use permissions-connect-header__subtitle in permissions-connect-header.component
Mark Stacey
Whymarrh Whitby
Erik Marks
Dan J Miller