Open MatheusFranco99 opened 8 months ago
@GalRogozinski Should we add a check inside StartNewInstance
just to check if the value is not empty?
@MatheusFranco99 for your question... shouldn't this check be on decided?
@GalRogozinski What do you mean by "on decided"? Once the instance decides? Btw, should this PR be into main or dev?
@MatheusFranco99
Sorry was being super unclear.
I meant that the check you offered should be on decide()
the function that calls StartNewInstance
@GalRogozinski Oh yeah, I agree
Actually, thinking about it again, we would have something like:
func (b *BaseRunner) decide(runner Runner, input *types.ConsensusData) error {
byts, err := input.Encode()
if err != nil {
return errors.Wrap(err, "could not encode ConsensusData")
}
// New check
if len(byts) == 0 {
return errors.Wrap(err, "...")
}
if err := runner.GetValCheckF()(byts); err != nil {
return errors.Wrap(err, "input data invalid")
}
// ...
But runner.GetValCheckF
should already cover it. What do you think?
@MatheusFranco99
For example on BeaconVoteValueCheckF
I don't see an empty check. I see we do a Decode
that I am not sure will fail (maybe it returns empty struct, unclear from the code and spec should be clear). It will of course fail but with some other error message.
You have a point though, that it is nice that this will be inside the value check. But this probably means that you need to add the check in each function.
From a SW engineer perspective I would make a base function wrapper that does checks that should happen across all value checks and then calls an internal ValueCheck
function. But I don't know how readable it is for spec.
So for spec I would either use your snippet or add the empty check in each valueCheckF
. You can choose. The first one won't translate the check to the postconsensus stage, but this is fine since we should never have consensus on an empty value.
@GalRogozinski
It seems so according to the test: postconsensus.InvalidDecidedValue
Drop duplicated value check call as pointed by https://github.com/bloxapp/ssv-spec/issues/325.
The tests for starting an instance with empty value, nil value and invalid value were dropped since the instance doesn't perform a value check anymore.
These tests can't be replicated into
Runner
tests because the input value for starting an instance is defined internally and not by an external argument.Closes https://github.com/bloxapp/ssv-spec/issues/325.