Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Do not require scroll-to-bottom for SIWE or Signatures with simulations before enabling Confirm footer #26887

Merged
merged 20 commits into from
Sep 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
66a04dd
refactor: rn isScrollToBottomNeeded-> isScrollToBottomFulfilled
digiwand Sep 4, 2024
aace264
refactor: mv conditional to isConfirmDisabled
digiwand Sep 4, 2024
2c59ab8
fix: rn isScrollToBottomFulfilled -> isScrollToBottomUnfulfilled
digiwand Sep 4, 2024
5f68231
refactor: update isSIWESignatureRequest to allow Confirmation types t…
digiwand Sep 4, 2024
5491bc7
feat: add ConfirmFooter scroll tests
digiwand Sep 4, 2024
00a2998
refactor: rn isScrollToBottomUncompleted unfulfilled -> uncomplete
digiwand Sep 4, 2024
fc6377b
refactor: reverse isScrollToBottomUncompleted -> isScrollToBottomComp…
digiwand Sep 4, 2024
c159bf0
fix: ConfirmFooter scroll tests
digiwand Sep 4, 2024
2c8673a
feat: ConfirmFooter do not require scroll for SIWE
digiwand Sep 4, 2024
9f30b98
refactor: isPermitSignatureRequest can accept any Confirmation to tes…
digiwand Sep 4, 2024
dcf9a9c
feat: do not require scroll if Permit Simulation is shown
digiwand Sep 4, 2024
64cbdd4
fix: confirm snapshots
digiwand Sep 4, 2024
7ceebad
fix: restore async ConfirmPage test
digiwand Sep 4, 2024
16449a8
Merge branch 'develop' into feat-update-confirm-footer-requirements
digiwand Sep 5, 2024
f0db665
fix: update ConfirmPage & Footer tests following merge changes
digiwand Sep 5, 2024
fb116e5
fix: ScrollToBottom test
digiwand Sep 5, 2024
e8ecf66
Merge branch 'develop' into feat-update-confirm-footer-requirements
digiwand Sep 5, 2024
b3d2f3c
Merge branch 'develop' into feat-update-confirm-footer-requirements
digiwand Sep 5, 2024
534c3c9
Merge branch 'develop' into feat-update-confirm-footer-requirements
digiwand Sep 5, 2024
3d47a03
fix: isScrollToBottomNeeded -> isScrollToBottomCompleted
digiwand Sep 5, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 19 additions & 11 deletions test/data/confirmations/helper.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { ApprovalType } from '@metamask/controller-utils';
import { merge } from 'lodash';

