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

tests: add bip25519 derivation in integration tests #106

Merged
merged 1 commit into from
Nov 1, 2023
Merged

tests: add bip25519 derivation in integration tests #106

merged 1 commit into from
Nov 1, 2023

Conversation

ajinkyaraj-23
Copy link
Collaborator

@ajinkyaraj-23 ajinkyaraj-23 commented Oct 31, 2023 •
edited by emturner
Loading

fixes #53

@ajinkyaraj-23 ajinkyaraj-23 force-pushed the ajinkyaraj-23@test_derivations_2 branch from 9fa6ab6 to 319f13c Compare October 31, 2023 13:27
@ajinkyaraj-23 ajinkyaraj-23 force-pushed the ajinkyaraj-23@test_derivations_2 branch 2 times, most recently from a8efd69 to 6d9d597 Compare October 31, 2023 16:08
@ajinkyaraj-23 ajinkyaraj-23 linked an issue Oct 31, 2023 that may be closed by this pull request
Test: ensure compatibility for all derivation paths between old/new app #53
Closed
@ajinkyaraj-23 ajinkyaraj-23 force-pushed the ajinkyaraj-23@test_derivations_2 branch 3 times, most recently from e5cdb04 to 3696b87 Compare November 1, 2023 10:22
@emturner emturner added app::wallet issues relating to the wallet app tests test - related isues labels Nov 1, 2023
@ajinkyaraj-23 ajinkyaraj-23 linked an issue Nov 1, 2023 that may be closed by this pull request
Tests: ensure bip25519 derivation included in generated tests #56
Open
@ajinkyaraj-23 ajinkyaraj-23 changed the title Adding bip25519 derivation in integration tests tests: add bip25519 derivation in integration tests Nov 1, 2023
emturner
emturner previously approved these changes Nov 1, 2023
View reviewed changes
Copy link
Collaborator

@emturner emturner left a comment

Choose a reason for hiding this comment

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

overall looks pretty good I think - couple minor comments.

Would be great to add a comment at the top of each bip25519 test explaining why the increased RETRIES is needed?

@emturner emturner self-requested a review November 1, 2023 10:56
@ajinkyaraj-23 ajinkyaraj-23 force-pushed the ajinkyaraj-23@test_derivations_2 branch 3 times, most recently from a6c5e14 to bf6508a Compare November 1, 2023 11:18
@emturner emturner dismissed their stale review November 1, 2023 12:12

didn't mean to approve

emturner
emturner reviewed Nov 1, 2023
View reviewed changes
tests/integration/nano/test_provide_tz1_bip25519_pk.sh Outdated
press_button right
expected_accept
press_button both
RETRIES=100 expect_apdu_return 210293c6b359964a4332bf1355579d665b753343f7b0a42567978cea1671f7b89f479000
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
RETRIES=100 expect_apdu_return 210293c6b359964a4332bf1355579d665b753343f7b0a42567978cea1671f7b89f479000
TIMEOUT=100 expect_apdu_return 210293c6b359964a4332bf1355579d665b753343f7b0a42567978cea1671f7b89f479000

@ajinkyaraj-23 ajinkyaraj-23 force-pushed the ajinkyaraj-23@test_derivations_2 branch from bf6508a to abbff67 Compare November 1, 2023 13:36
@ajinkyaraj-23 ajinkyaraj-23 force-pushed the ajinkyaraj-23@test_derivations_2 branch from abbff67 to 600881c Compare November 1, 2023 14:24
@emturner
Copy link
Collaborator

emturner commented Nov 1, 2023

One thing spotted: we derive pk twice in the prompt flow!

@emturner emturner mentioned this pull request Nov 1, 2023
Closed
@ajinkyaraj-23 ajinkyaraj-23 merged commit dca8aa6 into main Nov 1, 2023
55 checks passed
@ajinkyaraj-23 ajinkyaraj-23 deleted the ajinkyaraj-23@test_derivations_2 branch November 1, 2023 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emturner emturner emturner approved these changes

Assignees

@ajinkyaraj-23 ajinkyaraj-23

Labels
app::wallet issues relating to the wallet app tests test - related isues
Projects
None yet
Milestone
Wallet: v3.0.3 - security audit
2 participants
@ajinkyaraj-23 @emturner