Fix confirmation UI never displaying

[Zosoled]

Checklist

• App update process has been followed
• Target branch is develop
• Application version has been bumped

Problem

The confirmation UI for signing blocks is not being displayed, and the app is getting stuck in LIBN_STATE_CONFIRM_SIGNATURE which blocks subsequent commands from executing.

Cause

The conditional guard in app_apply_state() is checking io_seproxyhal_spi_is_status_sent(). When libn_apdu_sign_block() sets the state to LIBN_STATE_CONFIRM_SIGNATURE and calls app_apply_state(), this guard returns true i.e. a status is pending. This prevents the function from moving on to displaying the confirmation UI using libn_bagl_apply_state(). Further calls to app_apply_state() make no difference since io_seproxyhal_spi_is_status_sent() continues to block the UI and leaves the state machine stuck.

Solution

Remove the io_seproxyhal_spi_is_status_sent() check from the guard condition. UX_DISPLAYED() is sufficient to know that the display is ready.

[tdejoigny-ledger]

@Zosoled could you please rebase this PR and add this one here
#13

[tdejoigny-ledger]

@Zosoled not this change already merged
https://github.com/LedgerHQ/app-nano/pull/14/files

[tdejoigny-ledger]

this one
#13

[Zosoled]

I rebased onto develop, that's why #14 was pulled in.

[tdejoigny-ledger]

your rebase seems wrong

[Zosoled]

@tdejoigny-ledger I applied more fixes to resolve various other build errors and warnings, and they all appear to be resolved on my end. Can you check this latest version please?

EDIT: I included #13 in this too, as requested.

[tdejoigny-ledger]

As you can see you have to rebase your PR from develop branch

[tdejoigny-ledger]

And I forgot to mention also that you need to include the mandatory CI : the reusable build part included in this CI
https://github.com/LedgerHQ/app-boilerplate/blob/master/.github/workflows/build_and_functional_tests.yml

[Zosoled]

Got it, thanks. No longer showing as behind any commits:

Added the workflow you mentioned too, and it looks to have run OK on my fork:

If there is anything else required, let me know.

[tdejoigny-ledger]

@Zosoled almost done ;-)
As I said you have to keep only the reusable build part in the CI because you don't have ragger tests
As there are no tests, did you try to sideload the app and to test it ?
BTW there are still few remaining warnings in the static analysis.

[Zosoled]

Ah, ok my mistake. Yes, I'm sideloading and successfully deriving and signing. Odd though, I didn't see any more messages in the static analysis; do all warnings need to be cleared in order for the updated app to be deployed, or just errors?

[tdejoigny-ledger]

@Zosoled normally the CI shall be green but I guess that for this time we can have some flexibility here

[Zosoled]

@tdejoigny-ledger CI checks all look green on my end. Please review at your earliest convenience.

[tdejoigny-ledger]

@Zosoled great job ! CI is 🟢
A last thing (not mandatory) : It would probably be good to do a cleanup of the commits by merging some of them (lint, for example, fix of the analysis static in an other, ...). There are a lot of commits, and for the history it could be a bit complicated.

[Zosoled]

@tdejoigny-ledger Great, thanks! I have done so, with this PR's original intended bugfix applied last after all the other cleanup.

I also chose to bump to minor version 1.3.0 since there were so many other changes required to get this app up to speed with current requirements. If that's incorrect, let me know and I can revert to using a patch version of 1.2.9 instead.

[tdejoigny-ledger]

@Zosoled commits cleanup 👍

Sorry I have a last batch of comments about makefile and we should be ready to deploy the app

#HAVE_APPLICATION_FLAG_GLOBAL_PIN = 1 disabled and calling os_global_pin_is_validated() seem incompatible. I’d be surprised if that works on a real device

this part could be probably removed as in boilerplate app

CC       := $(CLANGPATH)clang

#CFLAGS   += -O0
CFLAGS   += -O3 -Os -Wno-typedef-redefinition

AS     := $(GCCPATH)arm-none-eabi-gcc

LD       := $(GCCPATH)arm-none-eabi-gcc
LDFLAGS  += -O3 -Os
LDLIBS   += -lm -lgcc -lc

What is the shared mode ?

    ifeq ($(APP_TYPE), shared)
        APP_LOAD_PARAMS += $(ALL_PATH_PARAMS)
        HAVE_APPLICATION_FLAG_LIBRARY = 1
        DEFINES += IS_SHARED_LIBRARY

The app doesn’t support swap, so I don’t see why we’d need to be able to libcall it.

[Zosoled]

I am just a community member who decided to contribute to fix this app since the original dev is gone, so I will answer the best I can.

1. include/appflags.h does not have documentation for the flag HAVE_APPLICATION_FLAG_GLOBAL_PIN, so I do not know its purpose; if you mean you'd be surprised if the app works as a whole, the syscall has been there for a long time and the app has worked for countless transactions by real-world users, so I'm not sure if the call is just skipped or what. This call was part of the most recent updates for the APDU implementations (e95407fcd0b578ed69095343d670d25b720c2842 and 3c1307371167260685666dc69329f9b1bf1f7555) which were 5 years ago. Please advise.
2. OK, I have removed that code since I see that compiler options are set in Makefile.defines which is included in Makefile.standard_app. (The custom compiler options were introduced in the original makefile 8 years ago when I believe Makefile.standard_app et al. did not exist).
3. Nano is used as the basis of two other currencies: meme coin Banano $BAN and the now-defunct stablecoin Nollar $NOS. They are forks of Nano, just on different BIP-44 paths, so its code is used for derivation and signing. The original commit that introduced this appears to be 79f8eab720c0f82802dc435618ff0e95072751fb. If the lib code is unnecessary for variants, then I can look into removing it, but this PR has crept up in scope a lot already, so if it can be left alone then I think it should be.

[tdejoigny-ledger]

@Zosoled

1. • if it's working on your side on real device ok for me
2. • ty for the update
3. • this one is a bit annoying and this extra permission should be removed

for the next PR:
could be interesting to add some test (we have a dedicated framework for that).

[Zosoled]

@tdejoigny-ledger It looks like removing that lib permission also changes the --appFlags for every variant; they were previously 0x800 (0xa00 for "Nano") but now with the removal of 0x800 APPLICATION_FLAG_LIBRARY they are 0x0 (0x200 for "Nano"). Is this now blocked until a PR to update the flags in LedgerHQ/ledger-app-database can be merged?

[tdejoigny-ledger]

@Zosoled yes you need to open a PR for the app database
LedgerHQ/ledger-app-database#427

After that it should be good 👍

[Zosoled]

@tdejoigny-ledger Done.

LedgerHQ/ledger-app-database#428

[tdejoigny-ledger]

@Zosoled LGTM. I will launch the app deployment next Wednesday.