Conversation
emlautarom1
left a comment
emlautarom1
left a comment
There was a problem hiding this comment.
There are some flows that are not 100% matching with Charon. Consider adding tests for them if it can be done in a reasonable time.
|
|
||
| let num_operators = | ||
| u64::try_from(operators.len()).map_err(|_| CreateDkgError::OperatorCountOverflow)?; | ||
| let safe_thresh = safe_threshold(num_operators)?; |
There was a problem hiding this comment.
This does not look correct: num_operators = args.operator_addresses.len() + args.operator_enrs.len() when it should be num_operators = operators_len
There was a problem hiding this comment.
operator_enrs or operator_addresses are mutually exclusive, the result is the same. I'll add a comment to describe it.
| // Verify signatures when an ETH1 endpoint is available. Skipped when the | ||
| // endpoint is empty because the client cannot connect — safe for DKG create | ||
| // since operators have no signatures at this stage. | ||
| if !args.publish && !args.execution_engine_addr.is_empty() { |
There was a problem hiding this comment.
We should not have the !args.execution_engine_addr.is_empty() check: if args.publish == true we must verify the signature. A missing args.execution_engine_addr should be an error (automatically propagated with ?).
There was a problem hiding this comment.
Publish and ExecutionEngineAddr are unrelated. Publish controls sending the resulting lock file to Obol API, whereas ExecutionEngineAddr controls on-chain signature verification for gnosis/safe chains. In most cases (ethereum mainnet/testnets) this parameter is omitted.
There was a problem hiding this comment.
I found the source of the confusion, Charon implements noop eth client for empty execution_engine_addr, added similar behaviour to the Pluto.
There was a problem hiding this comment.
You may not need to implement any real eth1 client at this time (as well as EL sig verification), as this is very low priority feature, not a priority for your work, so please don't waste your time for the next milestone.
|
@emlautarom1 I've addressed all comments, please check. |
3 participants
Implementation of the
pluto create dkgcommandfixes #304