From 11c5890bdd711f6a28d9f6ec5335154998a0a8d8 Mon Sep 17 00:00:00 2001
From: Michele Esposito <34438276+mikesposito@users.noreply.github.com>
Date: Tue, 1 Oct 2024 16:00:09 +0200
Subject: [PATCH] fix!(keyring-eth-ledger-bridge): change `addAccounts` to
 return new accounts only (#63)

<!--
Thanks for your contribution! Take a moment to answer these questions so
that reviewers have the information they need to properly understand
your changes:

* What is the current state of things and why does it need to change?
* What is the solution your changes offer and how does it work?

Are there any issues or other links reviewers should consult to
understand this pull request better? For instance:

* Fixes #12345
* See: #67890
-->

The `addAccounts` method should return newly create accounts, instead of
all the accounts present in the keyring.

Related: https://github.com/MetaMask/accounts-planning/issues/615

## Examples

<!--
Are there any examples of this change being used in another repository?

When considering changes to the MetaMask module template, it's strongly
preferred that the change be experimented with in another repository
first. This gives reviewers a better sense of how the change works,
making it less likely the change will need to be reverted or adjusted
later.
-->
---
 .../keyring-eth-ledger-bridge/jest.config.js  |  4 +--
 .../src/ledger-keyring.test.ts                | 32 ++++++++++++-------
 .../src/ledger-keyring.ts                     |  4 ++-
 3 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/packages/keyring-eth-ledger-bridge/jest.config.js b/packages/keyring-eth-ledger-bridge/jest.config.js
index 64f143e8..c19752ab 100644
--- a/packages/keyring-eth-ledger-bridge/jest.config.js
+++ b/packages/keyring-eth-ledger-bridge/jest.config.js
@@ -25,8 +25,8 @@ module.exports = merge(baseConfig, {
     global: {
       branches: 90.64,
       functions: 95.95,
-      lines: 93.02,
-      statements: 93.09,
+      lines: 94.76,
+      statements: 94.81,
     },
   },
 });
diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts
index 3895e205..be074816 100644
--- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts
+++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts
@@ -388,16 +388,26 @@ describe('LedgerKeyring', function () {
       describe('with a numeric argument', function () {
         it('returns that number of accounts', async function () {
           keyring.setAccountToUnlock(0);
-          const accounts = await keyring.addAccounts(5);
-          expect(accounts).toHaveLength(5);
+          const firstBatch = await keyring.addAccounts(3);
+          keyring.setAccountToUnlock(3);
+          const secondBatch = await keyring.addAccounts(2);
+
+          expect(firstBatch).toHaveLength(3);
+          expect(secondBatch).toHaveLength(2);
         });
 
         it('returns the expected accounts', async function () {
           keyring.setAccountToUnlock(0);
-          const accounts = await keyring.addAccounts(3);
-          expect(accounts[0]).toBe(fakeAccounts[0]);
-          expect(accounts[1]).toBe(fakeAccounts[1]);
-          expect(accounts[2]).toBe(fakeAccounts[2]);
+          const firstBatch = await keyring.addAccounts(3);
+          keyring.setAccountToUnlock(3);
+          const secondBatch = await keyring.addAccounts(2);
+
+          expect(firstBatch).toStrictEqual([
+            fakeAccounts[0],
+            fakeAccounts[1],
+            fakeAccounts[2],
+          ]);
+          expect(secondBatch).toStrictEqual([fakeAccounts[3], fakeAccounts[4]]);
         });
       });
 
@@ -425,13 +435,13 @@ describe('LedgerKeyring', function () {
       describe('when called multiple times', function () {
         it('does not remove existing accounts', async function () {
           keyring.setAccountToUnlock(0);
-          await keyring.addAccounts(1);
+          const firstBatch = await keyring.addAccounts(1);
           keyring.setAccountToUnlock(1);
-          const accounts = await keyring.addAccounts(1);
+          const secondBatch = await keyring.addAccounts(1);
 
-          expect(accounts).toHaveLength(2);
-          expect(accounts[0]).toBe(fakeAccounts[0]);
-          expect(accounts[1]).toBe(fakeAccounts[1]);
+          expect(await keyring.getAccounts()).toHaveLength(2);
+          expect(firstBatch).toStrictEqual([fakeAccounts[0]]);
+          expect(secondBatch).toStrictEqual([fakeAccounts[1]]);
         });
       });
     });
diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts
index a4462baf..d32cc48d 100644
--- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts
+++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts
@@ -242,6 +242,7 @@ export class LedgerKeyring extends EventEmitter {
         .then(async (_) => {
           const from = this.unlockedAccount;
           const to = from + amount;
+          const newAccounts: string[] = [];
           for (let i = from; i < to; i++) {
             const path = this.#getPathForIndex(i);
             let address;
@@ -260,10 +261,11 @@ export class LedgerKeyring extends EventEmitter {
 
             if (!this.accounts.includes(address)) {
               this.accounts = [...this.accounts, address];
+              newAccounts.push(address);
             }
             this.page = 0;
           }
-          resolve(this.accounts.slice());
+          resolve(newAccounts);
         })
         .catch(reject);
     });