fix issues relating to race conditions where draftTx does not exist #15420
Conversation
| return thunkApi.rejectWithValue( | ||
| 'draftTransaction not found, possibly not on send flow', | ||
| ); |
There was a problem hiding this comment.
without returning this the rest of the code in this slice would run, this is unwanted. Returning here prevents attempts to access draftTransaction erroring out further in this code. Although in this case the error would be handled by redux and would not bubble up to sentry.
| if (draftTransaction) { | ||
| switch (true) { | ||
| case Boolean(draftTransaction.amount.error): | ||
| case Boolean(draftTransaction.gas.error): | ||
| case Boolean(draftTransaction.asset.error): | ||
| case draftTransaction.asset.type === ASSET_TYPES.TOKEN && | ||
| draftTransaction.asset.details === null: | ||
| case state.stage === SEND_STAGES.ADD_RECIPIENT: | ||
| case state.stage === SEND_STAGES.INACTIVE: | ||
| case state.gasEstimateIsLoading: | ||
| case new BigNumber(draftTransaction.gas.gasLimit, 16).lessThan( | ||
| new BigNumber(state.gasLimitMinimum), | ||
| ): | ||
| draftTransaction.status = SEND_STATUSES.INVALID; | ||
| break; | ||
| case draftTransaction.recipient.warning === 'loading': | ||
| case draftTransaction.recipient.warning === | ||
| KNOWN_RECIPIENT_ADDRESS_WARNING && | ||
| draftTransaction.recipient.recipientWarningAcknowledged === false: | ||
| draftTransaction.status = SEND_STATUSES.INVALID; | ||
| break; | ||
| default: | ||
| draftTransaction.status = SEND_STATUSES.VALID; | ||
| } |
There was a problem hiding this comment.
Only change the validity if the draftTransaction exists.
| const draftTransaction = getCurrentDraftTransaction(state); | ||
| if (!draftTransaction) { | ||
| return true; | ||
| } | ||
| return draftTransaction.status === SEND_STATUSES.INVALID; |
There was a problem hiding this comment.
If the draftTransaction does not exist, its invalid.
| if (draftTransaction) { | ||
| draftTransaction.gas.gasLimit = action.payload.gasLimit; | ||
| draftTransaction.gas.gasTotal = action.payload.gasTotal; | ||
| if (action.payload.chainHasChanged) { | ||
| // If the state was reinitialized as a result of the user changing | ||
| // the network from the network dropdown, then the selected asset is | ||
| // no longer valid and should be set to the native asset for the | ||
| // network. | ||
| draftTransaction.asset.type = ASSET_TYPES.NATIVE; | ||
| draftTransaction.asset.balance = | ||
| draftTransaction.fromAccount?.balance ?? | ||
| state.selectedAccount.balance; | ||
| draftTransaction.asset.details = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
So the only way this code should be hit is if there was an inflight attempt to initialize the send state, which at the time had a valid draftTransaction, and then the send flow is reset between when the initialize is fulfilled and when redux runs this case the only way this could be done (to my knowledge) is by changing networks in the send flow and canceling and hope you get lucky to time it right?
|
|
||
| // There may be a case where the send has been canceled by the user while | ||
| // the gas estimate is being computed. So we check again to make sure that | ||
| // a currentTransactionUUID exists and matches the previous tx. | ||
| const newState = thunkApi.getState(); | ||
| if ( | ||
| newState.send.currentTransactionUUID !== sendState.currentTransactionUUID | ||
| ) { | ||
| return thunkApi.rejectWithValue( | ||
| `draftTransaction changed during initialization. | ||
| A new initializeSendState action must be dispatched.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
This should hopefully eliminate the race condition, or make it very difficult to hit.
Explanation
This PR adds additional safety to the send duck when draftTransaction is not present. It does this in a few ways:
fulfilledcondition without a valid draftTransaction implies that we have a race condition, so...More Information
Fixes #15403
Fixes #15401
Screenshots/Screencaps
Before
After
Manual Testing Steps
Pre-Merge Checklist
+ If there are functional changes: