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

Use previous Ledger app version and re-enable Ledger signer tests #1487

Merged

Conversation

franciszekjob
Copy link
Collaborator

Closes #

Introduced changes

Ledger Starknet app latest version has changed APDU command. It doesn't support e.g. signing deploy account transaction) which is used in our tests. Due to it, we use previous version.
Once future release of Starknet app will support features used in out tests, updating versions and tests should be addressed in #1473.

  • This PR contains breaking changes

@franciszekjob franciszekjob changed the title Re-enable Ledger signer tests Use previous Ledger app version; Re-enable Ledger signer tests Sep 11, 2024
@franciszekjob franciszekjob changed the title Use previous Ledger app version; Re-enable Ledger signer tests Use previous Ledger app version and re-enable Ledger signer tests Sep 11, 2024
Copy link
Collaborator

@ddoktorski ddoktorski left a comment

Choose a reason for hiding this comment

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

Shouldn't the documentation be updated to mention that the latest starknet app version is not supported?

@franciszekjob
Copy link
Collaborator Author

franciszekjob commented Sep 11, 2024

I think it's better if we include information that our interface doesn't allow clear-signing transactions yet, instead it allows to blind-sign. This is more better information for an end-user imo. Wdyt @ddoktorski ?

@franciszekjob
Copy link
Collaborator Author

franciszekjob commented Sep 12, 2024

offline discussion: We will add info about the app's version, as it's visible when installing on Ledger device so it's worth to include it.

@franciszekjob franciszekjob merged commit d972124 into development Sep 12, 2024
10 checks passed
@franciszekjob franciszekjob deleted the franciszekjob/re-enable-ledger-signer-tests branch September 12, 2024 09:04
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