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.

Sign up for GitHub

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: enhance code performance #211

Merged
merged 3 commits into from
Feb 27, 2024
Merged

feat: enhance code performance #211

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2f1e97d
feat: enhance code performance
stanleyyconsensys Nov 30, 2023
f5ed7d7
fix: update jest score
stanleyyconsensys Nov 30, 2023
0f580cc
fix: remove redundant code
stanleyyconsensys Dec 1, 2023
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
8 changes: 4 additions & 4 deletions jest.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,10 @@ module.exports = {
// An object that configures minimum threshold enforcement for coverage results
coverageThreshold: {
global: {
branches: 68.06,
functions: 88.57,
lines: 81.29,
statements: 81.21,
branches: 68.33,
functions: 88.23,
lines: 81.52,
statements: 81.44,
},
},

Expand Down
45 changes: 22 additions & 23 deletions src/ledger-keyring.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,11 +138,10 @@ export class LedgerKeyring extends EventEmitter {

this.implementFullBIP44 = opts.implementFullBIP44 ?? false;

const keys = new Set<string>(Object.keys(this.accountDetails));
// Remove accounts that don't have corresponding account details
this.accounts = this.accounts.filter((account) =>
Object.keys(this.accountDetails).includes(
ethUtil.toChecksumAddress(account),
),
keys.has(ethUtil.toChecksumAddress(account)),
);

return Promise.resolve();
Expand All @@ -157,26 +156,25 @@ export class LedgerKeyring extends EventEmitter {
};
}
}

const keys = new Set<string>(Object.keys(this.accountDetails));
// try to migrate non-LedgerLive accounts too
if (!this.#isLedgerLiveHdPath()) {
this.accounts
.filter(
(account) =>
!Object.keys(this.accountDetails).includes(
ethUtil.toChecksumAddress(account),
),
)
.forEach((account) => {
try {
this.accountDetails[ethUtil.toChecksumAddress(account)] = {
this.accounts.forEach((account) => {
try {
const key = ethUtil.toChecksumAddress(account);

if (!keys.has(key)) {
this.accountDetails[key] = {
bip44: false,
hdPath: this.#pathFromAddress(account),
};
} catch (error) {
console.log(`failed to migrate account ${account}`);
}
});
// fix a edge case when accounts itself has duplicate address
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks to me like this part is actually a fix unrelated to the performance improvement - does it warrant breaking out and a test-case? Also: Do we know that this is always fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks to me like this part is actually a fix unrelated to the performance improvement - does it warrant breaking out and a test-case? Also: Do we know that this is always fine?

actually i find that is no necessary, or it will not cause any issue if any edge case happen
e.g
accounts = [acc1, acc1, acc2] , accountDetails = {acc2}

if we take a dry run on the Prev code

  1. filter accounts where those not in accountDetails, accounts become [acc1, acc1]
  2. acc1 not in accountDetails, acc1 add to accountDetails
  3. acc1 not in accountDetails, acc1 add to accountDetails , but the content will be same
  4. at the end result will become accountDetails = {acc2, acc1}

if we take a dry run on the Updated code

  1. acc2 in set, skip
  2. acc1 not in set, acc1 add to accountDetails , add to set
  3. acc1 in set, skip
  4. at the end result will become accountDetails = {acc2, acc1}

keys.add(key);
} catch (error) {
console.log(`failed to migrate account ${account}`);
}
});
}
}

Expand Down Expand Up @@ -275,15 +273,16 @@ export class LedgerKeyring extends EventEmitter {
}

removeAccount(address: string) {
if (
!this.accounts.map((a) => a.toLowerCase()).includes(address.toLowerCase())
) {
const filteredAccounts = this.accounts.filter(
(a) => a.toLowerCase() !== address.toLowerCase(),
);

// if filteredAccounts length is the same, means the address is not exist in the accounts
legobeat marked this conversation as resolved.
Show resolved Hide resolved
if (filteredAccounts.length === this.accounts.length) {
throw new Error(`Address ${address} not found in this keyring`);
}

this.accounts = this.accounts.filter(
(a) => a.toLowerCase() !== address.toLowerCase(),
);
this.accounts = filteredAccounts;
delete this.accountDetails[ethUtil.toChecksumAddress(address)];
}

Expand Down