Skip to content

Commit

Permalink
fix: issue with default nonce value being wrong when switching networ…
Browse files Browse the repository at this point in the history
…ks between transactions (#27634)

## **Description**

Nonce in state should be reset correctly when switching between
different networks.
The issue is already fixed in develop by PR:
#27297

## **Related issues**

Fixes: #27657

## **Manual testing steps**

1. Connect to Arbitrum
2. Submit transaction with the high nonce
3. Observe it fails with Internal JSON RPC error
4. Switch to BNB Chain
5. Start a transaction
6. Transaction is created with correct nonce

## **Screenshots/Recordings**
NA

## **Pre-merge author checklist**

- [X] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [X] I've completed the PR template to the best of my ability
- [X] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [X] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
  • Loading branch information
jpuri authored Oct 8, 2024
1 parent c8f966c commit 9446efa
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 7 deletions.
26 changes: 25 additions & 1 deletion ui/pages/confirmations/components/confirm/footer/footer.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -88,20 +88,44 @@ describe('ConfirmFooter', () => {
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
.mockImplementation(() => ({} as any));
const updateCustomNonceSpy = jest
.spyOn(Actions, 'updateCustomNonce')
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
.mockImplementation(() => ({} as any));
const setNextNonceSpy = jest
.spyOn(Actions, 'setNextNonce')
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
.mockImplementation(() => ({} as any));
fireEvent.click(cancelButton);
expect(rejectSpy).toHaveBeenCalled();
expect(updateCustomNonceSpy).toHaveBeenCalledWith('');
expect(setNextNonceSpy).toHaveBeenCalledWith('');
});

it('invoke action resolvePendingApproval when submit button is clicked', () => {
it('invoke required actions when submit button is clicked', () => {
const { getAllByRole } = render();
const submitButton = getAllByRole('button')[1];
const resolveSpy = jest
.spyOn(Actions, 'resolvePendingApproval')
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
.mockImplementation(() => ({} as any));
const updateCustomNonceSpy = jest
.spyOn(Actions, 'updateCustomNonce')
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
.mockImplementation(() => ({} as any));
const setNextNonceSpy = jest
.spyOn(Actions, 'setNextNonce')
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
.mockImplementation(() => ({} as any));
fireEvent.click(submitButton);
expect(resolveSpy).toHaveBeenCalled();
expect(updateCustomNonceSpy).toHaveBeenCalledWith('');
expect(setNextNonceSpy).toHaveBeenCalledWith('');
});

it('displays a danger "Confirm" button there are danger alerts', async () => {
Expand Down
8 changes: 8 additions & 0 deletions ui/pages/confirmations/components/confirm/footer/footer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@ import useAlerts from '../../../../../hooks/useAlerts';
import {
rejectPendingApproval,
resolvePendingApproval,
setNextNonce,
///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
updateAndApproveTx,
///: END:ONLY_INCLUDE_IF
updateCustomNonce,
} from '../../../../../store/actions';
import { confirmSelector } from '../../../selectors';
import { REDESIGN_DEV_TRANSACTION_TYPES } from '../../../utils';
Expand Down Expand Up @@ -156,6 +160,8 @@ const Footer = () => {
dispatch(
rejectPendingApproval(currentConfirmation.id, serializeError(error)),
);
dispatch(updateCustomNonce(''));
dispatch(setNextNonce(''));
},
[currentConfirmation],
);
Expand Down Expand Up @@ -186,6 +192,8 @@ const Footer = () => {
mmiOnSignCallback();
///: END:ONLY_INCLUDE_IF
}
dispatch(updateCustomNonce(''));
dispatch(setNextNonce(''));
}, [currentConfirmation, customNonceValue]);

const onFooterCancel = useCallback(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ export default class ConfirmTransactionBase extends Component {
image: PropTypes.string,
type: PropTypes.string,
getNextNonce: PropTypes.func,
setNextNonce: PropTypes.func,
nextNonce: PropTypes.number,
tryReverseResolveAddress: PropTypes.func.isRequired,
hideSenderToRecipient: PropTypes.bool,
Expand Down Expand Up @@ -696,12 +697,14 @@ export default class ConfirmTransactionBase extends Component {
history,
mostRecentOverviewPage,
updateCustomNonce,
setNextNonce,
} = this.props;

this._removeBeforeUnload();
updateCustomNonce('');
await cancelTransaction(txData);
history.push(mostRecentOverviewPage);
updateCustomNonce('');
setNextNonce('');
}

handleSubmit() {
Expand All @@ -723,6 +726,7 @@ export default class ConfirmTransactionBase extends Component {
history,
mostRecentOverviewPage,
updateCustomNonce,
setNextNonce,
methodData,
maxFeePerGas,
customTokenAmount,
Expand Down Expand Up @@ -786,6 +790,9 @@ export default class ConfirmTransactionBase extends Component {

sendTransaction(txData, false, loadingIndicatorMessage)
.then(() => {
updateCustomNonce('');
setNextNonce('');

if (!this._isMounted) {
return;
}
Expand All @@ -796,19 +803,19 @@ export default class ConfirmTransactionBase extends Component {
},
() => {
history.push(mostRecentOverviewPage);
updateCustomNonce('');
},
);
})
.catch((error) => {
updateCustomNonce('');
setNextNonce('');
if (!this._isMounted) {
return;
}
this.setState({
submitting: false,
submitError: error.message,
});
updateCustomNonce('');
});
},
);
Expand All @@ -822,6 +829,7 @@ export default class ConfirmTransactionBase extends Component {
history,
mostRecentOverviewPage,
updateCustomNonce,
setNextNonce,
unapprovedTxCount,
accountType,
isNotification,
Expand Down Expand Up @@ -894,6 +902,8 @@ export default class ConfirmTransactionBase extends Component {

sendTransaction(txData)
.then(() => {
updateCustomNonce('');
setNextNonce('');
if (txData.custodyStatus) {
showCustodianDeepLink({
fromAddress,
Expand All @@ -912,7 +922,6 @@ export default class ConfirmTransactionBase extends Component {
}
this.setState({ submitting: false }, () => {
history.push(mostRecentOverviewPage);
updateCustomNonce('');
});
},
});
Expand All @@ -926,12 +935,14 @@ export default class ConfirmTransactionBase extends Component {
},
() => {
history.push(mostRecentOverviewPage);
updateCustomNonce('');
},
);
}
})
.catch((error) => {
updateCustomNonce('');
setNextNonce('');

if (!this._isMounted) {
return;
}
Expand All @@ -943,7 +954,6 @@ export default class ConfirmTransactionBase extends Component {
submitError: error.message,
});
setWaitForConfirmDeepLinkDialog(true);
updateCustomNonce('');
});
},
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
updateEditableParams,
setSwapsFeatureFlags,
fetchSmartTransactionsLiveness,
setNextNonce,
} from '../../../store/actions';
import { isBalanceSufficient } from '../send/send.utils';
import { shortenAddress, valuesFor } from '../../../helpers/utils/util';
Expand Down Expand Up @@ -429,6 +430,7 @@ export const mapDispatchToProps = (dispatch) => {
dispatch(fetchSmartTransactionsLiveness());
},
getNextNonce: () => dispatch(getNextNonce()),
setNextNonce: (val) => dispatch(setNextNonce(val)),
setDefaultHomeActiveTabName: (tabName) =>
dispatch(setDefaultHomeActiveTabName(tabName)),
updateTransactionGasFees: (gasFees) => {
Expand Down

0 comments on commit 9446efa

Please sign in to comment.