Skip to content

Commit

Permalink
feat: multiple accounts support in ledger (#10109)
Browse files Browse the repository at this point in the history
## **Description**
This PR will enable the multiple accounts supports for ledger devices.
Following changes has been made in this PR:
1. use @metamask/eth-ledger-keyring-bridge@4.0.0 to replace old
@consensys/ledgerhq-metamask-keyring@0.0.9
2. add `LedgerSelectAccount` component to allow user select multiple
accounts from ledger devices. (screen is similiar to QR code select
account screen)
3. add `remove accounts` for all hardware wallet accounts in
`AccountActions.tsx` file.
4. add some metric logging for `remove accounts` and `connect accounts`
5. modify the `engine.ts` code and `ledger.ts` to allow intialise the
new ledger keyring and its middleware and transport object.
6. Modify the `BlockingActionModel` to support `onAnimationCompleted`
event so that we can have better smooth model animation than before.
(very lagging animation when some heavy operations like import multiple
accounts happened in the background)


<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
7. What is the improvement/solution?
-->

## **Related issues**

Fixes:

## **Manual testing steps**

**Connect Multiple Ledger accounts**

1. Launch wallet
2. Create a new account
3. Once on the wallet view, add a new account or hardware wallet
4. Add hardware wallet
5. Ledger
6. Once device is found, click Continue
7. Tick the selected Ledger accounts to be imported
8. Click Unlock


**Forget Multiple Ledger accounts**

1. Use an existing wallet that has multiple Ledger accounts imported
2. Once on the wallet view, add a new account or hardware wallet
4. Add hardware wallet
5. Ledger
6. Once device is found, click 'Continue'
7. Click 'Forget this device' button
8. User will be returned to account list with all Ledger accounts
removed

**Forget Individual Ledger Account**

1. Use an existing wallet that has multiple Ledger accounts imported
2. Once on the wallet view, click on the three dots beside the Address
section
3. Click 'Remove hardware account'
4. Click 'Remove'
5. User will be returned to account list with the individual Ledger
account removed

**Forget Individual QR Code Wallet Account**

1. Use an existing wallet that has multiple QR Code accounts imported
2. Once on the wallet view, click on the three dots beside the Address
section
3. Click 'Remove hardware account'
4. Click 'Remove'
5. User will be returned to account list with the individual QR Code
account removed

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.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
- [x] 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-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [x] 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.

---------

Co-authored-by: Gustavo Antunes <17601467+gantunesr@users.noreply.github.com>
  • Loading branch information
dawnseeker8 and gantunesr authored Jul 16, 2024
1 parent 70d0fcc commit 38cf842
Show file tree
Hide file tree
Showing 34 changed files with 2,045 additions and 564 deletions.
17 changes: 12 additions & 5 deletions app/components/Nav/App/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,6 @@ import ImportPrivateKey from '../../Views/ImportPrivateKey';
import ImportPrivateKeySuccess from '../../Views/ImportPrivateKeySuccess';
import ConnectQRHardware from '../../Views/ConnectQRHardware';
import SelectHardwareWallet from '../../Views/ConnectHardware/SelectHardware';
import LedgerAccountInfo from '../../Views/LedgerAccountInfo';
import LedgerConnect from '../../Views/LedgerConnect';
import { AUTHENTICATION_APP_TRIGGERED_AUTH_NO_CREDENTIALS } from '../../../constants/error';
import { UpdateNeeded } from '../../../components/UI/UpdateNeeded';
import { EnableAutomaticSecurityChecksModal } from '../../../components/UI/EnableAutomaticSecurityChecksModal';
Expand Down Expand Up @@ -111,6 +109,7 @@ import { MetaMetrics } from '../../../core/Analytics';
import trackErrorAsAnalytics from '../../../util/metrics/TrackError/trackErrorAsAnalytics';
import generateDeviceAnalyticsMetaData from '../../../util/metrics/DeviceAnalyticsMetaData/generateDeviceAnalyticsMetaData';
import generateUserSettingsAnalyticsMetaData from '../../../util/metrics/UserSettingsAnalyticsMetaData/generateUserProfileAnalyticsMetaData';
import LedgerSelectAccount from '../../Views/LedgerSelectAccount';
import OnboardingSuccess from '../../Views/OnboardingSuccess';
import DefaultSettings from '../../Views/OnboardingSuccess/DefaultSettings';
import BasicFunctionalityModal from '../../UI/BasicFunctionality/BasicFunctionalityModal/BasicFunctionalityModal';
Expand Down Expand Up @@ -473,6 +472,7 @@ const App = ({ userLoggedIn }) => {
}
}
}

