Skip to content

CompactSeedQR format - #138

Merged
SeedSigner merged 28 commits into
SeedSigner:mainfrom
kdmukai:compact_seed_qr
Feb 18, 2022
Merged

CompactSeedQR format#138
SeedSigner merged 28 commits into
SeedSigner:mainfrom
kdmukai:compact_seed_qr

Conversation

@kdmukai

@kdmukai kdmukai commented Jan 20, 2022

Copy link
Copy Markdown
Contributor

Adds a new CompactSeedQR format which encodes the QR data as bytes in order to fit into a smaller QR code size.

A 24-word seed is encoded as its 256-bit entropy into a 25x25 QR code.
A 12-word seed is encoded as its 128-bit entropy into a 21x21 QR code.

A mnemonic's checksum word is uniquely determined by the full entropy and so it is trivially calculated from this data in order to rebuild the full seed (in other words: it's implicit already in the data and so doesn't need to be stored explicitly).

CHANGES:

  • Requires zbar 0.23 or higher
  • Requires forked update of pyzbar
  • Updates qrcode for more advanced rendering options (CircleModuleDrawer)
  • Adds option screen to select "Standard" SeedQR or the new "Compact" format.
  • Reads the new CompactSeedQR format for 12- or 24-word seeds while maintaining full reverse-compatibility with the "Standard" SeedQR format.
  • Alters QR decoding pipeline to keep data in binary format
  • Renders the zoomed-in SeedQR with dots instead of filled squares
  • Updated SeedQR template using dot targets instead of the mini grid lines.
  • Begins a safer approach to check for a settings.ini field before trying to read it in Settings.__config_to_data but a better overall approach to setting and storing config is probably in order.

Misc:

  • Somewhat ugly workaround to reset the Singleton classes as needed in the test suite.
  • Controller can now be instantiated with disable_hardware so that the test suite can keep re-instantiating the Controller without creating gpio conflicts on the hardware itself.

TODO:

  • Binary download option for zbar?
  • Settings option to enable CompactSeedQR; hide and use standard SeedQR by default
  • Verify that pyzbar/zbar are fine scanning everything with binary=True.
  • Support 12- or 24-word entry for 25x25 SeedQR template generator
  • Documentation for both SeedQR formats
  • Review test suite
  • Add test cases

@newtonick

Copy link
Copy Markdown
Collaborator

@kdmukai can you add "compact_seedqr_enabled = False" to the settings var in tests/test_controller.py

When I ran the test suite it failed because it was expecting compact_seedqr_enabled when performing configparser in the settings class.

--- a/tests/test_controller.py
+++ b/tests/test_controller.py
@@ -33,6 +33,7 @@ def test_singleton_get_instance_preserves_state():
         software = Prompt
         qr_density = 2
         custom_derivation = m/0/0
+        compact_seedqr_enabled = False
     """
     config = configparser.ConfigParser()
     config.read_string(settings)

@kdmukai

kdmukai commented Jan 23, 2022

Copy link
Copy Markdown
Contributor Author

@newtonick ah, dang. I must have made my test suite changes before that Settings change happened. Good catch!

This opened a bit of a can of worms on the testing side that prompted some good improvements.

  • Adding the above to the test suite does fix the test suite, but would be an unresolved bug for any actual users with existing settings.ini files! Added check for the new field and to handle its absence gracefully.
  • The additional test to confirm this behavior introduced a new problem: Re-initializing each Singleton class. pytest cannot isolate them across tests so we have to use a somewhat ugly way to reset them each time.
  • Reseting the Controller means re-mapping(?) the buttons. The second attempt throws a RuntimeError: "Conflicting edge detection already enabled for this GPIO channel". So that required a new disable_hardware on the Controller config.

@kdmukai
kdmukai marked this pull request as ready for review January 23, 2022 16:27
@newtonick

Copy link
Copy Markdown
Collaborator

LGTM. Great work here!

@SeedSigner

Copy link
Copy Markdown
Owner

Appreciate this Keith and thank you for the review Nick!

@SeedSigner
SeedSigner merged commit be70e5a into SeedSigner:main Feb 18, 2022
@kdmukai kdmukai mentioned this pull request Feb 18, 2022
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