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.
* 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
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.
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.
The sidebar used to speed up a transaction while it's pending or after
it has failed currently allows editing the gas limit, but that new
limit is ignored. This is especially problematic for transactions that
failed due to a low gas limit, as the problem becomes impossible to fix
by retrying.
The gas limit specified by the user is now used in the speed up
transaction.
Fixes#8156Fixes#7977
This was done to reduce the number of direct dependencies we have. It
should be functionally equivalent. The bundle size should not change,
as we use `clone` as a transitive dependency in a number of places.
* Specify type before parameter name
Various JSDoc `@param` entries were specified as `name {type}` rather
than `{type} name`.
A couple of `@return` entries have been given types as well.
* Use JSDoc optional syntax rather than Closure syntax
* Use @returns rather than @return
* Use consistent built-in type capitalization
Primitive types are lower-case, and Object is upper-case.
* Separate param/return description with a dash
