Skip to content

Commit

Permalink
fix: QR/Keystone notification window disappears (MetaMask#22460)
Browse files Browse the repository at this point in the history
## **Description**

Fixes the longstanding bug where the notification window disappears when
using a QR wallet.

## **Related issues**

Fixes: MetaMask#19780

## **Manual testing steps**

1. Use any QR wallet (AirGap Vault is free)
2. Go to https://metamask.github.io/test-dapp/
3. Use any of the Sign methods (useful for testing because they do not
require gas in your account)
4. Click 'Sign' in the Notification window that appears
5. The window **does not** close
  • Loading branch information
HowardBraham authored Jan 12, 2024
1 parent e889bfe commit 6255979
Show file tree
Hide file tree
Showing 4 changed files with 10 additions and 52 deletions.
6 changes: 3 additions & 3 deletions ui/pages/home/home.component.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,21 +99,21 @@ function shouldCloseNotificationPopup({
institutionalConnectRequests,
///: END:ONLY_INCLUDE_IF
}) {
let shouldCLose =
let shouldClose =
isNotification &&
totalUnapprovedCount === 0 &&
!hasApprovalFlows &&
!isSigningQRHardwareTransaction;

///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
shouldCLose &&=
shouldClose &&=
// MMI User must be shown a deeplink
!waitForConfirmDeepLinkDialog &&
// MMI User is connecting to custodian
institutionalConnectRequests.length === 0;
///: END:ONLY_INCLUDE_IF

return shouldCLose;
return shouldClose;
}

export default class Home extends PureComponent {
Expand Down
9 changes: 2 additions & 7 deletions ui/pages/home/home.container.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ import {
getShowTermsOfUse,
getShowOutdatedBrowserWarning,
getNewNetworkAdded,
hasUnsignedQRHardwareTransaction,
hasUnsignedQRHardwareMessage,
getIsSigningQRHardwareTransaction,
getNewNftAddedMessage,
getNewTokensImported,
getShouldShowSeedPhraseReminder,
Expand Down Expand Up @@ -123,10 +122,6 @@ const mapStateToProps = (state) => {
getWeb3ShimUsageStateForOrigin(state, originOfCurrentTab) ===
Web3ShimUsageAlertStates.recorded;

const isSigningQRHardwareTransaction =
hasUnsignedQRHardwareTransaction(state) ||
hasUnsignedQRHardwareMessage(state);

const hasWatchTokenPendingApprovals = getSuggestedTokens(state).length > 0;

const hasWatchNftPendingApprovals = getSuggestedNfts(state).length > 0;
Expand Down Expand Up @@ -170,7 +165,7 @@ const mapStateToProps = (state) => {
getIsBrowserDeprecated() && getShowOutdatedBrowserWarning(state),
seedPhraseBackedUp,
newNetworkAddedName: getNewNetworkAdded(state),
isSigningQRHardwareTransaction,
isSigningQRHardwareTransaction: getIsSigningQRHardwareTransaction(state),
newNftAddedMessage: getNewNftAddedMessage(state),
removeNftMessage: getRemoveNftMessage(state),
newTokensImported: getNewTokensImported(state),
Expand Down
44 changes: 2 additions & 42 deletions ui/selectors/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ import {
HardwareTransportStates,
} from '../../shared/constants/hardware-wallets';
import { KeyringType } from '../../shared/constants/keyring';
import { MESSAGE_TYPE } from '../../shared/constants/app';

import { TRUNCATED_NAME_CHAR_LIMIT } from '../../shared/constants/labels';

Expand Down Expand Up @@ -166,47 +165,8 @@ export function getCurrentQRHardwareState(state) {
return qrHardware || {};
}

export function hasUnsignedQRHardwareTransaction(state) {
const { txParams } = state.confirmTransaction.txData;
if (!txParams) {
return false;
}
const { from } = txParams;
const { keyrings } = state.metamask;
const qrKeyring = keyrings.find((kr) => kr.type === KeyringType.qr);
if (!qrKeyring) {
return false;
}
return Boolean(
qrKeyring.accounts.find(
(account) => account.toLowerCase() === from.toLowerCase(),
),
);
}

export function hasUnsignedQRHardwareMessage(state) {
const { type, msgParams } = state.confirmTransaction.txData;
if (!type || !msgParams) {
return false;
}
const { from } = msgParams;
const { keyrings } = state.metamask;
const qrKeyring = keyrings.find((kr) => kr.type === KeyringType.qr);
if (!qrKeyring) {
return false;
}
switch (type) {
case MESSAGE_TYPE.ETH_SIGN_TYPED_DATA:
case MESSAGE_TYPE.ETH_SIGN:
case MESSAGE_TYPE.PERSONAL_SIGN:
return Boolean(
qrKeyring.accounts.find(
(account) => account.toLowerCase() === from.toLowerCase(),
),
);
default:
return false;
}
export function getIsSigningQRHardwareTransaction(state) {
return state.metamask.qrHardware?.sign?.request !== undefined;
}

export function getCurrentKeyring(state) {
Expand Down
3 changes: 3 additions & 0 deletions ui/store/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import {
hasTransactionPendingApprovals,
getApprovalFlows,
getCurrentNetworkTransactions,
getIsSigningQRHardwareTransaction,
///: BEGIN:ONLY_INCLUDE_IF(snaps)
getNotifications,
///: END:ONLY_INCLUDE_IF
Expand Down Expand Up @@ -2479,6 +2480,7 @@ export function hideIpfsModal(): Action {
type: actionConstants.SHOW_IPFS_MODAL_CLOSE,
};
}

export function closeCurrentNotificationWindow(): ThunkAction<
void,
MetaMaskReduxState,
Expand All @@ -2491,6 +2493,7 @@ export function closeCurrentNotificationWindow(): ThunkAction<
if (
getEnvironmentType() === ENVIRONMENT_TYPE_NOTIFICATION &&
!hasTransactionPendingApprovals(state) &&
!getIsSigningQRHardwareTransaction(state) &&
approvalFlows.length === 0
) {
closeNotificationPopup();
Expand Down

0 comments on commit 6255979

Please sign in to comment.