Skip to content

feat: decouple ecpair from tiny-secp256k1 (allow injecting ecc lib) #4

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
Merged
merged 10 commits into from
Dec 15, 2021
Merged

feat: decouple ecpair from tiny-secp256k1 (allow injecting ecc lib) #4

merged 10 commits into from
Dec 15, 2021

Conversation

motorina0
Copy link
Member

@motorina0 motorina0 commented Dec 14, 2021 •
edited
Loading

Closes: #3

Summary

  • the changes introduced here follow the pattern from bip32
  • decouple ecpair from tiny-secp256k1
  • the ecc lib must be provided by the consumer of ecpair (see tests for details)
  • the TinySecp256k1Interface interface only exposes the methods required by ecpair
    • testecc.ts checks the methos of TinySecp256k1Interface
  • the class ECPair is no longer directly exported, but instead ECPairInterface is exposed
  • signSchnorr() and verifySchnorr() have been added

The changes are split in 4 commits which can be cherry picked if needed.

Further comments

  • update documentation (if this PR is approved)
  • not sure how/if SignerAsync is used
    • signSchnorr() - has been added to this interface

@motorina0
feat: decouple the ECC dependency from this library; allow to inject …
371dab3 
…the needed ECC lib

- no functional changes have been made (only refactoring)
- TinySecp256k1Interface follows the "tiny-secp256k1" version 1.1.6 of the interface
- tiny-secp256k1 moved to dev dependencies
- fromXXX() functions declare as return type the `ECPairInterface` instead of the `ECPair` class
- `ECPair` class is no longer exported
@motorina0
feat: use tiny-secp256k1 version 2.1.2; update TinySecp256k1Interface
10589ff
@motorina0
feat: add signSchnorr() and verifySchnorr() functions and unit tests
e729300
@motorina0
feat: add checks for the injected ecc lib
25a6b12
@junderw
Copy link
Member

junderw commented Dec 14, 2021

Signer and SignerAsync are interfaces that are being used in bitcoinjs-lib in Psbt etc... so adding more constraints would be a breaking change and we don't want to do that.

Right now, just leave those interfaces alone. Once proper taproot is supported, we will probably add a new interface for schnorr.

@junderw
Copy link
Member

junderw commented Dec 14, 2021

linter is failing... (tslint should exclude test files tbh)

@motorina0
Copy link
Member Author

Updated:

  • fix lint:tests
    • did not removed it from the CI flow tough, the issues it reported were not noise
  • remove signSchnorr() from SignerAsync
    • not removed from Signer since ECPairInterface extends Signer
    • adding a new method should not affect existing users
  • update README
  • bump version (major - since the public API changed)
  • update dependencies to latest
    • strange enough these deps were already to latest in package-lock.json

@junderw
Copy link
Member

junderw commented Dec 15, 2021

Please remove signSchnorr from Signer as well. And add it to ECPairInterface instead.

@motorina0
Copy link
Member Author

Please remove signSchnorr from Signer as well. And add it to ECPairInterface instead.

Done!

junderw
junderw reviewed Dec 15, 2021
View reviewed changes
README.md Outdated
Comment on lines 16 to 17
const tinysecp = require('tiny-secp256k1');
const ECPair: ECPairAPI = ECPairFactory(tinysecp);
Copy link
Member

Choose a reason for hiding this comment

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

ECPairAPI is not imported.

Also, I think you want to put const tinysecp: TinySecp256k1Interface = ...

@junderw
Copy link
Member

junderw commented Dec 15, 2021

LGTM other than the README stuff.

@motorina0
Copy link
Member Author

LGTM other than the README stuff.

Great! README file code sample fixed.

@junderw junderw merged commit a9c2bf5 into bitcoinjs:master Dec 15, 2021
@junderw
Copy link
Member

junderw commented Dec 15, 2021

Thanks a ton. Published as 2.0.0

@motorina0 motorina0 mentioned this pull request May 30, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@junderw junderw junderw 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.

Add modular method of injecting the needed ecc interfaces
2 participants
@motorina0 @junderw