Drop duplicated value check call

[MatheusFranco99]

Drop duplicated value check call as pointed by #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 #325.

[MatheusFranco99]

@GalRogozinski Should we add a check inside StartNewInstance just to check if the value is not empty?

[GalRogozinski]

@MatheusFranco99
for your question... shouldn't this check be on decided?

[MatheusFranco99]

@GalRogozinski
What do you mean by "on decided"? Once the instance decides?
Btw, should this PR be into main or dev?

[GalRogozinski]

@MatheusFranco99
Sorry was being super unclear.
I meant that the check you offered should be on decide() the function that calls StartNewInstance

[MatheusFranco99]

@GalRogozinski
Oh yeah, I agree

[MatheusFranco99]

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?

[GalRogozinski]

@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]

Just a question: we are still covered test wise for empty values?

[GalRogozinski]

👍

[MatheusFranco99]

@GalRogozinski
It seems so according to the test: postconsensus.InvalidDecidedValue