Enable Cubist Signer integration #3965
Conversation
The code was changed without changing the behaviour. Instead of `os.Stat`ing the file, we just try to read the file (which returned the same error type).
Prior to this, we were checking if the file existed before attempting to readd it. It was unnecessary and the logic has been removed.
| StakingTLSKeyPath: getExpandedArg(v, StakingTLSKeyPathKey), | ||
| StakingTLSCertPath: getExpandedArg(v, StakingCertPathKey), |
There was a problem hiding this comment.
I renamed these to disambiguate from the bls signing items.
StephenButtolph
requested review from
a team and
maru-ava
and removed request for
a team
|
|
||
| default: | ||
| return node.SignerPathConfig{ | ||
| SigningKeyPath: getExpandedArg(v, StakingSignerKeyPathKey), |
There was a problem hiding this comment.
Just to confirm, if StakingSignerKeyPathKey is not set, do we use the default path?
There was a problem hiding this comment.
That's my understanding? But I'm not 100% how this expands to the default path - I just took the existing logic 😅
| if cfg.SignerPathIsSet { | ||
| return nil, errMissingStakingSigningKeyFile | ||
| } |
There was a problem hiding this comment.
I don't love this pattern of embedding metadata into the config struct that's only used for internal control flow. One alternative would be to return a signerPathIsSet bool from getStakingSignerConfig and pass that around, but dedicating an entire return parameter that needs to be managed by the caller for one specific configuration type is also cumbersome. I mainly wanted to call this out as a slightly awkward implementation, that may not have a cleaner alternative.
There was a problem hiding this comment.
Yeah, I don't think passing a return parameter around is a good choice. Would renaming the boolean make more sense? I just kind of went off what was there, but I think "fromExistingFile" might make more sense?
There was a problem hiding this comment.
I think a private field and a getter makes sense. I agree that I wouldn't add a return value.
There was a problem hiding this comment.
A private field in StakingConfig or in SignerPathConfig?
There was a problem hiding this comment.
I was thinking in SignerPathConfig but I now realize that this is in node package instead of config which would require a constructor or a setter. I don't feel strongly about this either way but do prefer not serializing it (by omitting the json tag) if it's left public.
Why this should be merged
The enables the rpc-signer configuration.
This is based off of this old PR.
How this works
I moved the instantiation of the signer from the
configpackage tonode.New()where we instantiate everything else that needs filesystem/networking. This way we don't need to propagatecleanupall over the place.I also opted for a
Shutdown()method onbls.Signer, which is how the other resources attached tonodeare managed. Currently, resources are not shutdown if we fail in another part ofnode.New(), but this is how the rest of the resources are also managed.Out of scope for this PR, but we may want to consider some sort of registry of cleanup/shutdown functions that can be unwound whenever we fail.
How this was tested
Tests are still WIP
Need to be documented in RELEASES.md?
Yes?