feat: add algorithm property to Wallet class#3220
Open
slurpyone wants to merge 4 commits intoXRPLF:mainfrom
Open
feat: add algorithm property to Wallet class#3220slurpyone wants to merge 4 commits intoXRPLF:mainfrom
slurpyone wants to merge 4 commits intoXRPLF:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a public Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/xrpl/src/Wallet/index.ts (1)
233-240: Avoid silent no-op foropts.algorithmin bip39 mnemonic flow.In the bip39 branch,
opts.algorithmis accepted but ignored (Line 267 always sets secp256k1). Consider throwing when a non-secp256k1 algorithm is provided to make misuse explicit.Optional guard
public static fromMnemonic( mnemonic: string, @@ ): Wallet { if (opts.mnemonicEncoding === 'rfc1751') { @@ } + if ( + opts.algorithm != null && + opts.algorithm !== ECDSA.secp256k1 + ) { + throw new ValidationError( + 'bip39 mnemonics only derive secp256k1 keypairs', + ) + } // Otherwise decode using bip39's mnemonic standardAlso applies to: 265-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/Wallet/index.ts` around lines 233 - 240, In Wallet.fromMnemonic, the bip39 branch currently ignores opts.algorithm (always uses secp256k1) which can silently accept wrong input; update the bip39 flow in fromMnemonic to validate opts.algorithm and throw a clear error if a non-secp256k1 algorithm is supplied (e.g., if opts.algorithm is set and !== ECDSA.secp256k1), so callers cannot pass unsupported algorithms silently; reference the fromMnemonic method and the bip39 branch logic when adding this guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/xrpl/src/Wallet/index.ts`:
- Around line 119-121: Validate and normalize the algorithm assignment: if
opts.algorithm is provided, coerce it to a known value (e.g., compare
normalized/uppercased strings against allowed symbols like ECDSA.ed25519 and
ECDSA.secp256k1) and reject or fallback when it's invalid; otherwise infer from
publicKey using a case-insensitive prefix check (e.g., uppercased
publicKey.startsWith('ED')) before assigning this.algorithm, ensuring the final
value is one of the ECDSA.ed25519 or ECDSA.secp256k1 constants (and not an
arbitrary string).
In `@packages/xrpl/test/wallet/index.test.ts`:
- Around line 65-70: The test currently supplies ECDSA.secp256k1 for a secp256k1
key so it doesn't verify override precedence; change the provided algorithm in
the Wallet constructor to a different algorithm than the key's inferred one
(e.g., use ED25519 or another non-secp256k1 value) and assert wallet.algorithm
equals that provided value (use the existing symbols Wallet, publicKeySecp256k1,
privateKeySecp256k1 and the algorithm enum constants like ECDSA.secp256k1 and
ED25519 to locate and update the test).
---
Nitpick comments:
In `@packages/xrpl/src/Wallet/index.ts`:
- Around line 233-240: In Wallet.fromMnemonic, the bip39 branch currently
ignores opts.algorithm (always uses secp256k1) which can silently accept wrong
input; update the bip39 flow in fromMnemonic to validate opts.algorithm and
throw a clear error if a non-secp256k1 algorithm is supplied (e.g., if
opts.algorithm is set and !== ECDSA.secp256k1), so callers cannot pass
unsupported algorithms silently; reference the fromMnemonic method and the bip39
branch logic when adding this guard.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 20: Move the "npm-run-all2" entry out of package.json dependencies and
add it under devDependencies instead; specifically remove the "npm-run-all2":
"^8.0.4" line from the top-level dependencies and insert the same entry into
devDependencies, and then decide whether to remove the existing "npm-run-all"
devDependency to avoid redundancy (prefer keeping only "npm-run-all2"). Ensure
package.json remains valid JSON and run npm install / npm ci after the change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonpackages/xrpl/package.jsonpackages/xrpl/src/Wallet/index.tspackages/xrpl/test/wallet/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/xrpl/test/wallet/index.test.ts
- packages/xrpl/src/Wallet/index.ts
slurpyone
force-pushed
the
feat/wallet-algorithm-property
branch
from
c284b39 to
eef9517
Compare
slurpyone
force-pushed
the
feat/wallet-algorithm-property
branch
from
eef9517 to
ee2cf8a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/xrpl/src/Wallet/index.ts (1)
122-126:⚠️ Potential issue | 🟠 MajorStill validate
opts.algorithmat runtime.JS callers can still pass any string here, so
wallet.algorithmcan stop matching the actual keypair. Reject non-ECDSAvalues instead of trusting the runtime input.💡 Suggested change
+ const inferredAlgorithm = publicKey.toUpperCase().startsWith('ED') + ? ECDSA.ed25519 + : ECDSA.secp256k1 + + if ( + opts.algorithm != null && + !Object.values(ECDSA).includes(opts.algorithm) + ) { + throw new ValidationError('Invalid cryptographic signing algorithm') + } + this.algorithm = - opts.algorithm ?? - (publicKey.toUpperCase().startsWith('ED') - ? ECDSA.ed25519 - : ECDSA.secp256k1) + opts.algorithm ?? inferredAlgorithm🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/Wallet/index.ts` around lines 122 - 126, The code assigns this.algorithm from opts.algorithm without runtime validation, allowing callers to set arbitrary strings that may not match the expected ECDSA constants; update the Wallet constructor (or wherever this.algorithm is set) to validate opts.algorithm when provided: if opts.algorithm is present ensure it equals one of the known values (ECDSA.ed25519 or ECDSA.secp256k1) and throw a clear error if not, otherwise fall back to the existing publicKey-based detection (publicKey.toUpperCase().startsWith('ED') ? ECDSA.ed25519 : ECDSA.secp256k1); reference opts.algorithm, this.algorithm, publicKey, ECDSA.ed25519 and ECDSA.secp256k1 when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/xrpl/src/Wallet/index.ts`:
- Around line 324-331: The code currently forces DEFAULT_ALGORITHM into
deriveKeypair by computing const algorithm = opts.algorithm ?? DEFAULT_ALGORITHM
and always passing it; change this so deriveKeypair receives an
options.algorithm key only when the caller explicitly provided opts.algorithm
(i.e., if opts.algorithm !== undefined), letting deriveKeypair fall back to the
algorithm encoded in the seed otherwise; update the call site around
deriveKeypair(seed, { ... }) in the Wallet creation path (symbols:
deriveKeypair, DEFAULT_ALGORITHM, opts.algorithm,
Wallet.fromSeed/Wallet.fromSecret wrapping code) to conditionally include the
algorithm property instead of unconditionally supplying DEFAULT_ALGORITHM.
---
Duplicate comments:
In `@packages/xrpl/src/Wallet/index.ts`:
- Around line 122-126: The code assigns this.algorithm from opts.algorithm
without runtime validation, allowing callers to set arbitrary strings that may
not match the expected ECDSA constants; update the Wallet constructor (or
wherever this.algorithm is set) to validate opts.algorithm when provided: if
opts.algorithm is present ensure it equals one of the known values
(ECDSA.ed25519 or ECDSA.secp256k1) and throw a clear error if not, otherwise
fall back to the existing publicKey-based detection
(publicKey.toUpperCase().startsWith('ED') ? ECDSA.ed25519 : ECDSA.secp256k1);
reference opts.algorithm, this.algorithm, publicKey, ECDSA.ed25519 and
ECDSA.secp256k1 when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5747dbcb-d4e3-446e-ab0f-e0da6bc9fcfe
📒 Files selected for processing (2)
packages/xrpl/src/Wallet/index.tspackages/xrpl/test/wallet/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/test/wallet/index.test.ts
slurpyone
force-pushed
the
feat/wallet-algorithm-property
branch
4 times, most recently
from
bcf624b to
3c830e6
Compare
slurpyone
force-pushed
the
feat/wallet-algorithm-property
branch
3 times, most recently
from
87e03da to
39075c8
Compare
Adds a public readonly 'algorithm' property (ECDSA enum) to the Wallet class that reports the cryptographic signing algorithm used to derive the keypair. When not explicitly provided, the algorithm is inferred from the public key prefix: keys starting with 'ED' are ed25519, all others secp256k1. All factory methods (generate, fromSeed, fromEntropy, fromMnemonic) now thread the algorithm through to the constructor, so the property is always accurate regardless of how the wallet was created. Fixes XRPLF#3190
…e tests - Improve JSDoc with @link references to ECDSA enum values - Add comment explaining why bip39 path hardcodes secp256k1 - Add roundtrip recreation test (the core use case from XRPLF#3190) - Add bip39 mnemonic test without explicit algorithm param - Add fromSecretNumbers default algorithm assertion
- Revert accidental package.json/package-lock.json changes (dev dep pollution) - Fix override test: use ed25519 key with explicit secp256k1 to actually test precedence - Add case-insensitive public key prefix check (.toUpperCase()) for robustness - Add validation guard: throw when bip39 mnemonic is used with non-secp256k1 algorithm - Add test for bip39 + ed25519 algorithm rejection
…eriveKeypair - Validate opts.algorithm in constructor: throw if not a known ECDSA value - Only pass algorithm to deriveKeypair when explicitly provided (don't force DEFAULT_ALGORITHM) - Preserves fallback to seed-encoded algorithm when algorithm isn't specified - Addresses CodeRabbit review feedback on potential issue (line 122-126) and optimization (line 324-331)
slurpyone
force-pushed
the
feat/wallet-algorithm-property
branch
from
39075c8 to
d02c501
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #3190
Adds a
public readonly algorithm: ECDSAproperty to theWalletclass that reports the cryptographic signing algorithm used to derive the keypair.Problem
When creating a wallet via
Wallet.generate(),Wallet.fromSeed(), orclient.fundWallet()with default options, there is no way to determine which algorithm was used. This matters because:Changes
public readonly algorithm: ECDSAproperty to theWalletclassalgorithmfield to the constructor'soptsparametergenerate,fromSeed,fromEntropy,fromMnemonic) now thread the resolved algorithm through to the constructornew Wallet(pubKey, privKey)), the algorithm is inferred from the public key prefix: keys starting withEDuse ed25519, all others use secp256k1Backward Compatibility
algorithmparameter in constructoroptsis optional; existingnew Wallet(pub, priv)calls work identicallyExample