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

add --nfc flag to use NFC apdu media #519

Merged
merged 6 commits into from
Nov 25, 2024
Merged

Conversation

yrichard-ledger
Copy link
Contributor

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2024

Codecov Report

Attention: Patch coverage is 39.71292% with 126 lines in your changes missing coverage. Please review.

Project coverage is 43.86%. Comparing base (9913397) to head (6518a6d).

Files with missing lines Patch % Lines
speculos/mcu/transport/usb.py 39.25% 65 Missing ⚠️
speculos/mcu/transport/nfc.py 28.26% 33 Missing ⚠️
speculos/mcu/seproxyhal.py 23.07% 10 Missing ⚠️
speculos/mcu/transport/interface.py 73.07% 7 Missing ⚠️
speculos/main.py 0.00% 5 Missing ⚠️
speculos/mcu/transport/__init__.py 54.54% 5 Missing ⚠️
src/bolos/os.c 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #519   +/-   ##
=======================================
  Coverage   43.85%   43.86%           
=======================================
  Files         122      125    +3     
  Lines       12358    12448   +90     
  Branches      993      993           
=======================================
+ Hits         5420     5460   +40     
- Misses       6603     6653   +50     
  Partials      335      335           

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


🚨 Try these New Features:

@lpascal-ledger
Copy link
Contributor

Some comments on my latest changes:

  • Added the NFC flag in Speculos OS settings on Flex and Stax. We could add an argument to be able to configure it at runtime, but I don't have clear use case for this and didn't though it was worth it for now.
  • As USB and NFC are exclusive (Speculos is started with NFC xor HID xor U2F communication), there is little point instantiating and managing a .usb and a .nfc variable. As they share similar behavior (with only different names), I merged them under the same TransportLayer mother class, and instantiate it from a factory function build_transport.
  • HID and U2F classes were managed through composition in USB, which led to callback injection, backward dependencies (HID and U2F inherit from a Transport class, but are instrumented through a USB(TransportLayer) class (which is no transport) and weird interface (the factory function knows the transport - HID, U2F, NFC- but only deals with NFC and "USB", which itself manages HID and U2F).
    Now HID and U2F are (inheritance) USBTransport, which is (inheritance) itself a TransportLayer, just like NFC is also a TransportLayer. All can be easily instantiated into the factory function.
  • As --nfc and --usb argument could obstruct each other, and --usb feels deprecated now that NFC is available (and maybe bluetooth someday?), I rather added a --transport argument superseeding the --usb one, which is now flagged as deprecated.

@lpascal-ledger lpascal-ledger merged commit 4e6d6c2 into master Nov 25, 2024
38 checks passed
@lpascal-ledger lpascal-ledger deleted the support_nfc_apdu branch November 25, 2024 08:20
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