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

Verify receive address #819

Merged
merged 2 commits into from
Nov 20, 2023

Conversation

edouardparis
Copy link
Member

No description provided.

@edouardparis edouardparis force-pushed the verify-receive-address branch 5 times, most recently from aa2298a to bd19ebe Compare November 20, 2023 11:32
@edouardparis edouardparis marked this pull request as ready for review November 20, 2023 12:11
@darosior
Copy link
Member

A rebase on master would fix the CI, i think. (See #824.)

@darosior
Copy link
Member

darosior commented Nov 20, 2023

I don't know if it has anything to do with the implementation here, but after verifying the address on my Ledger Nano S (not plus) my Ledger's screen stay stuck on "Approve". Trying again unstucks it but it gets stuck again after i approved it. Same with "Reject".

@edouardparis edouardparis force-pushed the verify-receive-address branch from bd19ebe to c7c465c Compare November 20, 2023 14:10
@darosior
Copy link
Member

darosior commented Nov 20, 2023

If i trigger an error (reject the verification or otherwise disconnect the Ledger) it doesn't get cleaned up after i try again and approve the address.

UPDATE: screenshot removed for privacy

@edouardparis
Copy link
Member Author

edouardparis commented Nov 20, 2023

rebased on master to satisfy the ci

@darosior
Copy link
Member

This looks great! 🚀

@edouardparis edouardparis force-pushed the verify-receive-address branch from c7c465c to e9a217b Compare November 20, 2023 14:30
@edouardparis
Copy link
Member Author

fixed the two comments and rebase, I do not know for the Ledger Nano.

@Rob1Ham
Copy link

Rob1Ham commented Nov 20, 2023

I don't know if it has anything to do with the implementation here, but after verifying the address on my Ledger Nano S (not plus) my Ledger's screen stay stuck on "Approve". Trying again unstucks it but it gets stuck again after i approved it. Same with "Reject".

For what it is worth, I've seen this issue as well using the javascript library as well.

@darosior
Copy link
Member

Good to know.

@darosior
Copy link
Member

cc @bigspider: the above two comments may be of interest to you.

@darosior
Copy link
Member

Tested this on a mainnet wallet with a Ledger Nano S. Looks good to me. Will now ask @jp1ac4 to have a look at the code.

@darosior darosior requested a review from jp1ac4 November 20, 2023 14:52
@bigspider
Copy link

bigspider commented Nov 20, 2023

cc @bigspider: the above two comments may be of interest to you.

Please let me know how I can try to reproduce (e.g. policy and PSBT you were signing) and I'll give this a look ASAP

Copy link
Collaborator

@jp1ac4 jp1ac4 left a comment

Choose a reason for hiding this comment

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

utACK e9a217b.

As well as reviewing the code, I checked the receive screen, but did not verify any addresses on a device.

@darosior
Copy link
Member

@edouardparis i trust you tested this with Bitbox and the Nano S+?

@darosior darosior merged commit 1df4ab8 into wizardsardine:master Nov 20, 2023
@edouardparis edouardparis deleted the verify-receive-address branch November 20, 2023 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants