-
Notifications
You must be signed in to change notification settings - Fork 514
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
[Feature] Generate Wallet from secret numbers #1799
Changes from 5 commits
42bb7bb
99fb3bd
976a77a
55245e0
75ccbbd
02f638f
8e14f6a
01a3938
ce833a6
ea32c88
e558399
88e9b4d
63c469f
c655993
45b8d91
22e104e
700a18b
7efdbe7
15cfee9
7e83e17
e50e7a2
d606d9c
130594d
451cdbf
a8d3a93
0336c26
ca9cebf
d3a467a
d416f5a
91f9f9a
74bf3b7
db77d01
61d62fd
ac17c09
0eef2c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,10 @@ import { | |
sign, | ||
} from 'ripple-keypairs' | ||
|
||
import { | ||
Utils | ||
} from 'xrpl-secret-numbers' | ||
|
||
import ECDSA from '../ECDSA' | ||
import { ValidationError } from '../errors' | ||
import { Transaction } from '../models/transactions' | ||
|
@@ -112,7 +116,8 @@ class Wallet { | |
privateKey: string, | ||
opts: { | ||
masterAddress?: string | ||
seed?: string | ||
seed?: string, | ||
secretNumbers?: string | ||
nixer89 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} = {}, | ||
) { | ||
this.publicKey = publicKey | ||
|
@@ -197,6 +202,35 @@ class Wallet { | |
}) | ||
} | ||
|
||
/** | ||
* Derives a wallet from secret numbers. | ||
* | ||
* @param secretNumbers - A string consisting of 8 times 6 numbers (whitespace delimited) used to derive a wallet. | ||
* @param opts - (Optional) Options to derive a Wallet. | ||
* @param opts.masterAddress - Include if a Wallet uses a Regular Key Pair. It must be the master address of the account. | ||
nixer89 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* @returns A Wallet derived from secret numbers. | ||
* @throws ValidationError if unable to derive private key from secret number input. | ||
*/ | ||
public static fromSecretNumbers( | ||
secretNumbers: Array<string> | string, | ||
opts: { masterAddress?: string, algorithm?: ECDSA } = {}, | ||
): Wallet { | ||
let numbersArray:Array<string> = []; | ||
|
||
if (typeof secretNumbers === 'string') { | ||
numbersArray = Utils.parseSecretString(secretNumbers); | ||
} else if (Array.isArray(secretNumbers)) { | ||
numbersArray = secretNumbers; | ||
} | ||
nixer89 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const entropy = Utils.secretToEntropy(numbersArray) | ||
|
||
return Wallet.fromEntropy(entropy, { | ||
algorithm: opts.algorithm ?? ECDSA.secp256k1, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can also just forward the return Wallet.fromEntropy(entropy, opts) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I decided to fallback to So if someone wants to create a xrpl.js "Wallet.fromSecretNumbers" he always has to define the algorithm parameter as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense, could you add a comment with that context? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, will do tomorrow 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I don't agree. I think that XUMM in this case should override the default. We want this lib to be useful to anyone and we want people to default to ed25519 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant it the other way around: Users create a wallet in XUMM. This process gives them the "Secret Numbers" via Now this XUMM user is also a developer. So he wants to build on XRPL, using xrpl.js and his Secret Numbers he got from XUMM. If What is the reason to always default to ed25519? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason to default to ed25519 is that it's apparently got better performance. (See comment on the two algorithms here: https://xrpl.org/cryptographic-keys.html#signing-algorithms) |
||
masterAddress: opts.masterAddress, | ||
}); | ||
} | ||
|
||
/** | ||
* Derives a wallet from an entropy (array of random numbers). | ||
* | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be included in the monorepo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe wait to bring it up to the top level until another package needs it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no i mean, should
xrpl-secret-numbers
's source be in this monorepo. not sure where else it's used or how big it is, but that might make senseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sure the XRPL-Labs team uses this lib in their XUMM app. I think @WietseWind is the maintainer?
For now I would leave it as "external" import and would not add it to the monorepo. but maybe Wietse thinks this would be a good idea :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ledhed2222 It's a fairly small package, and the (non dev) dependencies overlap with
ripple-keypairs
(brorand, ripple-keypairs)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that would be a good reason to maintain it within the monorepo :)
IMO, the criteria for whether it should be part of the monorepo:
ripple-keypairs
simultaneously.