Add support for using the FileSystem Wallet without using EthAddress#94
Conversation
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
EnriqueL8
left a comment
EnriqueL8
left a comment
There was a problem hiding this comment.
Thanks @peterbroadhurst - looks good, a comment about the underlying API
| Close() error | ||
|
|
||
| GetAccounts(ctx context.Context) ([]string, error) | ||
| GetWalletFile(ctx context.Context, addr string) (keystorev3.WalletFile, error) |
There was a problem hiding this comment.
You need to update the WalletFile as it has a KeyPair function that returns a *secp256k1.KeyPair as other Web3 protocols might not use that signing algo/curve
There was a problem hiding this comment.
I understand users of keystorev3.WalletFile might choose to ignore the existence of the KeyPair() function as irrelevant to them (and use PrivateKey() instead).
However, I don't see a need to remove this function or modify that other package to have an interface that excludes it.
Do you have a strong reason for removing it?
There was a problem hiding this comment.
I see they could reconstruct their own representation using the private key expose
| if err == nil { | ||
| keypair := kv3.KeyPair() | ||
| if keypair.Address != *addr { | ||
| err = i18n.NewError(ctx, signermsgs.MsgAddressMismatch, keypair.Address, addr) |
There was a problem hiding this comment.
The reason I mentioned the comment above is because you are using that KeyPair() API to validate the address here - I think as part of this PR there should be a KeyPairGeneric construct interface which has three methods GetPubKey , GetPrivKey and GetAddr
There was a problem hiding this comment.
Specially the GetAddr should be a string inside of the ethaddress for the new Generic flow
There was a problem hiding this comment.
Looking at the code a bit closer, this will result in a lot of changes but is needed to plugin a new type of signing curve in the future
There was a problem hiding this comment.
Agreed that plugging into the other package keystorev3 a new form of cryptography would be a lot of work. I haven't proposed that here. Instead (per my comment above) users of this just access the private key from the wallet file using .PrivateKey() and can consider the KeyPair() function as "historical cruft".
|
What if instead of an AddressValidator it's actually an address creator, you want all your keys inside one FS Wallet to you the same address semantics so allow that to be provided and inside the KeyPair it's just an opaque string to the system? |
I'm trying to relate this question to the PR @EnriqueL8 sorry. Certainly interesting if someone wanted to contribute a (quite large) extensions to this simple FS wallet to support more advanced use cases like this. But it's not my proposal. I simply want to:
|
EnriqueL8
left a comment
EnriqueL8
left a comment
There was a problem hiding this comment.
Agree with the above, my concerns over the API are not completely valid as there are other paths to get the priv/pub key pair
| Close() error | ||
|
|
||
| GetAccounts(ctx context.Context) ([]string, error) | ||
| GetWalletFile(ctx context.Context, addr string) (keystorev3.WalletFile, error) |
There was a problem hiding this comment.
I see they could reconstruct their own representation using the private key expose
2 participants
The FS Wallet feature is very useful for managing on-disk wallets, but you currently have to use the Ethereum Address to reference the KeystoreV3 files.
This is odd in cases where using this library for signing with different Web3 ecosystems, such as when building connectors for FireFly to those ecosystems.
So this PR decouples the Eth specific layer (no change to API) from the generic underlying layer (new direct API for this).