Revert with error message when release validation fails

[cgewecke]

#13, #15.

• modifies the ReleaseValidator contract to return an integer error code if validation fails (current behavior is to return boolean)
• defines an integer code -> human readable message mapping in the validator contract.
• modifies PackageIndex to revert with error message when release validation fails (current behavior is to silently succeed without executing the release.)
• update tests to check for revert and appropriate error message.

This PR targets the allow-backfilling branch because they touch the same code / code has been removed in that PR.

NB: it might be cheaper to have the validator just return the strings? Not sure this implemented optimally at all. . .

[coveralls]

Pull Request Test Coverage Report for Build 105

• 9 of 9 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.4%) to 97.175%

---

Totals
Change from base Build 87:	0.4%
Covered Lines:	385
Relevant Lines:	391

---

💛 - Coveralls

[pipermerriam]

These error numbers seem like they should be an Enum. That would make reading this code a lot easier.

[cgewecke]

Ah yes nice! 🙂

[pipermerriam]

Maybe instead of returning the code, the revert should just be thrown from within the releaseValidator? That way you don't have to juggle a return code.

[cgewecke]

@pipermerriam Agree - that's a more elegant solution and works perfectly for the case where you're attempting a release and have an error.

There's a small tradeoff - if you call validateRelease on its own - say as a pre-flight check - the errors don't propagate because it's a view function. It can be structured to correctly return true/false for that case though.

Your suggestion is simpler/cleaner (probably lighter) ... on reflection definitely prefer.

@gnidan WDYT?

[gnidan]

So right now the benefit is that errors propagate easily via eth_call...

To continue to get errors to propagate, we could just leave it as a client concern (say, sendTransaction to the view function on a forked network). But this is messy and heavy-handed in implementation...

It does make it a lot simpler in Solidity, to revert with errors inside ReleaseValidator. To get a pre-flight check we'd need another method, though, one that doesn't error opaquely.

I'm not sure. I don't feel strongly either way, but I guess my vote is that pre-flight checks are valuable enough to warrant providing that capability in some form, even if it means a bit of ugly code somewhere. But I'll abstain from voting on where the ugly code to support this should live.

[pipermerriam]

TLDR: Do what you think is best.

Lately I've structured my APIs to look something like this:

is_valid_thing()  # returns `true/false`
validate_thing()  # throws error

I find I almost always want both of these and it is easy to make one a wrapper around the other. In this case it isn't trivial because we don't have good error handling so you might need a 3rd internal method which returns a status code which can then be converted to either a boolean, or an exception.

[cgewecke]

@pipermerriam Yes, makes sense. For now opting to revert from the validator per suggestion because its cleaner/less weird. Will possibly add a second is_valid_thing -like method that returns error string or "ok" for pre-flight checks in a later PR. After the revisions in 61e356a the behavior is:

• reverts with error message on send
• returns true/false on call

[gnidan]

Apart from my mostly-useless thoughts on how to error, this looks good to me.

[gnidan]

So right now the benefit is that errors propagate easily via eth_call...

To continue to get errors to propagate, we could just leave it as a client concern (say, sendTransaction to the view function on a forked network). But this is messy and heavy-handed in implementation...

It does make it a lot simpler in Solidity, to revert with errors inside ReleaseValidator. To get a pre-flight check we'd need another method, though, one that doesn't error opaquely.

I'm not sure. I don't feel strongly either way, but I guess my vote is that pre-flight checks are valuable enough to warrant providing that capability in some form, even if it means a bit of ugly code somewhere. But I'll abstain from voting on where the ugly code to support this should live.