Support for xrpl-secret-numbers encoded seed #187
Conversation
There was a problem hiding this comment.
Nice, looks really good! Read over the source code of secret numbers and everything looks good. Made one issue over there but nothing major.
I do hope we can standardize on one format for exporting and displaying seeds. Also it would be nice if we could generate seeds using only one lib (even though they both use brorand at the lowest level.)
|
I'm generally happy with this and the implementation looks excellent (I'd expect no less). I just want to make sure that Xpring is ready to support this across the suite of products we're going to offer (ripple-lib and rippled, for starters). Can you clarify a bit about what you hope to do with this? Where else does this exist in the ripple ecosystem? No wrong answers, just fact finding. |
|
@Stormtv Thanks. Fixed that issue and updated this PR. @keefertaylor Great :) Regarding rippled: it's bad practice to send the family seed (or any other secret) anyway, and for now the secret numbers are compatible with a family seed anyway. @Stormtv + @keefertaylor regarding standardize & ripple-lib: I think at some point -if we decide secret numbers are helpful- I may suggest a PR for ripple-keypairs to add it and then another PR to ripple-lib, etc. One could argue that a semi XRPL-sepecific secret, be it family seed or secret numbers, is undesirable anyway as the BIP standards are more commonly used for hardware & software wallets & can be used to derive accounts for multiple currencies. However: I've had plenty mnemonic recovery requests over time. In all cases I was able to brute force recover the right mnemonic. Always spelling mistakes / handwriting errors / vowels simply replaced ending up with another existing, valid word. I do believe using the secret numbers representation will save a lot of people the fear of not being able to access their funds in the future. So And (possible follow up question) on "why start with |
Sure. I think that I'm concerned that the native methods in rippled (ex wallet_propose) return b58check encoded seeds rather than magic numbers. Similarly, the XRP {Dev, Test}Net faucets only deal in b58check encoded seeds and I'm sure theres some other tooling elsewhere as well.
My concern here is that if we're not standardized across the entire ecosystem then we increase the number of options folks have and also the chances of confusion / loss of funds.
Argument bought
In general, I'd like to see this drafted as a standard in the XRP ecosystem before we fully support it. It sounds like there are numerous advantages, and I just want to make sure we have broad community support before we bolt this onto the side of the ecosystem and you / me / whoever else has to support it forever. Would you be willing to put out a draft standard? EDIT: Drafting a standard would also be advantageous so even if one day this gets deprecated, some random user who saved their seed this way can restore it, even if our software doesn't support it. |
# Conflicts: # src/XRP/wallet.ts # test/XRP/wallet.test.ts
|
@keefertaylor Took a while (sorry, ended up at my "pile of todo's not written down").
|
Add wallet with 'secret numbers' seed.
High Level Overview of Change
Add
generateWalletFromSecretNumbersand tests relying on xrpl-secret-numbers. Based on the idea by @nbougalisContext of Change
As it is really easy for humans to make mistakes in alpha chars (mnemonic, but family seeds are even worse) a new suggested seed encoding ("secret numbers") with per chunk checksumming will be used in XUMM by default. This commit adds support for 'secret numbers'. No new sub-dependencies as it relies on existing libs like ripple-keypairs / brorand.
(Secret numbers are 'backwards compatible' with family seeds as they simply generate the seedbuffer usable encoded into the family seed)
(Type:
string/string[]»string[]instead ofnumber[]as a chunk of numbers can start with a zero)Type of Change
Add generateWalletFromSecretNumbers() & tests, add support for another secret type (mnemonic, family seed, adding 'secret numbers')
Test Plan
Etc.