Skip to content

Add TCP transport support for Ledger wallet#3593

Merged
mergify[bot] merged 5 commits intomainfrom
tomas/ledger-tcp-trans
Aug 12, 2024
Merged

Add TCP transport support for Ledger wallet#3593
mergify[bot] merged 5 commits intomainfrom
tomas/ledger-tcp-trans

Conversation

@tzemanovic
Copy link
Copy Markdown
Collaborator

@tzemanovic tzemanovic commented Aug 7, 2024 •
edited
Loading

Describe your changes

TODO:

  • add instructions on how to setup and use emulator

Indicate on which release or other PRs this topic is based on

#3570

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@tzemanovic tzemanovic force-pushed the tomas/ledger-tcp-trans branch 2 times, most recently from 93928a8 to 2dab9ae Compare August 7, 2024 16:44
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 7, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 7.04225% with 132 lines in your changes missing coverage. Please review.

Project coverage is 54.60%. Comparing base (5959d7d) to head (5a1a69b).
Report is 42 commits behind head on main.

Files Patch % Lines
crates/apps_lib/src/wallet/transport.rs 0.00% 39 Missing ⚠️
crates/apps_lib/src/cli.rs 0.00% 32 Missing ⚠️
crates/apps_lib/src/config/genesis/transactions.rs 0.00% 15 Missing ⚠️
crates/apps_lib/src/client/utils.rs 0.00% 12 Missing ⚠️
crates/apps_lib/src/client/tx.rs 0.00% 11 Missing ⚠️
crates/sdk/src/args.rs 0.00% 9 Missing ⚠️
crates/apps_lib/src/config/genesis/utils.rs 0.00% 7 Missing ⚠️
crates/apps_lib/src/cli/wallet.rs 0.00% 4 Missing ⚠️
crates/sdk/src/lib.rs 0.00% 2 Missing ⚠️
crates/apps_lib/src/config/genesis/templates.rs 90.90% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3593      +/-   ##
==========================================
+ Coverage   54.55%   54.60%   +0.05%     
==========================================
  Files         323      324       +1     
  Lines      113871   113723     -148     
==========================================
- Hits        62118    62096      -22     
+ Misses      51753    51627     -126

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tzemanovic tzemanovic force-pushed the tomas/ledger-tcp-trans branch from caccb88 to 3a31025 Compare August 8, 2024 11:55
tzemanovic added a commit that referenced this pull request Aug 8, 2024
@tzemanovic
changelog: add #3593
670edc5
@tzemanovic tzemanovic requested a review from murisi August 8, 2024 11:57
@tzemanovic tzemanovic marked this pull request as ready for review August 8, 2024 11:57
murisi
murisi approved these changes Aug 11, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@murisi murisi 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 to me, thanks! Tested this on both the physical device and Speculos, and everything is working. It would have been nice to avoid needing to use macros, but it seems like the definition of Exchange precludes the use of trait objects.

@tzemanovic
Copy link
Copy Markdown
Collaborator Author

tzemanovic commented Aug 12, 2024

Looks good to me, thanks! Tested this on both the physical device and Speculos, and everything is working. It would have been nice to avoid needing to use macros, but it seems like the definition of Exchange precludes the use of trait objects.

Agree, it would be nice to get rid of the macro. I think we could make an enum for the transport layer. I'll add the change

@tzemanovic
Copy link
Copy Markdown
Collaborator Author

tzemanovic commented Aug 12, 2024

@murisi added 8f5cc8b

@tzemanovic tzemanovic force-pushed the tomas/ledger-tcp-trans branch from 8f5cc8b to 5a1a69b Compare August 12, 2024 11:04
@tzemanovic
Copy link
Copy Markdown
Collaborator Author

tzemanovic commented Aug 12, 2024

squashed fixups in last push

@tzemanovic tzemanovic added the merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass label Aug 12, 2024
murisi
murisi approved these changes Aug 12, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@murisi murisi 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, the device transport construction code is no longer duplicated. Thanks!

mergify bot added a commit that referenced this pull request Aug 12, 2024
@mergify
Merge of #3593
448f30c
@mergify mergify bot mentioned this pull request Aug 12, 2024
Closed
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Aug 12, 2024

Hey @tzemanovic, your pull request has been dequeued due to the following reason: CHECKS_FAILED.
Sorry about that, but you can requeue the PR by using @mergifyio requeue if you think this was a mistake.

@tzemanovic
Copy link
Copy Markdown
Collaborator Author

tzemanovic commented Aug 12, 2024

@Mergifyio requeue

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Aug 12, 2024

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

mergify bot added a commit that referenced this pull request Aug 12, 2024
@mergify
Merge of #3593
4bf80af
@mergify mergify bot mentioned this pull request Aug 12, 2024
Closed
@mergify mergify bot merged commit dd0e04a into main Aug 12, 2024
@mergify mergify bot deleted the tomas/ledger-tcp-trans branch August 12, 2024 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@murisi murisi murisi approved these changes

Assignees

No one assigned

Labels

merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tzemanovic @murisi