search

cardano-foundation / ledger-app-cardano

Cardano Ledger App for Ledger Nano S
Apache License 2.0
26 stars 14 forks source link

Fix headless mode crashing in Nano X #8

Closed refi93 closed 5 years ago

refi93 commented 5 years ago

I found out, that the UX_STEP_CB macro used in our confirm screens actually sets the internal G_ux.stack[0].ticker_callback, which ends up overriding our callback in the UX_TICKER_EVENT macro. The thing is that UX_STEP_CB sets that callback with timeout 0ms (ux_flow_engine, line 125, sdk nanox dev6), so it isn't actually executed, so neither it isn't cleared from the stack (not sure if it is a bug or feature).

The newly introduced UX_STEP_TIMEOUT macro does not seem to suit well our needs since it seems to be meant as a separate ux flow with autoconfirm after a certain timeout, it cannot override the behavior of an existing flow like we want in headless mode.

UX_STEP_TIMEOUT is therefore effectively disabled by this PR and I added to the code a warning about that. Other functions in the SDK do not seem to actually use the G_ux.stack[].ticker_callback so we should be ok.

I believe that we can refactor headless mode to be impelemented through UX_STEP_TIMEOUT by "ifdefing" all the flows we have to be UX_STEP_TIMEOUT in headless mode, but it makes sense to postpone that until Nano S UI API is aligned with Nano X.

refi93 commented 5 years ago

a simple workaround could be replacing for Nano X the set_timer calls with

    UX_CALLBACK_SET_INTERVAL(HEADLESS_DELAY)

The UX_CALLBACK_SET_INTERVAL would set the internal G_ux.stack[].ticker_value to a value greater than zero, which would trigger the "autoconfirm" callback internally set by UX_STEP_CB and UX_STEP_NOCB, but I'm not sure if this is desired since it would not call our own headless callbacks and therefore skip the asserts we have there. But I guess the additional asserts in the headless callbacks won't be really needed if we rely on the integrity of the SDK.

ppershing commented 5 years ago

We decided to go with an alternate fix implementation