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

Replace PR #184: Validate Generated Addresses Against Expected Ones in JS Client Library #189

Merged
merged 4 commits into from
Jul 28, 2023

Conversation

landabaso
Copy link
Contributor

@landabaso landabaso commented Jul 26, 2023

This PR replaces the earlier PR #184, which had issues due to a faulty rebase operation. Like the previous one, it aims to address issue #171 by implementing an enhanced validation method in the JavaScript client library. It validates the generated wallet addresses against the expected ones using the "@bitcoinerlab/descriptors" library. Here are the key updates:

  1. Updated package.json to incorporate new dependencies "@bitcoinerlab/descriptors" and "@bitcoinerlab/secp256k1", and the latest version of "bitcoinjs-lib" to "^6.1.3".

  2. Introduced a new test in appClient.test.ts to validate the generated address or throw an error if it doesn't match the expected one.

  3. Made modifications in appClient.ts to integrate new imports. Changes have been made to the AppClient class to use these libraries for address validation. Some previous validations were removed as they are now handled more effectively with the new approach.

In response to the review comments on the previous PR, I've:

  • Bumped the package version to 0.2.3.
  • Added address validation at the end of registerWallet.
  • Completely replaced validatePolicy with validateAddress and removed containsA.
  • Adjusted the code for correctly parsing the <M;N> format. Included a test case using this format for better clarity.

For further context and details, please refer to the discussion and changes in the previous PR #184.

… end of registerWallet; validateAddress replaces validatePolicy; bumped package version; correctly deal with <M;N> format (added a test using this format); Fixed incomplete keys in registerWallet test
Revised the key replacement logic in the `validateAddress` function to prevent misinterpretation of key indices. The loop now iterates in reverse order to avoid scenarios where, for example, @10 is mistakenly replaced as @1, leaving an extra 0. This change ensures that the correct keys are replaced when deriving the wallet address.
@codecov-commenter
Copy link

codecov-commenter commented Jul 26, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (ab32733) 84.27% compared to head (89ab751) 84.27%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #189   +/-   ##
========================================
  Coverage    84.27%   84.27%           
========================================
  Files           17       17           
  Lines         2168     2168           
========================================
  Hits          1827     1827           
  Misses         341      341           
Flag Coverage Δ
unittests 84.27% <ø> (ø)

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.

Copy link
Collaborator

@bigspider bigspider left a comment

Choose a reason for hiding this comment

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

LGTM!

I removed the comment about closing #171 − I'll update it to reflect that only taproot scripts are missing.

Thanks a lot!

@bigspider bigspider merged commit 0fd12af into LedgerHQ:develop Jul 28, 2023
133 of 139 checks passed
@landabaso
Copy link
Contributor Author

Thank you Salvatore!

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.

None yet

3 participants