Fix speed-up/cancel: don't update existing transaction data #14355
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
Hi @danjm : we may need to do more code cleanup. I will run this branch locally and check. |
ui/contexts/gasFee.js
Outdated
| minimumGasLimit, | ||
| editGasMode, | ||
| setRetryTxMeta, | ||
| ); | ||
| return ( | ||
| <GasFeeContext.Provider value={gasFeeDetails}> |
There was a problem hiding this comment.
Hi @danjm , I would prefer to see 2 changes in this PR:
- Move logic
const [retryTxMeta, setRetryTxMeta] = useState({touseGasFeeInputshook as that is place where all logic of this context lies. I would avoid passingsetRetryTxMetadown to 2 levels. - Remove previous gas details that I added here: https://github.com/MetaMask/metamask-extension/blob/develop/ui/hooks/gasFeeInput/useTransactionFunctions.js#L44 they are redundant now.
There was a problem hiding this comment.
@jpuri I just pushed a commit for your first suggestion.
For the second suggestion, are previous gas params still needed for this code to work -> https://github.com/MetaMask/metamask-extension/blob/develop/ui/components/app/edit-gas-fee-popover/edit-gas-item/useGasItemFeeDetails.js#L109
useEffect(() => {
// For cancel and speed-up medium / high option is disabled if
// gas used in transaction + 10% is greater tham estimate
if (
(editGasMode === EDIT_GAS_MODES.CANCEL ||
editGasMode === EDIT_GAS_MODES.SPEED_UP) &&
(priorityLevel === PRIORITY_LEVELS.MEDIUM ||
priorityLevel === PRIORITY_LEVELS.HIGH)
) {
const estimateGreater = !gasEstimateGreaterThanGasUsedPlusTenPercent(
transaction.previousGas || transaction.txParams,
gasFeeEstimates,
priorityLevel,
);
setEstimateGreaterThanGasUse(estimateGreater);
}
}, [editGasMode, gasFeeEstimates, priorityLevel, transaction]);
There was a problem hiding this comment.
I was thinking may be we can use this field _transaction.originalGasEstimate we need previousGas to derive new estimate type Plus 10 Percent for cancel and speedup transactions.
There was a problem hiding this comment.
actually, originalGasEstimate is just the gas limit
I think previousGas might be needed
|
@seaona That might be related... not sure. We should investigate |
danjm
force-pushed
the
fix-speedup-v10.13.0
branch
from
0a32bcb
to
b7effc5
Compare
|
|
||
| expect( | ||
| screen.queryAllByTitle(`${EXPECTED_ETH_FEE_2} ETH`).length, | ||
| ).toBeGreaterThan(0); |
…enarios now that updateTransaction used in cancel-speedup is defined on the front end
danjm
force-pushed
the
fix-speedup-v10.13.0
branch
from
86827f7
to
8ee6553
Compare
Fixes #14284
Explanation:
EIP-1559 v2 introduced new logic for speed up and cancellation. This new logic updates metamask state data of the existing transaction (the one being replaced / "sped up" / "cancelled"). We should not do this. Instead, we should use the data of the existing transaction (in particular the nonce) to create an entirely new transaction.
The error was caught because of the new transaction update methods introduced in #13646. Those methods (correctly) throw an error when attempting to update a transaction that is not in an "unapproved" state.
This PR fixes this by maintaining transaction data for speed ups and cancels in front-end / react state, instead of modifying the background transaction.