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.

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

[nano] remove blind large tx option #297

Closed

Conversation

spalmer25
Copy link
Collaborator

As followed a discussion with ledger:
This PR

  • use the behaviour of Large tx blindsign option for the ON blindsigning option
  • remove the Large tx blindsign option

@spalmer25 spalmer25 self-assigned this Sep 30, 2024
@spalmer25 spalmer25 requested review from ajinkyaraj-23 and removed request for ajinkyaraj-23 September 30, 2024 12:31
@spalmer25 spalmer25 force-pushed the palmer@functori@nano-remove-blind-large-tx-option branch from 4165523 to 06e3b2e Compare September 30, 2024 12:47
 - OFF is the default value
Redundant with test_sign_too_long_operation tests
@spalmer25 spalmer25 force-pushed the palmer@functori@nano-remove-blind-large-tx-option branch from 06e3b2e to b73908b Compare September 30, 2024 12:48
@spalmer25 spalmer25 marked this pull request as ready for review September 30, 2024 12:49
@@ -63,9 +63,14 @@ typedef enum {
} main_step_t;

typedef enum {
#ifdef HAVE_BAGL
Copy link
Collaborator

Choose a reason for hiding this comment

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

use bool. there are only two settings so enum does not make sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 I will change it when I will apply my commits in your PR

@@ -76,11 +81,13 @@ typedef enum {
#define NB_MAX_SCREEN_ALLOWED 8
#endif

#ifdef HAVE_NBGL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure you dont need the reason,
You have to show amount, fees, hash in case of large_tx but only hash in case of parsing error. How do you differenciate the two cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The two flow are different, so no need to differentiate the case later:

  • On parsing error the screen are directly displayed then the flow displayed is the blindsigning flow.
  • On too-large-operation the flow is the summary flow (finish the parsing before display warning screen)

@@ -1,238 +0,0 @@
#!/usr/bin/env python3
Copy link
Collaborator

Choose a reason for hiding this comment

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

where is the test to verify blindsign on behaviour now? for example for screens greater 12/20 for nanox/nanosp, also what happens when parsing error occurs after this limit when blindsigning is on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since the blindsigning flow as been replaced by the too-large-tx flow, the tests regarding blindsigning are for parsing_error, see:

@spalmer25
Copy link
Collaborator Author

The commits of this PR have been merged to the PR #296

@spalmer25 spalmer25 closed this Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants