sdk-wallet: some refactoring, additional explanations #32
Conversation
andreibancioiu
changed the title
sdk-wallet/cookbook.md
Outdated
| if keystore.get_kind() == "secretKey": | ||
| first_secret_key = keystore.get_secret_key() | ||
| else: | ||
| mnemonic = keystore.get_mnemonic() |
There was a problem hiding this comment.
Wallet-like applications that support JSON keystores (e.g. web wallet, mxpy) have to handle the keystore with respect to it's kind.
| @@ -5,12 +5,20 @@ For languages that support **structural typing** (e.g. Go, Python, TypeScript), | |||
| For languages that only support **nominal typing** (e.g. C#), these interfaces can be _exported_. | |||
|
|
|||
| ``` | |||
| interface IWalletProvider: | |||
| generate_keypair(): (ISecretKey, IPublicKey) | |||
| interface ISigner: | |||
There was a problem hiding this comment.
IWalletProvider was segregated.
See: #31 (comment).
|
|
||
| // Should not throw. | ||
| generate_mnemonic(): Mnemonic | ||
|
|
||
| // Should not throw. | ||
| validate_mnemonic(mnemonic: Mnemonic): bool | ||
|
|
||
| // Can throw: | ||
| // - ErrInvalidMnemonic | ||
| // Below, "passphrase" is the optional bip39 passphrase used to derive a secret key from a mnemonic. | ||
| // Reference: https://en.bitcoin.it/wiki/Seed_phrase#Two-factor_seed_phrases | ||
| derive_secret_key_from_mnemonic(mnemonic: Mnemonic, address_index: int, passphrase: string = ""): ISecretKey |
There was a problem hiding this comment.
MnemonicComputer has been merged into this.
| @@ -1,33 +1,48 @@ | |||
| ## EncryptedKeystore | |||
|
|
|||
| `EncryptedKeystore` handles encrypted JSON files. | |||
|
|
|||
| Note: current design is suboptimal. `EncryptedKeystore` handles both legacy keystore files (that hold encrypted secret keys) and new keystore files (that hold encrypted mnemonics). `get_kind()` can be used as a differentiator. A future design might involve two separate classes. | |||
There was a problem hiding this comment.
Question for review: all right for the moment? 🙏
Also, see the example(s) in cookbook.md.
sdk-wallet/user_wallet_provider.md
Outdated
| @@ -1,7 +1,11 @@ | |||
| ## UserWalletProvider | |||
|
|
|||
| Provides functionality for generating secret keys, derivating public keys, signing & verifying data. | |||
|
|
|||
| Additionally, allows one to generate and validate mnemonics, and to deriving secret keys from mnemonics (i.e. BIP39). | |||
| ``` | ||
| class EncryptedKeystore: | ||
| // The constructor is not captured by the specs; it's up to the implementing library to define it. | ||
| // The constructor is not strictly captured by the specs; it's up to the implementing library to define it. Suggestion below. |
There was a problem hiding this comment.
Please remove this private constructor, it's out of the scope of the provided specs. We should have only those 2 named constructors as example
+ providing a type on which the constructor will do a switch is not a very good example (clean code wise)
There was a problem hiding this comment.
Fixed phrasing, removed private constructor. No switch should be made in the implementation.
| // If kind == 'mnemonic', `secret_key` must be nil, while `mnemonic` must be provided. | ||
| // In the implementation, all the parameters would be held as instance state (private fields). | ||
| private constructor(keys_computer: IKeysComputer, kind: string, secret_key?: ISecretKey, mnemonic?: Mnemonic) | ||
| // The constructor is not captured by the specs; it's up to the implementing library to define it. |
There was a problem hiding this comment.
As stated in the last comment this can be implemented in a cleaner way by the following 2 named constructors: one for the mnemonic and one for the secret key. This is clearly not required
| static import_from_object(wallet_provider: IWalletProvider, object: KeyfileObject, password: string): EncryptedKeystore | ||
| // This should decrypt the encrypted data, then call the (private) constructor. | ||
| // "password" should not be retained. | ||
| static import_from_object(keys_computer: IKeysComputer, object: KeyfileObject, password: string): EncryptedKeystore |
There was a problem hiding this comment.
object: KeyfileObject should be something like keystore_content: EncryptedKeystoreContent
| ``` | ||
|
|
||
| ``` | ||
| interface IKeysComputer: |
There was a problem hiding this comment.
In User/Validator wallet provider this is called IKeysGenerator. Which name is correct?
IWalletProviderinto:ISigner,IVerifier,IKeysComputer,IMnemonicComputer.MnemonicComputerback intoUserWalletProvider.