initSDKConnect()
.then(() => {
queueOfHandleDeeplinkFunctions.current.forEach((func) => func());
Expand Down Expand Up @@ -741,8 +741,16 @@ const App = ({ userLoggedIn }) => {
);

const LedgerConnectFlow = () => (
<Stack.Navigator initialRouteName={Routes.HW.LEDGER_CONNECT}>
<Stack.Screen name={Routes.HW.LEDGER_CONNECT} component={LedgerConnect} />
<Stack.Navigator
screenOptions={{
headerShown: false,
}}
initialRouteName={Routes.HW.LEDGER_CONNECT}
>
<Stack.Screen
name={Routes.HW.LEDGER_CONNECT}
component={LedgerSelectAccount}
/>
</Stack.Navigator>
);

Expand All @@ -753,7 +761,6 @@ const App = ({ userLoggedIn }) => {
component={SelectHardwareWallet}
options={SelectHardwareWallet.navigationOptions}
/>
<Stack.Screen name="LedgerAccountInfo" component={LedgerAccountInfo} />
</Stack.Navigator>
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ exports[`BlockingActionModal should render correctly 1`] = `
deviceHeight={null}
deviceWidth={null}
hasBackdrop={true}
hideModalContentWhileAnimating={false}
hideModalContentWhileAnimating={true}
isVisible={true}
onBackButtonPress={[Function]}
onBackdropPress={[Function]}
Expand Down
5 changes: 5 additions & 0 deletions app/components/UI/BlockingActionModal/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export default function BlockingActionModal({
children,
modalVisible,
isLoadingAction,
onAnimationCompleted,
}) {
const { colors } = useTheme();
const styles = createStyles(colors);
Expand All @@ -43,6 +44,8 @@ export default function BlockingActionModal({
backdropOpacity={1}
isVisible={modalVisible}
style={styles.modal}
onModalShow={onAnimationCompleted}
hideModalContentWhileAnimating
>
<View style={styles.modalView}>
<View style={baseStyles.flexGrow}>
Expand All @@ -69,4 +72,6 @@ BlockingActionModal.propTypes = {
* Content to display above the action buttons
*/
children: PropTypes.node,

onAnimationCompleted: PropTypes.func,
};
11 changes: 7 additions & 4 deletions app/components/UI/HardwareWallet/AccountSelector/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ interface ISelectQRAccountsProps {
selectedAccounts: string[];
nextPage: () => void;
prevPage: () => void;
toggleAccount: (index: number) => void;
onCheck?: (index: number) => void;
onUnlock: (accountIndex: number[]) => void;
onForget: () => void;
title: string;
Expand All @@ -30,7 +30,7 @@ const AccountSelector = (props: ISelectQRAccountsProps) => {
accounts,
prevPage,
nextPage,
toggleAccount,
onCheck,
selectedAccounts,
onForget,
onUnlock,
Expand Down Expand Up @@ -69,9 +69,12 @@ const AccountSelector = (props: ISelectQRAccountsProps) => {
prev.has(index) ? prev.delete(index) : prev.add(index);
return new Set(prev);
});
toggleAccount(index);

if (onCheck) {
onCheck(index);
}
},
[toggleAccount],
[onCheck],
);

return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export const createStyle = (colors: any) =>
bottom: {
alignItems: 'center',
justifyContent: 'space-between',
paddingTop: 70,
paddingTop: 30,
paddingBottom: Device.isIphoneX() ? 20 : 10,
},
button: {
Expand Down
12 changes: 3 additions & 9 deletions app/components/UI/LedgerModals/LedgerConfirmationModal.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,11 @@ import {
BluetoothPermissionErrors,
LedgerCommunicationErrors,
} from '../../../core/Ledger/ledgerErrors';
import { unlockLedgerDefaultAccount } from '../../../core/Ledger/Ledger';
import { strings } from '../../../../locales/i18n';
import { useMetrics } from '../../hooks/useMetrics';
import { MetaMetricsEvents } from '../../../core/Analytics';
import { fireEvent } from '@testing-library/react-native';

jest.mock('../../../core/Ledger/Ledger', () => ({
unlockLedgerDefaultAccount: jest.fn(),
}));
import { HardwareDeviceTypes } from '../../../constants/keyringTypes';

jest.mock('../../hooks/Ledger/useBluetooth', () => ({
__esModule: true,
Expand Down Expand Up @@ -340,7 +336,6 @@ describe('LedgerConfirmationModal', () => {

it('calls onConfirmation when ledger commands are being sent and confirmed have been received.', async () => {
const onConfirmation = jest.fn();
unlockLedgerDefaultAccount.mockReturnValue(Promise.resolve(true));
useLedgerBluetooth.mockReturnValue({
isSendingLedgerCommands: true,
isAppLaunchConfirmationNeeded: false,
Expand All @@ -359,11 +354,10 @@ describe('LedgerConfirmationModal', () => {
// eslint-disable-next-line @typescript-eslint/no-empty-function
await act(async () => {});

expect(unlockLedgerDefaultAccount).toHaveBeenCalled();
expect(onConfirmation).toHaveBeenCalled();
});

it('logs LEDGER_HARDWARE_WALLET_ERROR thrown by unlockLedgerDefaultAccount', async () => {
it('logs LEDGER_HARDWARE_WALLET_ERROR event when the ledger error occurs', async () => {
const onConfirmation = jest.fn();

const ledgerLogicToRun = jest.fn();
Expand Down Expand Up @@ -400,7 +394,7 @@ describe('LedgerConfirmationModal', () => {
1,
MetaMetricsEvents.LEDGER_HARDWARE_WALLET_ERROR,
{
device_type: 'Ledger',
device_type: HardwareDeviceTypes.LEDGER,
error: 'LEDGER_ETH_APP_NOT_INSTALLED',
},
);
Expand Down
13 changes: 6 additions & 7 deletions app/components/UI/LedgerModals/LedgerConfirmationModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@ import ConfirmationStep from './Steps/ConfirmationStep';
import ErrorStep from './Steps/ErrorStep';
import OpenETHAppStep from './Steps/OpenETHAppStep';
import SearchingForDeviceStep from './Steps/SearchingForDeviceStep';
import { unlockLedgerDefaultAccount } from '../../../core/Ledger/Ledger';
import { MetaMetricsEvents } from '../../../core/Analytics';
import { useMetrics } from '../../../components/hooks/useMetrics';
import {
BluetoothPermissionErrors,
LedgerCommunicationErrors,
} from '../../../core/Ledger/ledgerErrors';
import { HardwareDeviceTypes } from '../../../constants/keyringTypes';

const createStyles = (colors: Colors) =>
StyleSheet.create({
Expand Down Expand Up @@ -71,14 +71,13 @@ const LedgerConfirmationModal = ({
const connectLedger = () => {
try {
ledgerLogicToRun(async () => {
await unlockLedgerDefaultAccount(false);
await onConfirmation();
});
} catch (_e) {
// Handle a super edge case of the user starting a transaction with the device connected
// After arriving to confirmation the ETH app is not installed anymore this causes a crash.
trackEvent(MetaMetricsEvents.LEDGER_HARDWARE_WALLET_ERROR, {
device_type: 'Ledger',
device_type: HardwareDeviceTypes.LEDGER,
error: 'LEDGER_ETH_APP_NOT_INSTALLED',
});
}
Expand All @@ -90,7 +89,7 @@ const LedgerConfirmationModal = ({
onRejection();
} finally {
trackEvent(MetaMetricsEvents.LEDGER_HARDWARE_TRANSACTION_CANCELLED, {
device_type: 'Ledger',
device_type: HardwareDeviceTypes.LEDGER,
});
}
};
Expand Down Expand Up @@ -179,7 +178,7 @@ const LedgerConfirmationModal = ({
}
if (ledgerError !== LedgerCommunicationErrors.UserRefusedConfirmation) {
trackEvent(MetaMetricsEvents.LEDGER_HARDWARE_WALLET_ERROR, {
device_type: 'Ledger',
device_type: HardwareDeviceTypes.LEDGER,
error: `${ledgerError}`,
});
}
Expand Down Expand Up @@ -208,7 +207,7 @@ const LedgerConfirmationModal = ({
}
setPermissionErrorShown(true);
trackEvent(MetaMetricsEvents.LEDGER_HARDWARE_WALLET_ERROR, {
device_type: 'Ledger',
device_type: HardwareDeviceTypes.LEDGER,
error: 'LEDGER_BLUETOOTH_PERMISSION_ERR',
});
}
Expand All @@ -219,7 +218,7 @@ const LedgerConfirmationModal = ({
subtitle: strings('ledger.bluetooth_off_message'),
});
trackEvent(MetaMetricsEvents.LEDGER_HARDWARE_WALLET_ERROR, {
device_type: 'Ledger',
device_type: HardwareDeviceTypes.LEDGER,
error: 'LEDGER_BLUETOOTH_CONNECTION_ERR',
});
}
Expand Down
9 changes: 8 additions & 1 deletion app/components/Views/AccountActions/AccountActions.styles.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,25 @@
// Third party dependencies.
import { StyleSheet } from 'react-native';
import { fontStyles } from '../../../styles/common';
import { Colors } from '../../../util/theme/models';

/**
* Style sheet function for AccountActions component.
*
* @returns StyleSheet object.
*/
const styleSheet = () =>
const styleSheet = (colors: Colors) =>
StyleSheet.create({
actionsContainer: {
alignItems: 'flex-start',
justifyContent: 'center',
paddingVertical: 16,
},
text: {
color: colors.text.default,
fontSize: 14,
...fontStyles.normal,
},
});

export default styleSheet;
Loading

0 comments on commit 38cf842

Please sign in to comment.