import {
Confirmation,
Expand Down Expand Up @@ -142,19 +143,26 @@ export const getMockConfirmStateForTransaction = (
transaction: Confirmation,
args: RootState = { metamask: {} },
) =>
getMockConfirmState({
...args,
metamask: {
...args.metamask,
pendingApprovals: {
[transaction.id]: {
id: transaction.id,
type: ApprovalType.Transaction,
getMockConfirmState(
merge(
{
metamask: {
...args.metamask,
pendingApprovals: {
[transaction.id]: {
id: transaction.id,
type: ApprovalType.Transaction,
},
},
transactions: [transaction],
},
confirm: {
currentConfirmation: transaction,
},
},
transactions: [transaction],
},
});
args,
),
);

export const getMockContractInteractionConfirmState = (
args: RootState = { metamask: {} },
Expand Down
4 changes: 3 additions & 1 deletion test/data/mock-state.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@
"warning": null,
"customTokenAmount": "10"
},
"confirm": {},
"confirm": {
"isScrollToBottomCompleted": true
},
"confirmAlerts": {
"alerts": [],
"confirmed": []
Expand Down
2 changes: 1 addition & 1 deletion ui/ducks/confirm/confirm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const createActionType = (action: string): string =>
export const UPDATE_CONFIRM = createActionType('UPDATE_CONFIRM');

const initState = {
isScrollToBottomNeeded: false,
isScrollToBottomCompleted: true,
};

export default function confirmReducer(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,21 @@ import {
WebHIDConnectedStatuses,
} from '../../../../../../shared/constants/hardware-wallets';
import { BlockaidResultType } from '../../../../../../shared/constants/security-provider';
import {
signatureRequestSIWE,
unapprovedPersonalSignMsg,
} from '../../../../../../test/data/confirmations/personal_sign';
import { permitSignatureMsg } from '../../../../../../test/data/confirmations/typed_sign';
import {
getMockContractInteractionConfirmState,
getMockPersonalSignConfirmState,
getMockPersonalSignConfirmStateForRequest,
getMockTypedSignConfirmState,
getMockTypedSignConfirmStateForRequest,
} from '../../../../../../test/data/confirmations/helper';
import mockState from '../../../../../../test/data/mock-state.json';
import { fireEvent } from '../../../../../../test/jest';
import { renderWithConfirmContextProvider } from '../../../../../../test/lib/confirmations/render-helpers';
import { unapprovedPersonalSignMsg } from '../../../../../../test/data/confirmations/personal_sign';
import * as MMIConfirmations from '../../../../../hooks/useMMIConfirmations';
import * as Actions from '../../../../../store/actions';
import configureStore from '../../../../../store/store';
Expand Down Expand Up @@ -50,7 +56,14 @@ describe('ConfirmFooter', () => {
});

it('should match snapshot with transaction confirmation', () => {
const { container } = render(getMockContractInteractionConfirmState());
const { container } = render(
getMockContractInteractionConfirmState({
confirm: {
isScrollToBottomCompleted: true,
},
metamask: {},
}),
);
expect(container).toMatchSnapshot();
});

Expand All @@ -62,6 +75,88 @@ describe('ConfirmFooter', () => {
expect(getByText('Cancel')).toBeInTheDocument();
});

describe('renders enabled "Confirm" Button', () => {
it('when isScrollToBottomCompleted is true', () => {
const mockStateTypedSign = getMockTypedSignConfirmState({
confirm: {
isScrollToBottomCompleted: true,
},
metamask: {},
});
const { getByText } = render(mockStateTypedSign);

const confirmButton = getByText('Confirm');
expect(confirmButton).not.toBeDisabled();
});

it('when the confirmation is a Sign-in With Ethereum (SIWE) request', () => {
const mockStateSIWE = getMockPersonalSignConfirmStateForRequest(
signatureRequestSIWE,
{
confirm: {
isScrollToBottomCompleted: false,
},
metamask: {},
},
);
const { getByText } = render(mockStateSIWE);

const confirmButton = getByText('Confirm');
expect(confirmButton).not.toBeDisabled();
});

it('when the confirmation is a Permit with the transaction simulation setting enabled', () => {
const mockStatePermit = getMockTypedSignConfirmStateForRequest(
permitSignatureMsg,
{
confirm: {
isScrollToBottomCompleted: false,
},
metamask: {
useTransactionSimulations: true,
},
},
);
const { getByText } = render(mockStatePermit);

const confirmButton = getByText('Confirm');
expect(confirmButton).not.toBeDisabled();
});
});

describe('renders disabled "Confirm" Button', () => {
it('when isScrollToBottomCompleted is false', () => {
const mockStateTypedSign = getMockTypedSignConfirmState({
confirm: {
isScrollToBottomCompleted: false,
},
metamask: {},
});
const { getByText } = render(mockStateTypedSign);

const confirmButton = getByText('Confirm');
expect(confirmButton).toBeDisabled();
});

it('when the confirmation is a Permit with the transaction simulation setting disabled', () => {
const mockStatePermit = getMockTypedSignConfirmStateForRequest(
permitSignatureMsg,
{
confirm: {
isScrollToBottomCompleted: false,
},
metamask: {
useTransactionSimulations: false,
},
},
);
const { getByText } = render(mockStatePermit);

const confirmButton = getByText('Confirm');
expect(confirmButton).toBeDisabled();
});
});

it('invoke action rejectPendingApproval when cancel button is clicked', () => {
const { getAllByRole } = render();
const cancelButton = getAllByRole('button')[0];
Expand Down
32 changes: 23 additions & 9 deletions ui/pages/confirmations/components/confirm/footer/footer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,13 @@ import {
///: END:ONLY_INCLUDE_IF
} from '../../../../../store/actions';
import { confirmSelector } from '../../../selectors';
import { REDESIGN_DEV_TRANSACTION_TYPES } from '../../../utils';
import { selectUseTransactionSimulations } from '../../../selectors/preferences';

import {
isPermitSignatureRequest,
isSIWESignatureRequest,
REDESIGN_DEV_TRANSACTION_TYPES,
} from '../../../utils';
import { useConfirmContext } from '../../../context/confirm';
import { getConfirmationSender } from '../utils';
import { MetaMetricsEventLocation } from '../../../../../../shared/constants/metametrics';
Expand Down Expand Up @@ -109,9 +115,12 @@ const Footer = () => {
const t = useI18nContext();
const confirm = useSelector(confirmSelector);
const customNonceValue = useSelector(getCustomNonceValue);
const useTransactionSimulations = useSelector(
selectUseTransactionSimulations,
);

const { currentConfirmation } = useConfirmContext();
const { isScrollToBottomNeeded } = confirm;
const { isScrollToBottomCompleted } = confirm;
const { from } = getConfirmationSender(currentConfirmation);

///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
Expand All @@ -126,6 +135,17 @@ const Footer = () => {
return false;
});

const isSIWE = isSIWESignatureRequest(currentConfirmation);
const isPermit = isPermitSignatureRequest(currentConfirmation);
const isPermitSimulationShown = isPermit && useTransactionSimulations;

const isConfirmDisabled =
(!isScrollToBottomCompleted && !isSIWE && !isPermitSimulationShown) ||
///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
mmiSubmitDisabled ||
///: END:ONLY_INCLUDE_IF
hardwareWalletRequiresConnection;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be nice to create context for scroll functionality and move these logic to it - so that the logic does not goes to components like footer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that can be done in separate task

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion!

I created a new, enhancement issue to address this here: #26970


const onCancel = useCallback(
({ location }: { location?: MetaMetricsEventLocation }) => {
if (!currentConfirmation) {
Expand Down Expand Up @@ -196,13 +216,7 @@ const Footer = () => {
<ConfirmButton
alertOwnerId={currentConfirmation?.id}
onSubmit={() => onSubmit()}
disabled={
///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
mmiSubmitDisabled ||
///: END:ONLY_INCLUDE_IF
isScrollToBottomNeeded ||
hardwareWalletRequiresConnection
}
disabled={isConfirmDisabled}
onCancel={onCancel}
/>
</PageFooter>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,15 @@ describe('ScrollToBottom', () => {
expect(container.querySelector(buttonSelector)).not.toBeInTheDocument();
});

it('sets isScrollToBottomNeeded to false', () => {
it('sets isScrollToBottomCompleted to true', () => {
const updateSpy = jest.spyOn(ConfirmDucks, 'updateConfirm');
renderWithConfirmContextProvider(
<ScrollToBottom>foobar</ScrollToBottom>,
configureMockStore([])(mockState),
);

expect(updateSpy).toHaveBeenCalledWith({
isScrollToBottomNeeded: false,
isScrollToBottomCompleted: true,
});
});
});
Expand All @@ -86,15 +86,15 @@ describe('ScrollToBottom', () => {
expect(container.querySelector(buttonSelector)).toBeInTheDocument();
});

it('sets isScrollToBottomNeeded to true', () => {
it('sets isScrollToBottomCompleted to false', () => {
const updateSpy = jest.spyOn(ConfirmDucks, 'updateConfirm');
renderWithConfirmContextProvider(
<ScrollToBottom>foobar</ScrollToBottom>,
configureMockStore([])(mockState),
);

expect(updateSpy).toHaveBeenCalledWith({
isScrollToBottomNeeded: true,
isScrollToBottomCompleted: false,
});
});

Expand Down Expand Up @@ -144,6 +144,7 @@ describe('ScrollToBottom', () => {
describe('when user has scrolled to the bottom', () => {
beforeEach(() => {
mockedUseScrollRequiredResult.isScrolledToBottom = true;
mockedUseScrollRequiredResult.hasScrolledToBottom = true;
});

it('hides the button', () => {
Expand All @@ -155,16 +156,15 @@ describe('ScrollToBottom', () => {
expect(container.querySelector(buttonSelector)).not.toBeInTheDocument();
});

it('sets isScrollToBottomNeeded to false', () => {
it('sets isScrollToBottomCompleted to true', () => {
const updateSpy = jest.spyOn(ConfirmDucks, 'updateConfirm');
const { container } = renderWithConfirmContextProvider(
renderWithConfirmContextProvider(
<ScrollToBottom>foobar</ScrollToBottom>,
configureMockStore([])(mockState),
);

expect(container.querySelector(buttonSelector)).not.toBeInTheDocument();
expect(updateSpy).toHaveBeenCalledWith({
isScrollToBottomNeeded: true,
isScrollToBottomCompleted: true,
});
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ const ScrollToBottom = ({ children }: ContentProps) => {
useEffect(() => {
dispatch(
updateConfirm({
isScrollToBottomNeeded: isScrollable && !hasScrolledToBottom,
isScrollToBottomCompleted: !isScrollable || hasScrolledToBottom,
}),
);
}, [isScrollable, hasScrolledToBottom]);
Expand Down
2 changes: 1 addition & 1 deletion ui/pages/confirmations/types/confirm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export type Confirmation = SignatureRequestType | TransactionMeta;

export type ConfirmMetamaskState = {
confirm: {
isScrollToBottomNeeded?: boolean;
isScrollToBottomCompleted?: boolean;
};
metamask: {
pendingApprovals: ApprovalControllerState['pendingApprovals'];
Expand Down
23 changes: 17 additions & 6 deletions ui/pages/confirmations/utils/confirm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
} from '../../../../shared/constants/signatures';
import { parseTypedDataMessage } from '../../../../shared/modules/transaction.utils';
import { sanitizeMessage } from '../../../helpers/utils/util';
import { SignatureRequestType } from '../types/confirm';
import { Confirmation, SignatureRequestType } from '../types/confirm';
import { TYPED_SIGNATURE_VERSIONS } from '../constants';

export const REDESIGN_APPROVAL_TYPES = [
Expand Down Expand Up @@ -50,8 +50,13 @@ export const parseSanitizeTypedDataMessage = (dataToParse: string) => {
return { sanitizedMessage, primaryType };
};

export const isSIWESignatureRequest = (request: SignatureRequestType) =>
Boolean(request?.msgParams?.siwe?.isSIWEMessage);
/**
* Returns true if the request is a SIWE signature request
*
* @param request - The confirmation request to check
*/
export const isSIWESignatureRequest = (request?: Confirmation) =>
Boolean((request as SignatureRequestType)?.msgParams?.siwe?.isSIWEMessage);

export const isOrderSignatureRequest = (request: SignatureRequestType) => {
if (
Expand All @@ -69,17 +74,23 @@ export const isOrderSignatureRequest = (request: SignatureRequestType) => {
return PRIMARY_TYPES_ORDER.includes(primaryType);
};

export const isPermitSignatureRequest = (request: SignatureRequestType) => {
/**
* Returns true if the request is a Permit Typed Sign signature request
*
* @param request - The confirmation request to check
*/
export const isPermitSignatureRequest = (request?: Confirmation) => {
if (
!request ||
!isSignatureTransactionType(request) ||
request.type !== 'eth_signTypedData' ||
request.msgParams?.version?.toUpperCase() === TYPED_SIGNATURE_VERSIONS.V1
(request as SignatureRequestType).msgParams?.version?.toUpperCase() ===
TYPED_SIGNATURE_VERSIONS.V1
) {
return false;
}
const { primaryType } = parseTypedDataMessage(
request.msgParams?.data as string,
(request as SignatureRequestType).msgParams?.data as string,
);

return PRIMARY_TYPES_PERMIT.includes(primaryType);
Expand Down