Ledger Review Fixes

[chris124567]

WIP PR to house fixes in response to Ledger's review.

Currently have added NanoSP support, added confirmation/cancellation status screen when generating address/pubkey, and adds developer information to the about page in the app. The issue regarding transaction signing on Stax in siacentral web wallet still needs to be resolved.

[chris124567]

format_hex returns uppercase hex which is not what users are used to and makes it different to visually compare things like addresses.

I was unable to get snprintf to work (always returned 0 and no error emitted with HAVE_SNPRINTF_DEBUG) but looking at the code anyways it doesn't seem like it supports uint64s: https://github.com/LedgerHQ/nanos-secure-sdk/blob/0532bf20fbbb11dd08dada62060f8337097b6078/src/os_printf.c#L572 as there is no unsigned long long / 'l' formatter.

Re: glyphs, icon_validate is used in the UI for txn signing on Nano S / Nano X:

UX_STEP_VALID(ux_sign_txn_flow_2_step,
              pb,
              io_seproxyhal_touch_txn_hash_ok(),
              {&C_icon_validate, "Approve"});

Everything else I can fix.

[xchapron-ledger]

format_hex returns uppercase hex which is not what users are used to and makes it different to visually compare things like addresses.

True, we should add a lower case version in the SDK someday...

looking at the code anyways it doesn't seem like it supports uint64s

Indeed, I missed that you where looking to display big numbers like this. Though I'm still not sure this is really needed?

Re: glyphs, icon_validate is used in the UI for txn signing on Nano S / Nano X:

You could use C_icon_validate_14 from the SDK instead: https://github.com/LedgerHQ/ledger-secure-sdk/blob/master/lib_ux/glyphs/icon_validate_14.gif

[chris124567]

The first failure is because the version of clang-format I have locally formats slightly different for some reason. The second is caused by what appears to be invisible rendering differences on the nanosp (maybe I need to update speculos?). I'll investigate that more.

Other than maybe trying to get snprintf to work, these are the main remaining things I need to do:

It would be better to stop using TRY / CATCH / THROW mechanism, and replace it with error cascading or LEDGER_ASSERT() when the error doesn't need to be cascaded. It helps the readability of the app error handling flow and would remove some magical things from sia_main().
There is no public key comparison screen on Stax, so the behavior is different, though this is usually not how we do pk comparison on our Apps but it can depend on software wallet implementation I guess.
The sign hash UI is probably not compliant on Stax, you should have a hold to sign.
It would be great if the tests include a transaction signature with more fields, which justify the need for streaming and custom NBGL usage.

[xchapron-ledger]

The first failure is because the version of clang-format I have locally formats slightly different for some reason.

You can probably update the version either on your setup on the CI workflow file?

The second is caused by what appears to be invisible rendering differences on the nanosp (maybe I need to update speculos?)

This is probably because on your setup your are generation snapshot for NanoSP 1.1.0. There is a new version of the SDK / ledger-app-builder docker image which support NanoSP 1.1.1 with very small font changes.

[chris124567]

Do you know what this build error is caused by? It's failing to remove build artifacts before upload and I've never seen that error before. All I changed was code formatting and fixed the test images for the NanoSP. There was a change in https://github.com/LedgerHQ/ledger-app-workflows/commits/master/ earlier today but it has to do with Rust and shouldn't affect this.

[xchapron-ledger]

Do you know what this build error is caused by? It's failing to remove build artifacts before upload and I've never seen that error before. All I changed was code formatting and fixed the test images for the NanoSP. There was a change in https://github.com/LedgerHQ/ledger-app-workflows/commits/master/ earlier today but it has to do with Rust and shouldn't affect this.

Humf, that's our fault, I'm on it.

[xchapron-ledger]

@chris124567 Thanks for the notice, should be good now (I think you might need to relaunch the whole CI jobs, not just failings ones). Sorry for the issue :/

[chris124567]

Sorry for the delays... one of my lungs spontaneously collapsed. I should be able to work on these issues over the next couple weeks but may be a bit slower because it will take a while for me to get back to 100%. Then we can do the app update submission.

Still need to remove exceptions from transaction parsing and a couple other things.

[chris124567]

OK, on second thought, I'm not sure removing TRY/CATCH from txn.c actually improves readability. I would say removing it improved things in the other files but I'm not really seeing the benefits in this case. I started writing code to do it but it's starting to get ugly. We'd basically have to store an error status in the txn_state_t object and check it in a number of locations, which for instance would make every call to need_at_least require a corresponding check (every function that calls those functions need would also need to check the status code), and it would require passing the transaction state to functions that don't operate on it directly like cur2dec or changing the return value semantics in those cases.

[xchapron-ledger]

OK, on second thought, I'm not sure removing TRY/CATCH from txn.c actually improves readability. I would say removing it improved things in the other files but I'm not really seeing the benefits in this case. I started writing code to do it but it's starting to get ugly. We'd basically have to store an error status in the txn_state_t object and check it in a number of locations, which for instance would make every call to need_at_least require a corresponding check (every function that calls those functions need would also need to check the status code), and it would require passing the transaction state to functions that don't operate on it directly like cur2dec or changing the return value semantics in those cases.

Hello, are you aware of the recently introduced LEDGER_ASSERT() macro? You can learn more about it here: https://github.com/LedgerHQ/ledger-secure-sdk/blob/master/include/ledger_assert.h
Basically it's a non catch-able throw.
It can be used for error that shouldn't occurs under nominal conditions. That might apply to the cur2dec checks?

For the one in need_at_least I agree the alternative is a bit verbose...
You could change need_at_least and calling functions (seek, readInt, ..., __txn_next_elem) return type from void to txnDecoderState_e and use a macro like:

#define TXN_CHECK(call)                 \
    do {                                \
        txnDecoderState_e error = call; \
        if (error) {                    \
            return error;               \
        }                               \
    } while (0)

But does that make the code easier to follow than the current version, that's up to you! I'm not sure if I would have done it or not.

[chris124567]

I realized there actually is a public key confirmation page on Stax. You can see it in the snapshots folder under ./tests/snapshots/stax/test_get_public_key_confirm_accepted or for addresses ./tests/snapshots/stax/test_get_address_confirm_accepted. To be honest, I think modifying the transaction handling code is likely to introduce issues especially due to the number of places we'd have to add these checks.

Do you think we are OK to do the app submission now?

[xchapron-ledger]

I have two additional comments:

• Some comments from the code could be removed I guess.. They are from an old time and are probably at least not completely accurate.
• We merge this recently: nbgl: Add force_page_start to nbgl_layoutTagValue_t LedgerHQ/ledger-secure-sdk#520. This is not available yet, but I think this should be used to simplify the calctxnHash_nbgl.c code. The app should not be responsible of page formatting on NBGL.

Otherwise, kudos for the work of bringing this app to the new standard! IMO the code quality have greatly improved.