Migrate from elliptic to @noble/curves & Remove bitcoinjs-message#26
Migrate from elliptic to @noble/curves & Remove bitcoinjs-message#26
elliptic to @noble/curves & Remove bitcoinjs-message#26Conversation
Summary of ChangesHello @ACken2, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's security posture and maintainability by replacing outdated and vulnerable cryptographic dependencies with modern, well-audited alternatives. It introduces an internal, feature-rich implementation for Bitcoin message signing and verification, ensuring continued compatibility and robustness while streamlining the dependency footprint. The changes are thoroughly tested to guarantee functional equivalence and reliability. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Pull Request Test Coverage Report for Build 19595927260Details
💛 - Coveralls |
There was a problem hiding this comment.
Code Review
This is an excellent security enhancement, migrating from the unmaintained elliptic and bitcoinjs-message packages to the modern and audited @noble/curves. The new BitcoinMessage helper is a well-crafted replacement, and the comprehensive test suite, including cross-verification against the old library, is a great example of ensuring a safe migration. I've identified a critical bug in the new sign method due to incorrect API usage, which would lead to a runtime error. I have also included a suggestion to improve code clarity in the verify method. After addressing these points, this will be a very strong contribution to the project's security and maintainability.
Description:
🛡️ Security Fix & Dependency Migration
This PR addresses Issue #25 regarding the critical security vulnerabilities and lack of maintenance in the
ellipticpackage.To resolve this, we have completely removed the dependency tree rooted in
elliptic(includingbitcoinjs-messageand the legacysecp256k1node wrapper) and migrated to@noble/curves, which is modern, audited, and constant-time.🔄 Changes
Removed Dependencies:
bitcoinjs-message: Replaced with an internal, lightweight implementation.secp256k1: Replaced with@noble/curves.elliptic: Removed entirely (except its use in unit tests).New Implementation (
src/helpers/BitcoinMessage.ts):P2PKH), Nested Segwit (P2SH-P2WPKH), and Native Segwit (P2WPKH) signature headers.verifyfunction automatically detects the network (Mainnet, Testnet, or Regtest) of the provided address.Refactor:
src/helpers/Key.tsto usenoble-curvesfor key compression and decompression.src/helpers/BIP137.tsto usenoble-curvesfor public key recovery.🧪 Testing & Verification
We have added a comprehensive test suite to ensure zero regressions:
test/BitcoinMessage.test.ts) generates signatures using the oldbitcoinjs-messagelibrary and asserts that our new implementation generates the exact same byte-for-byte output.SignerandVerifierremains unchanged.🔗 Related Issues
Closes #25