[Feature] Generate Wallet from secret numbers #1799
Conversation
| @@ -27,7 +27,8 @@ | |||
| "ripple-address-codec": "^4.1.1", | |||
| "ripple-binary-codec": "^1.1.3", | |||
| "ripple-keypairs": "^1.0.3", | |||
| "ws": "^8.2.2" | |||
| "ws": "^8.2.2", | |||
| "xrpl-secret-numbers": "^0.3.3" | |||
There was a problem hiding this comment.
should this be included in the monorepo?
There was a problem hiding this comment.
Maybe wait to bring it up to the top level until another package needs it?
There was a problem hiding this comment.
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 sense
There was a problem hiding this comment.
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.
@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.
that would be a good reason to maintain it within the monorepo :)
IMO, the criteria for whether it should be part of the monorepo:
- is the purpose/scope of the package overlapping with the monorepo's? in this case i think it might be
- does it depend on other packages w/in this monorepo, potentially requiring work on it and
ripple-keypairssimultaneously.
intelliot
changed the title
| // TODO: Uncomment this test when the `deriveKeypair` fix is merged | ||
| // it('derives a wallet using algorithm ed25519', function () { | ||
| // const algorithm = ECDSA.ed25519 | ||
| // const wallet = Wallet.fromSecretNumbers(secretNumbersString, { | ||
| // algorithm, | ||
| // }) | ||
|
|
||
| // assert.equal(wallet.publicKey, publicKeyED25519) | ||
| // assert.equal(wallet.privateKey, privateKeyED25519) | ||
| // }) |
There was a problem hiding this comment.
This test fails because of this issue: #2376
Commented with a TODO to re-add the test once that PR is resolved. (Open to other ideas about what's the best way to handle this :) )
JST5000
requested review from
mvadari,
ckniffen,
jonathanlei and
khancode
and removed request for
khancode
JST5000
force-pushed
the
ds/wallet-from-secret-numbers
branch
from
9d06e03
to
a8d3a93
Compare
|
Ideally I would like to see this as a function separate from |
@ckniffen I think changing this to not be part of Do you think it's worth having this be off the |
|
One option is to deprecate the static methods in 3.0 besides This would allow tree shaking of |
Generate Wallet from Secret Numbers
Context of Change
This PR introduces a new feature. Many people are using XUMM. XUMM offers the recovery key for the user as Secret Numbers
To make it easy for developers to use their XUMM managed account for their development work, a new Method, "Wallet.fromSercretNumbers" is introduces. This method generates a new Wallet instance by the given Secret Numbers.
Alternatively, the developer had to first derive the family seed / secret key from the Secret Numbers and use the seed to initiate the Wallet instance.
Type of Change
Test Plan
Tests can be found in the index.ts of the /test/Wallet directory
Future Tasks
The documentation has to be updated / generated. I did not run the "npm run docgen" target because it did not just added my method but changed lots of other things in the docs as well. So maybe someone else can run it.