Add method to derive a wallet from PrivateKey #3059
Conversation
WalkthroughAdded functionality to derive public keys from private keys for secp256k1 and ed25519, including new derive methods in signing-scheme modules, an exported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/ripple-keypairs/src/index.ts (1)
98-105: LGTM! Consider adding input validation.The implementation correctly determines the algorithm and delegates to the appropriate signing scheme. The logic is clean and follows existing patterns.
Consider adding basic input validation for the
privateKeyparameter to provide better error messages for invalid inputs:function derivePublicKey(privateKey: string): string { + if (!privateKey || typeof privateKey !== 'string') { + throw new Error('privateKey must be a non-empty string') + } const algorithm = getAlgorithmFromPrivateKey(privateKey) if (algorithm === 'ecdsa-secp256k1') { return secp256k1.deriveKeypairFromPrivateKey(privateKey).publicKey } return ed25519.deriveKeypairFromPrivateKey(privateKey).publicKey }packages/xrpl/src/Wallet/index.ts (1)
177-185: LGTM! Consider adding input validation and security considerations.The implementation is clean and follows the existing patterns for static factory methods. The JSDoc documentation is thorough and consistent.
Consider these improvements:
- Input validation: Add basic validation to provide better error messages:
public static fromPrivateKey(privateKey: string): Wallet { + if (!privateKey || typeof privateKey !== 'string') { + throw new ValidationError('privateKey must be a non-empty string') + } return new Wallet(derivePublicKey(privateKey), privateKey) }
- Error handling: Consider wrapping the
derivePublicKeycall to provide more specific error messages for invalid private keys while being careful not to leak sensitive information.packages/ripple-keypairs/src/signing-schemes/secp256k1/index.ts (1)
33-46: LGTM! Consider adding input validation.The implementation correctly handles private key normalization and follows the same pattern as the existing
signmethod. The use of compressed public keys is consistent with thederiveKeypairmethod.Consider adding input validation for robustness:
deriveKeypairFromPrivateKey(privateKey: string): { privateKey: string publicKey: string } { + assert.ok( + (privateKey.length === 66 && privateKey.startsWith(SECP256K1_PREFIX)) || + privateKey.length === 64, + 'Invalid private key length or format' + ) const normalizedPrivateKey = privateKey.length === 66 && privateKey.startsWith(SECP256K1_PREFIX) ? privateKey.slice(2) : privateKey const buffer = Buffer.from(normalizedPrivateKey, 'hex') const publicKey = bytesToHex(nobleSecp256k1.getPublicKey(buffer, true)) return { privateKey, publicKey } },This matches the validation pattern used in the
signmethod (lines 52-55) and provides better error messages for invalid inputs.packages/ripple-keypairs/src/signing-schemes/ed25519/index.ts (1)
22-34: LGTM! Consider adding input validation for consistency.The implementation correctly handles private key normalization and is consistent with the existing
deriveKeypairmethod. The ED prefix handling matches the existing patterns.Consider adding input validation similar to the existing
signmethod for consistency:deriveKeypairFromPrivateKey(privateKey: string): { privateKey: string publicKey: string } { + assert.ok( + privateKey.startsWith(ED_PREFIX) ? privateKey.length === 66 : privateKey.length === 64, + 'Invalid ed25519 private key length' + ) const normalizedPrivateKey = privateKey.startsWith(ED_PREFIX) ? privateKey.slice(2) : privateKey const buffer = Buffer.from(normalizedPrivateKey, 'hex') const publicKey = ED_PREFIX + bytesToHex(nobleEd25519.getPublicKey(buffer)) return { privateKey, publicKey } },This provides better error messages and is consistent with the validation patterns used in the
signmethod (line 39-41).packages/xrpl/test/wallet/index.test.ts (2)
456-460: Consider improving error handling test specificity.The error handling tests verify that errors are thrown for malformed private keys but don't validate the specific error type or message. While this approach is consistent with other tests in the codebase, consider making the tests more specific to ensure proper error handling.
Consider this improvement to make error handling more specific:
- it('expect to throw error if private key incorrect format', function () { + it('throws error for malformed secp256k1 private key', function () { assert.throws(() => Wallet.fromPrivateKey(mockWallet_secp256k1.privateKey.slice(0, 10)), + /Invalid private key format/ ) }) - it('expect to throw error if private key incorrect format', function () { + it('throws error for malformed ed25519 private key', function () { assert.throws(() => Wallet.fromPrivateKey(mockWallet_ed25519.privateKey.slice(0, 10)), + /Invalid private key format/ ) })Note: Update the regex pattern to match the actual error message thrown by the implementation.
Also applies to: 476-480
450-450: Consider more specific test names.The test names could be more specific and concise. Since the tests are already organized in separate describe blocks by key type, the test names don't need to repeat the key type information.
Consider these more concise and specific names:
- it('derive keypair from private key', function () { + it('derives wallet from valid private key', function () { - it('expect to throw error if private key incorrect format', function () { + it('throws error for invalid private key format', function () { - it('derive keypair from private key', function () { + it('derives wallet from valid private key', function () { - it('expect to throw error if private key incorrect format', function () { + it('throws error for invalid private key format', function () {Also applies to: 456-456, 470-470, 476-476
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/ripple-keypairs/src/index.ts(2 hunks)packages/ripple-keypairs/src/signing-schemes/ed25519/index.ts(1 hunks)packages/ripple-keypairs/src/signing-schemes/secp256k1/index.ts(1 hunks)packages/ripple-keypairs/src/types.ts(1 hunks)packages/xrpl/src/Wallet/index.ts(2 hunks)packages/xrpl/test/wallet/index.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2024-12-06T18:44:55.095Z
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71
Timestamp: 2024-12-06T18:44:55.095Z
Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.
Applied to files:
packages/xrpl/test/wallet/index.test.ts
📚 Learning: 2024-12-06T19:25:15.376Z
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts:29-118
Timestamp: 2024-12-06T19:25:15.376Z
Learning: In the XRPLF/xrpl.js TypeScript client library, when writing tests (e.g., in `packages/xrpl/test/integration/transactions/`), we generally do not need to test rippled server behaviors, because those behaviors are covered by rippled's own integration and unit tests.
Applied to files:
packages/xrpl/test/wallet/index.test.ts
📚 Learning: 2025-01-08T02:12:28.489Z
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
Applied to files:
packages/xrpl/test/wallet/index.test.ts
📚 Learning: 2024-12-06T19:27:11.147Z
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.
Applied to files:
packages/xrpl/test/wallet/index.test.ts
📚 Learning: 2025-01-31T17:46:25.375Z
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2873
File: packages/xrpl/test/integration/transactions/trustSet.test.ts:0-0
Timestamp: 2025-01-31T17:46:25.375Z
Learning: For the XRPL implementation, extensive test cases for deep freeze behavior (high/low side interactions, clearing flags, etc.) are maintained in the C++ implementation and don't need to be duplicated in the JavaScript implementation.
Applied to files:
packages/xrpl/test/wallet/index.test.ts
📚 Learning: 2025-04-16T15:28:21.204Z
Learnt from: mvadari
PR: XRPLF/xrpl.js#2801
File: packages/xrpl/test/wallet/batchSigner.test.ts:0-0
Timestamp: 2025-04-16T15:28:21.204Z
Learning: In the XRPL.js library, hardcoded seeds in test files are acceptable as they don't represent protected data or real funds - they're only used for consistent test behavior.
Applied to files:
packages/xrpl/test/wallet/index.test.ts
📚 Learning: 2025-04-16T15:22:45.633Z
Learnt from: mvadari
PR: XRPLF/xrpl.js#2801
File: packages/xrpl/test/models/Batch.test.ts:0-0
Timestamp: 2025-04-16T15:22:45.633Z
Learning: Using `as any` type assertions is acceptable in test files for the XRPL.js project, as strict typing is not required for test code.
Applied to files:
packages/xrpl/test/wallet/index.test.ts
📚 Learning: 2025-02-12T23:30:40.622Z
Learnt from: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/DIDDelete.test.ts:28-31
Timestamp: 2025-02-12T23:30:40.622Z
Learning: In JavaScript/TypeScript transaction validation tests, object key validation can be performed using:
1. Object.keys() comparison with expected set
2. TypeScript interfaces with strict object literal checks
3. Object sanitization by filtering to allowed keys only
Applied to files:
packages/xrpl/test/wallet/index.test.ts
🧬 Code Graph Analysis (4)
packages/ripple-keypairs/src/signing-schemes/ed25519/index.ts (1)
packages/isomorphic/src/utils/index.ts (1)
bytesToHex(62-65)
packages/xrpl/src/Wallet/index.ts (2)
packages/xrpl/src/index.ts (1)
Wallet(12-12)packages/ripple-keypairs/src/index.ts (1)
derivePublicKey(116-116)
packages/ripple-keypairs/src/index.ts (1)
packages/ripple-keypairs/src/utils/getAlgorithmFromKey.ts (1)
getAlgorithmFromPrivateKey(119-121)
packages/ripple-keypairs/src/signing-schemes/secp256k1/index.ts (1)
packages/isomorphic/src/utils/index.ts (1)
bytesToHex(62-65)
🔇 Additional comments (4)
packages/ripple-keypairs/src/types.ts (1)
21-22: LGTM! Interface addition is well-designed.The new method signature follows the existing interface patterns and uses appropriate types. The
deriveKeypairFromPrivateKeymethod signature is consistent with the existingderiveKeypairmethod in terms of return type.packages/ripple-keypairs/src/index.ts (1)
116-116: LGTM! Export addition is correct.The new function is properly exported and follows the existing export patterns.
packages/xrpl/src/Wallet/index.ts (1)
20-20: LGTM! Import addition is correct.The
derivePublicKeyimport is properly added and follows the existing import patterns.packages/xrpl/test/wallet/index.test.ts (1)
442-448: LGTM! Well-structured test data.The mock wallet data follows proper XRPL key format conventions:
- secp256k1 private keys correctly start with '00' and public keys with '03'
- ed25519 keys correctly start with 'ED'
- All required fields (address, publicKey, privateKey) are present
The use of hardcoded test data is appropriate and consistent with the project's testing practices.
Also applies to: 463-469
There was a problem hiding this comment.
Thanks for this work. Please address the linter failures.
How do you envision the usage of your method? Will users generate their privateKeys on a trusted execution environment and use it to sign/verify the transactions?
Since there are existing methods to generate key-pairs from Seed, Entropy and Mnemonic, I'd like to understand the need for this method.
| const buffer = Buffer.from(normalizedPrivateKey, 'hex') | ||
|
|
||
| const publicKey = ED_PREFIX + bytesToHex(nobleEd25519.getPublicKey(buffer)) | ||
| return { privateKey, publicKey } |
There was a problem hiding this comment.
Would you consider adding the 'ED' prefix to the returned privateKey ? In this file, the deriveKeypair method returns both the publicKey and the privateKey along with the ED prefix.
There was a problem hiding this comment.
Having said that, I'm aware that the deriveKeypair method inside the secp256k1 (packages/ripple-keypairs/src/signing-schemes/secp256k1/index.ts) module does not explicitly add the 00 prefix.
Do you know what is the expected convention in cryptographic methods?
There was a problem hiding this comment.
Would you consider adding the 'ED' prefix to the returned
privateKey? In this file, thederiveKeypairmethod returns both thepublicKeyand theprivateKeyalong with theEDprefix.
https://github.com/interc0der/xrpl.js/blob/bed93562ebde0e13a2c919e74a5f3ab12d99ee0b/packages/ripple-keypairs/src/signing-schemes/ed25519/index.ts#L11-L20
The deriveKeypair method does not return the prefix. What are you referencing?
There was a problem hiding this comment.
Having said that, I'm aware that the
deriveKeypairmethod inside thesecp256k1(packages/ripple-keypairs/src/signing-schemes/secp256k1/index.ts) module does not explicitly add the00prefix.Do you know what is the expected convention in cryptographic methods?
This is why the deriveKeypairFromPrivateKey matches the conventions in the sign method. The function allows for a prefixed, or non-prefixed key - and derives the public address with the normalized private key.
Not sure what you mean by the expected convention in cryptographic methods.
- Secp256k1 expects/generates a 33 byte public key and a 32 byte private key.
- Ed25519 expects/generates a 32 byte public key and and 32 byte private key.
Key are normalized and padded according to algorithm. See this nice reference table: https://github.com/interc0der/xrpl.js/blob/main/packages/ripple-keypairs/src/utils/getAlgorithmFromKey.ts
| address: 'rhvh5SrgBL5V8oeV9EpDuVszeJSSCEkbPc', | ||
| publicKey: | ||
| '030E58CDD076E798C84755590AAF6237CA8FAE821070A59F648B517A30DC6F589D', | ||
| privateKey: | ||
| '00141BA006D3363D2FB2785E8DF4E44D3A49908780CB4FB51F6D217C08C021429F', |
There was a problem hiding this comment.
Hello, how did you generate this mocked privateKey and publicKey data? Which library/tool did you use?
There was a problem hiding this comment.
I believe I used this library to generate them. Why? - The unit check confirms that it is a valid keypair...
There was a problem hiding this comment.
Okay. As long as the newly introduced method was not used to generate the test case, it is acceptable.
| if (algorithm === 'ecdsa-secp256k1') { | ||
| return secp256k1.deriveKeypairFromPrivateKey(privateKey).publicKey | ||
| } | ||
| return ed25519.deriveKeypairFromPrivateKey(privateKey).publicKey |
There was a problem hiding this comment.
Can you add an explicit if-condition that does:
if (algorithm === 'ed25519') {
return ed25519.deriveKeypairFromPrivateKey(privateKey).publicKey
} else
throw new Error('Unknown signing scheme algorithm')
In the future, if support for new SigningSchemes are provided in the xrpl.js library, then this code is protected from incorrect default behavior.
There was a problem hiding this comment.
I didn't do this, because it was causing a linting issue.
I have added it and suppressed the warning.
https://github.com/interc0der/xrpl.js/blob/d6ba366155e56cbd9e736efcd9073717c01b7b69/packages/ripple-keypairs/src/index.ts#L98-L108
Exactly - there are methods to instantiate a wallet from Seed, Entropy and Mnemonic. There should also be a method for PrivateKey, not only for completeness, but for wallets that support both generating and deriving public addresses using a pk. Usage - Hmm, same usage as the methods above (Seed, Entropy and Mnemonic). Users can generate/derive a wallet using a private key. This method could be used for verifying that a private key is valid, and it can also be used to instantiate a Wallet for future signing. The linting issues seem to be coming from the binary-codec package which I did not modify. I am not going to correct an issue related to another change. I will continue to sync my branch with corrective commits. |
High Level Overview of Change
As there are more wallets natively supporting the XRP Ledger, private keys are being passed around in different context and there should be a method in this codebase to derive a wallet from a private key. Since we have (2) algorithms and this package correctly handles both, this is a necessary addition to the package and offers immense value.
Context of Change
Type of Change
Did you update HISTORY.md?
Test Plan