Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ledger assert #473

Merged
merged 1 commit into from
Dec 4, 2023
Merged

Ledger assert #473

merged 1 commit into from
Dec 4, 2023

Conversation

sgliner-ledger
Copy link
Contributor

Description

Add legder-assert and related defines to enable feature and display of error code on device screen.

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)
  • Tests
  • Documentation
  • Other (for changes that might not fit in any category)

Breaking changes

Replace HAVE_DEBUG_THROWS with HAVE_LEDGER_ASSERT_DISPLAY

@codecov-commenter
Copy link

codecov-commenter commented Nov 27, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (47af250) 59.44% compared to head (9ae87d9) 59.44%.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master     #473   +/-   ##
=======================================
  Coverage   59.44%   59.44%           
=======================================
  Files          12       12           
  Lines        1684     1684           
=======================================
  Hits         1001     1001           
  Misses        683      683
Flag Coverage Δ
unittests 59.44% <ø> (ø)

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.

@sgliner-ledger sgliner-ledger force-pushed the ledger-assert branch 3 times, most recently from 8986bc3 to 45ddf9c Compare November 27, 2023 09:46
xchapron-ledger
xchapron-ledger reviewed Nov 27, 2023
View reviewed changes
src/os.c Outdated Show resolved Hide resolved
src/os.c Show resolved Hide resolved
lib_standard_app/debug.c Outdated Show resolved Hide resolved
@sgliner-ledger sgliner-ledger force-pushed the ledger-assert branch 3 times, most recently from ae6e659 to e466fa4 Compare November 28, 2023 14:25
xchapron-ledger
xchapron-ledger reviewed Nov 29, 2023
View reviewed changes
include/os.h Outdated Show resolved Hide resolved
xchapron-ledger
xchapron-ledger reviewed Nov 29, 2023
View reviewed changes
src/os.c Outdated Show resolved Hide resolved
xchapron-ledger
xchapron-ledger reviewed Nov 29, 2023
View reviewed changes
Copy link
Contributor

@xchapron-ledger xchapron-ledger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should put this out of standard_app? So that it is used more simply on app not using standard_app?

xchapron-ledger
xchapron-ledger reviewed Nov 29, 2023
View reviewed changes
Copy link
Contributor

@xchapron-ledger xchapron-ledger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should allow a potential app callback before displaying the message / exiting the app? something to send back an APDU response in case there was an APDU waiting for a response?
I'd say this could be transparent in standard_app, with a callback in io.c that would respond a standard ASSERT error code + infos?
Or maybe let's keep that for another time. Software wallet probably doesn't need a gentle response when thing goes as bad as an assert is triggered.

@xchapron-ledger xchapron-ledger mentioned this pull request Nov 29, 2023
Merged
xchapron-ledger
xchapron-ledger reviewed Nov 30, 2023
View reviewed changes
Makefile.standard_app Outdated Show resolved Hide resolved
src/os.c Outdated Show resolved Hide resolved
include/ledger_assert.h Outdated Show resolved Hide resolved
include/ledger_assert.h Outdated Show resolved Hide resolved
src/ledger_assert.c Show resolved Hide resolved
@sgliner-ledger sgliner-ledger force-pushed the ledger-assert branch 3 times, most recently from 73569fd to b9ea9f5 Compare November 30, 2023 17:05
xchapron-ledger
xchapron-ledger reviewed Dec 1, 2023
View reviewed changes
include/ledger_assert.h Outdated Show resolved Hide resolved
include/ledger_assert.h Outdated Show resolved Hide resolved
include/ledger_assert.h Outdated Show resolved Hide resolved
include/ledger_assert.h Outdated Show resolved Hide resolved
lib_standard_app/main.c Outdated Show resolved Hide resolved
src/ledger_assert.c Outdated Show resolved Hide resolved
@sgliner-ledger sgliner-ledger force-pushed the ledger-assert branch 3 times, most recently from b334455 to 58078bb Compare December 1, 2023 15:35
xchapron-ledger
xchapron-ledger reviewed Dec 4, 2023
View reviewed changes
src/ledger_assert.c Outdated
#include <string.h>

#include "ledger_assert.h"
#include "exceptions.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary?

@sgliner-ledger sgliner-ledger force-pushed the ledger-assert branch 3 times, most recently from 9448536 to 9adfe8e Compare December 4, 2023 15:47
@sgliner-ledger sgliner-ledger merged commit efa7b8b into master Dec 4, 2023
158 checks passed
@sgliner-ledger sgliner-ledger deleted the ledger-assert branch December 4, 2023 16:03
This was referenced Dec 5, 2023
Merged
Merged
Merged
This was referenced Dec 5, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xchapron-ledger xchapron-ledger xchapron-ledger approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sgliner-ledger @codecov-commenter @xchapron-ledger