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.

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

fix ledger unlocking using hdPath in accountDetails #146

Merged
merged 6 commits into from
May 26, 2022
Merged

Conversation

jpuri
Copy link
Contributor

@jpuri jpuri commented May 9, 2022

@jpuri jpuri requested review from darkwing and danjm May 9, 2022 12:17
@jpuri jpuri requested a review from a team as a code owner May 9, 2022 12:17
@jpuri jpuri force-pushed the ledger_account_fix branch from 41cb74b to 14c491a Compare May 9, 2022 17:36
@jpuri jpuri force-pushed the ledger_account_fix branch from 5334dd5 to 3d5b3f9 Compare May 9, 2022 18:41
@@ -133,8 +133,10 @@ class LedgerBridgeKeyring extends EventEmitter {
},
({ success, payload }) => {
if (success) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to add a unit test for each of the true and false cases for updateHdk? (Or update existing tests of methods that call unlock to ensure that they would fail without this change)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep I added test cases

Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

Looks good. Only request is to add/modify at least one

@seaona
Copy link

seaona commented May 16, 2022

from QA side - working fine @jpuri

@jpuri jpuri requested a review from danjm May 18, 2022 17:56
Copy link
Contributor

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

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

Looks and works good to me

@jpuri jpuri merged commit 239e4a7 into main May 26, 2022
@jpuri jpuri deleted the ledger_account_fix branch May 26, 2022 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ledger Nano: Metamask Address does not correspond to Ledger
5 participants