-
Notifications
You must be signed in to change notification settings - Fork 781
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove E from validator_client types where it's not required #6727
base: unstable
Are you sure you want to change the base?
Conversation
30c3a7c
to
96dcd1d
Compare
object, | ||
Web3SignerObject::Deposit { .. } | Web3SignerObject::ValidatorRegistration(_) | ||
) && fork_info.is_some() | ||
if matches!(message_type, MessageType::ValidatorRegistration) && fork_info.is_some() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
object
can never be a Web3SignerObject::Deposit
as a SignableMessage
doesn't convert to it, see line 202
above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels kinda hard to keep track if SignableMessage
starts converting to a Web3SignerObject::Deposit
in the future.
I don't see the issue with keeping it as is, but what you said makes sense as well.
Although, i don't have enough context on the web3signer to know for sure
6f0df3d
to
b01c568
Compare
b4920c5
to
08b3156
Compare
…rom-validator-client
08b3156
to
a9cc2fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I'd recommend running this version of the VC on testnets before merging to make sure there isn't something that breaks unexpectedly
@@ -10,7 +10,10 @@ path = "src/lib.rs" | |||
|
|||
[dependencies] | |||
account_utils = { workspace = true } | |||
beacon_node_fallback = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really sure why we had to add dependencies here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup you are right, it was because I had remove doppelganger_service
as a separate crate but I have since removed again, thanks Pawan!
object, | ||
Web3SignerObject::Deposit { .. } | Web3SignerObject::ValidatorRegistration(_) | ||
) && fork_info.is_some() | ||
if matches!(message_type, MessageType::ValidatorRegistration) && fork_info.is_some() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels kinda hard to keep track if SignableMessage
starts converting to a Web3SignerObject::Deposit
in the future.
I don't see the issue with keeping it as is, but what you said makes sense as well.
Although, i don't have enough context on the web3signer to know for sure
and the
Ethspec
parameters can be passed on function all time