cryptoadvance / specter-diy

DIY airgapped hardware wallet that uses QR codes for communication with the host
MIT License
441 stars 73 forks source link

qr scanner: animated qr #53

Closed gorazdko closed 4 years ago

gorazdko commented 4 years ago

Abstract

Close #47

Status

QRScanner class should be able to detect these animated QR codes and continue scanning until we got all the codes

Currently, the user must press the button when he is ready to scan the next code. Is it possible to achieve this without a button, automatically?

stepansnigirev commented 4 years ago

Great! I will test it tonight. We can trigger the scanner again if the data we received is not complete or has a flag that more data is coming (we can use the last few characters for this purpose) I think the QR scanner we use has a config to ignore the same QR code on the next scan, I need to check. But relying on this config would break compatibility with other QR scanners. I think we can still use it but also check if incoming data is the same as before and ignore it if so.

I would suggest to use last 3 characters of the QR code to flag more data. First character - a flag itself, then a byte with the index of the current code, and lastly total number of codes.

As a flag we should use some character that is not in base58, bech32 or base64 sets. Maybe &?

stepansnigirev commented 4 years ago

Another thing, if we go with flags we should include this functionality in the qrscanner.py. In this case main logic doesn't need to care about partitioned data - it always gets full data at the end of all scans. Current counter and total counter should be available from the main as we may want to show the user some kind of progress bar.

stepansnigirev commented 4 years ago

Sorry, misclicked....

gorazdko commented 4 years ago

Another thing, if we go with flags we should include this functionality in the qrscanner.py. In this case main logic doesn't need to care about partitioned data - it always gets full data at the end of all scans

This is what this PR does - as the #47 describes. The difference is we put the flag into header not in tail: "part1of3 payload"

I think the QR scanner we use has a config to ignore the same QR code on the next scan

ok, this allows us to skip the button then. I'll check the datasheet...

stepansnigirev commented 4 years ago

Checked. Works nicely with transaction signing. Breaks other functions like scan new wallet or verify address. I think if we throw an error from qr scanner and @catchit in update function instead of sending it to the callback we should be fine. Alternatively, we can set a constant error callback for qr scanner in the constructor and if an error occurs - call that one instead. It should cancel current scan and show error message.

gorazdko commented 4 years ago

I still have to test this more tomorrow and will let you know when its ready...

gorazdko commented 4 years ago

Alternatively, we can set a constant error callback for qr scanner in the constructor and if an error occurs - call that one instead. It should cancel current scan and show error message.

This approach is better as i get to clean the state easier after the exception. However, i had problems instantiating the object qr_scanner = QRScanner(cb_error=qr_scanner_error) this way, so i decided to pass it to update function: qr_scanner.update(qr_scanner_error)

Let me know what you think. Otherwise i tested and it seems to work ok

I think the QR scanner we use has a config to ignore the same QR code on the next scan

i couldnt find it in the datasheet. One solution would be for software to check if the new scan is identical to previous scan and ignore and trigger a new scan. For this we would have to disable the beep sound, otherwise it would be bad UX. I would leave this feature for another issue

stepansnigirev commented 4 years ago

so i decided to pass it to update function: qr_scanner.update(qr_scanner_error)

Sounds good to me. We know current state of the app and how to handle error properly when we call qr_scanner.update(), so it's even more logical than using it in the constructor. qr_scanner.start() might also be good for this purpose, but if update works - it's all we need for now.

i couldnt find it in the datasheet. One solution would be for software to check if the new scan is identical to previous scan and ignore and trigger a new scan. For this we would have to disable the beep sound, otherwise it would be bad UX. I would leave this feature for another issue

Yes, let's leave it for later. I think clicking a button for the next scan is not such a big deal, and generally with all the psbt optimizations we did recently number of required QR codes will be pretty small even for huge transactions or wallet descriptors.

What about the simulator? Does it work there as well? You mentioned that there was some problem.

gorazdko commented 4 years ago

I have tested on stm32 and unix platform. This is one test example i used on both platforms: test_data.zip

gorazdko commented 4 years ago

Force pushed the changes. I also tested signing a transaction and adding a wallet on stm32 and unix platform

stepansnigirev commented 4 years ago

Awesome! Thanks! Merged.