Skip to content

Commit

Permalink
fix!(keyring-eth-ledger-bridge): change addAccounts to return new a…
Browse files Browse the repository at this point in the history
…ccounts 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: MetaMask/accounts-planning#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.
-->
  • Loading branch information
mikesposito authored Oct 1, 2024
1 parent cb10ee6 commit 11c5890
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 14 deletions.
4 changes: 2 additions & 2 deletions packages/keyring-eth-ledger-bridge/jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
},
});
32 changes: 21 additions & 11 deletions packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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]]);
});
});

Expand Down Expand Up @@ -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]]);
});
});
});
Expand Down
4 changes: 3 additions & 1 deletion packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
});
Expand Down

0 comments on commit 11c5890

Please sign in to comment.