Add Stax layouts for generating SSS shares

[aido]

Currently a work in progress while awaiting nbgl_useCaseKeypadDigits() use case (see LedgerHQ/ledger-secure-sdk#601).

[aido]

Hi,

LedgerHQ/speculos#508 is becoming a blocker for this PR.

[tdejoigny-ledger]

@aido still some error in the CI.
If I'm not wrong we already updated the SDK with the change you asked (disable USB flag)

[aido]

Hi @tdejoigny-ledger,

The latest Ledger app builder container uses Nano S SDK version lns-2.1.0-v22.1. This version is from last July and does not include the above mentioned fixes to the SDK. Those fixes were cherry picked into the latest API_LEVEL_LNS but were not tagged with a version to be included in the Ledger app builder container.

[tdejoigny-ledger]

Hi @tdejoigny-ledger,

The latest Ledger app builder container uses Nano S SDK version lns-2.1.0-v22.1. This version is from last July and does not include the above mentioned fixes to the SDK. Those fixes were cherry picked into the latest API_LEVEL_LNS but were not tagged with a version to be included in the Ledger app builder container.

my bad: I forgot to tag the LNS branch for NanoS before publishing the new app builder

[tdejoigny-ledger]

let's wait the new app builder version

[aido]

Hi @amanone ,

This is the PR we discussed on Reddit.

[aido]

Hi @tdejoigny-ledger,

An unsuccessful Nano S build was causing a lot of the automated checks to fail but this should be now remedied in the latest ledger-app-builder.

[aido]

Hi @tdejoigny-ledger and @lpascal-ledger,

All automated checks now pass except one. Annoyingly the "Compilation & test" action consistently fails on every pull request due to some issue uploading to codecov.io. We have discussed this before and tried a few different remedies but never really came up with a proper solution:

#8 (comment)

This strange issue is not related to the app code and should probably not block review or release.

[tdejoigny-ledger]

@aido agree
we just discussed about that with @lpascal-ledger
the codecov part could be probably removed to have a green CI.

[aido]

the codecov part could be probably removed to have a green CI.

Since I don't have the same Codecov.io rate limit as LedgerHQ it works fine on my fork. I like having the coverage stats so may put a condition into the workflow that if my repo upload to codecov.io but if LedgerHQ repo skip.
I'm not yet sure if that's even possible but will investigate for next release. If it's too messy I may just rip it out altogether as you suggest.

[lpascal-ledger]

@aido tried in #13 to update the codecov action version to see if it would be better.
I'm suspecting that somehow the -pretty old- version we're using kinda ignores the token (:shrug:).

[EDIT] Had to trigger the workflow manually, we can follow it here.

[EDIT bis] Well at least this one worked. Can't be sure it will continue to.

[aido]

yeah, strange.

Not that important to worry too much about or put much effort into fixing. I'll do something about it in next version; probably just remove.

EDIT:
@lpascal-ledger A previous comment from you suggested it was related to some rate limit:
Rate limit reached. Please upload with the Codecov repository upload token to resolve issue
This may explain the irregular behaviour i.e. it is ignoring the token as you say and sometimes LedgerHQ has hit this limit and other times we're just lucky that the limit hasn't yet been hit when the jobs run. ... maybe.
I'm not going to worry too much about it.

[lpascal-ledger]

@aido if you accept my PR, we could merge and see if it's robust over time, or just a lucky draw.

[aido]

Agree. There are currently 3 PRs open now, This one, your Codecov fix and a dependabot PR that updates version of action-download-artifact.

I think all three can be merged to make v1.8.0

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 69.62025% with 24 lines in your changes missing coverage. Please review.

Project coverage is 60.04%. Comparing base (aa58ae5) to head (205cca5).

Files with missing lines	Patch %	Lines
src/common/sskr/seed_sskr.c	60.00%	24 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (aa58ae5) and HEAD (205cca5). Click for more details.

HEAD has 17 uploads less than BASE

Flag	BASE (aa58ae5)	HEAD (205cca5)
unittests	18	1

Additional details and impacted files

@@             Coverage Diff              @@
##           develop       #5       +/-   ##
============================================
- Coverage    82.18%   60.04%   -22.14%     
============================================
  Files            5       14        +9     
  Lines          696     1632      +936     
  Branches         0      266      +266     
============================================
+ Hits           572      980      +408     
- Misses         124      612      +488     
- Partials         0       40       +40     

Flag	Coverage Δ
unittests	60.04% <69.62%> (-22.14%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[aido]

Whhhoohhhooo! 🥳
A clean run! Green ticks everywhere!

Thanks @lpascal-ledger. I declare Codecov issue squashed!

[aido]

@lpascal-ledger,

I would also like to mention that this PR now uses the standard app Makefile. I was able to remove a lot of unnecessary DEFINES from the app Makefile as the correct defaults are set in the standard app Makefile. Maybe the same or similar can be done in your recovery check app which this app was originally forked from.

[lpascal-ledger]

Thanks for the feedback @aido ; I think I've done the migration already (although Recovery Check got a bit out of my mind lately).

[tdejoigny-ledger]

good news ! and ty @lpascal-ledger for your help
@aido if i'm not wrong we can consider that this PR is ready to be audited and deployed on a test provider, right ?
cc: @vforgeoux-ledger

[aido]

this PR is ready to be audited and deployed on a test provider

Yes.
One small optional extra though. We could merge the other PR #12 too and have everything in v1.8.0.

#12 doesn't affect the app code though, its just a version change on one of the GitHub actions used. So its your call if you want to merge it now or not

[tdejoigny-ledger]

this PR is ready to be audited and deployed on a test provider

Yes. One small optional extra though. We could merge the other PR #12 too and have everything in v1.8.0.

#12 doesn't affect the app code though, its just a version change on one of the GitHub actions used. So its your call if you want to merge it now or not

ok ty. We will try to find an audit slot (probably around beginning of January)

[aido]

Hi @tdejoigny-ledger,

We will try to find an audit slot (probably around beginning of January)

Just checking in to ask if we are still on track for a January audit?

[tdejoigny-ledger]

Hi @aido

Yes, we are still trying to find a slot for January, but to be honest, it will likely be towards the end of the month.

[aido]

Thanks for the prompt response.
That's good. Hopefully we get a slot in the not too distant future.

Meanwhile I have been quietly beavering away at the BIP85 functionality in the app. I have made good progress and have all the functions written for the BIP85 applications I want to implement (password, BIP39, dice throw etc.). I will soon start work on the Stax and Flex user interface around these functions.