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

feat: ledger reconnect #6503

Merged
merged 2 commits into from
Aug 26, 2024

Conversation

brianp
Copy link
Contributor

@brianp brianp commented Aug 23, 2024

Description

If the HidApi disconnects, the instance will still exist but can no
longer be used. We need a new instance, but only a single one can
exist at a time. So we're locking it in a mutex to ensure mutable
safety. Then if at some point communication errors because the instance
is stale we will drop the existing instance by re-assignment of the
inner struct field, and create a new working instance.

Motivation and Context

Allow someone with an open console wallet to be able to connect and disconnect their ledger at will. Without the need to restart the wallet. If the connection to the ledger fails, it will re-establish the connection (only one attempt) and then proceed.

How Has This Been Tested?

Manually with the console wallet.
Using the ledger demo test file it mostly works but there's a weird error return on the reconnection of the ledger before opening the Tari app that could also use some more debugging.

What process can a PR reviewer use to test or verify this change?

  • Open a console wallet supported by ledger
  • Make a transaction successfully
  • Unplug the ledger
  • Make a failed transaction because the ledger is missing
  • Plug the ledger back in
  • Make a transaction successfully

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

brianp added 2 commits August 23, 2024 18:10
If the HidApi disconnects, the instance will still exist but can no
longer be used. We need a new instance, but only a single one can
exist at a time. So we're locking it in a mutex to ensure mutable
safety. Then if at some point communication errors because the instance
is stale we will drop the existing instance by re-assignment of the
inner struct field, and create a new working instance.
Copy link

Test Results (CI)

    3 files    126 suites   39m 35s ⏱️
1 306 tests 1 306 ✅ 0 💤 0 ❌
3 904 runs  3 904 ✅ 0 💤 0 ❌

Results for commit 2b1e14e.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Aug 23, 2024
Copy link

Test Results (Integration tests)

37 tests  +37   37 ✅ +37   17m 0s ⏱️ + 17m 0s
11 suites +11    0 💤 ± 0 
 2 files   + 2    0 ❌ ± 0 

Results for commit 2b1e14e. ± Comparison against base commit e734269.

Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

Hi, tested on Windows. We need some error handling for Invalid value for AppSW (28161) returned when the app is not started but the device connected. (For the demo I commented out the return; if an unexpected response was received to complete the test.)

test: Ledger app not running
✔ Exit the 'MinoTari Wallet' Ledger app and press Enter to continue.. · Ok

Error: Unexpected response (Processing error `Invalid value for AppSW (28161)`)


test: Ledger disconnected
✔ Disconnect the Ledger device and press Enter to continue.. · Ok

test: Ledger reconnected
✔ Reconnect the Ledger device (with password) and press Enter to continue.. · Ok

Error: Unexpected response (Processing error `Invalid value for AppSW (28161)`)


test: Ledger app restart
✔ Start the 'MinoTari Wallet' Ledger app and press Enter to continue.. · Ok
view_key:       64820a542d44b2fcde5cc9dbe2b56f9e486e0a6b7864de3fb1548728f623cb09

Test completed successfully

@SWvheerden SWvheerden merged commit 444b5a3 into tari-project:development Aug 26, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants