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

Ledger/Speculos TCP APDU transport #288

Merged
merged 3 commits into from
Mar 15, 2023
Merged

Ledger/Speculos TCP APDU transport #288

merged 3 commits into from
Mar 15, 2023

Conversation

e-asphyx
Copy link
Contributor

@e-asphyx e-asphyx commented Feb 3, 2023

No description provided.

@e-asphyx e-asphyx requested a review from jevonearth February 3, 2023 17:29
@e-asphyx e-asphyx self-assigned this Feb 3, 2023
@codeclimate
Copy link

codeclimate bot commented Feb 3, 2023

Code Climate has analyzed commit 72bca5e and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 47.1% (0.0% change).

View more on Code Climate.

@cloudflare-workers-and-pages
Copy link

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 72bca5e
Status: ✅  Deploy successful!
Preview URL: https://b3069298.signatory.pages.dev
Branch Preview URL: https://tcp-apdu-transport.signatory.pages.dev

View logs

Copy link
Contributor

@stephengaudet stephengaudet left a comment

Choose a reason for hiding this comment

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

on a high level, the documentation is accurate and the behaviour is correct. thank you for the speculos backend support @e-asphyx !

just wanted to double check that the removal of BlockSize from the devices' infos was an intentional change and not by mistake, seems unrelated to the PR. tested on Ledger X, S, and SP.

my first look at the code base, so, please take my comments with a grain of salt. passing around the ctx context as a parameter maybe a classic situation for dependency injection.

lastly, I just noticed, there is a failing check, code climate. max number of returns exceeded

pkg/vault/ledger/ledger/devices.go Show resolved Hide resolved
@stephengaudet stephengaudet self-requested a review February 4, 2023 02:04
@stephengaudet stephengaudet merged commit 36138c3 into main Mar 15, 2023
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.

3 participants