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

Fixed ledger not working #74

Merged
merged 4 commits into from
Jan 28, 2023
Merged

Fixed ledger not working #74

merged 4 commits into from
Jan 28, 2023

Conversation

Duddino
Copy link
Member

@Duddino Duddino commented Jan 23, 2023

Abstract

Ledger wasn't working because of the library being updated (I think?) when switching to webpack

What does this PR address?

Fix ledger

What features or improvements were added?

Fix ledger

How does this benefit users?

They can sue ledger

Liquid369
Liquid369 previously approved these changes Jan 23, 2023
Copy link

@Liquid369 Liquid369 left a comment

Choose a reason for hiding this comment

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

utACK c4cff62
Meets standards of the documentation of the lib, I guess soon time to buy a ledger

@JSKitty JSKitty added Bug This is either a bugfix (PR) or a bug (issue). Enhancement New feature or request Mid Priority (~405 PIV) labels Jan 23, 2023
Copy link
Member

@JSKitty JSKitty left a comment

Choose a reason for hiding this comment

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

There's a few issues remaining, tested on Brave + Ledger Nano S;

  • Ledger error codes aren't being translated in to English correctly.
  • Many "String Templates" aren't being filled in the notifications.

image

image

But functionally, I'm able to derive new addresses perfectly fine, and verify them, all as usual.

@Liquid369 Liquid369 dismissed their stale review January 23, 2023 17:12

needs error handling fixes

@Duddino Duddino requested review from JSKitty and Liquid369 January 25, 2023 16:06
Copy link
Member

@JSKitty JSKitty left a comment

Choose a reason for hiding this comment

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

tACK, basics, address imports + verification, and all notifications tested successfully.

Copy link

@Liquid369 Liquid369 left a comment

Choose a reason for hiding this comment

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

uTACK e2162c7

Looks good, soon to test with a ledger myself to help out more
Everything to specs based on the lib

Copy link
Member

@panleone panleone left a comment

Choose a reason for hiding this comment

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

tACK: tested with ledger emulator

@JSKitty JSKitty merged commit 2eb9ce4 into PIVX-Labs:master Jan 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug This is either a bugfix (PR) or a bug (issue). Enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

4 participants