ssv-signer support #2028
ssv-signer support #2028
Conversation
ssvsigner/types.go
Outdated
| type RemoteSigner interface { | ||
| ListKeys(ctx context.Context) ([]string, error) | ||
| ImportKeystore(ctx context.Context, keystoreList, keystorePasswordList []string) ([]web3signer.Status, error) | ||
| DeleteKeystore(ctx context.Context, sharePubKeyList []string) ([]web3signer.Status, error) | ||
| Sign(ctx context.Context, sharePubKey []byte, payload web3signer.SignRequest) ([]byte, error) | ||
| } |
There was a problem hiding this comment.
Because we are importing web3signer package here anyway - it seems web3signer package is a better place to have RemoteSigner interface defined as well don't you think ?
Just to be on the same page - I believe it only makes sense to define interface on the user/caller side in the following cases (which doesn't seem to apply here):
- we can't define it in the original package (maybe we can't modify that code, eg. it's a 3rd party lib)
- we don't want to import the package defining some
interfacewe need as a dependency for some reason - or we just want a subset of methods on some
interface(because we either want to make it explicit which ones we actually gonna use; and/or maybe it's just simpler to mock out smaller interface)
The reason I don't like to define interface(s) on the user/caller side is that eventually results in bunch of interface definitions (often duplicate ones) all over the code which makes it harder to navigate. Maybe it's not a problem if such definitions are made package-private (so it's clear they aren't meant to be re-imported by other packages) - but that's a convention we'd need to explicitly follow for it to work like that.
wdyt ? cc @olegshmuelov @oleg-ssvlabs @moshe-blox @y0sher @MatusKysel
There was a problem hiding this comment.
We also want to support other signers, such as https://github.com/attestantio/dirk. They require different implementations. I don't mind changing it to unexported though
There was a problem hiding this comment.
made it unexported
| if len(statuses) != 1 { | ||
| return fmt.Errorf("bug: expected 1 status, got %d", len(statuses)) | ||
| } |
There was a problem hiding this comment.
shouldn't this check be done by default in the client if the responses are always expected to return len(statuses) == len(pubkeys)?
| type BlockRootWithSlot struct { | ||
| spectypes.SSZBytes // block root | ||
| Slot phase0.Slot | ||
| } |
There was a problem hiding this comment.
if this is a type solely meant for signing, perhaps this should be in ekm package
also, if it should only be a phase0.Root, then let's please use that type to guarantee that...
| ) | ||
|
|
||
| type BeaconBlockData struct { | ||
| Version string `json:"version"` |
There was a problem hiding this comment.
lets use our custom DataVersion wrapper instead of string (see #2028 (comment))
Add remote signer support