Assess keeping invariants grouped

[loredanacirstea]

(must be updated with a proper description)

As mentioned in this thread: #39 (comment)

keeping the invariants grouped? I think it is easier to verify them if we don't need to jump around in the contract code.
IMO this should not take priority over clarity if the input is invalid, only in cases where the input is valid but a race happened (e.g. two monitoring services called update at the same time, only one of the transactions will work)

[LefterisJP]

Just my 2 cents. I also would really like to have all the require invariants somewhere. This would make each function so much easier to read and reason about.

[loredanacirstea]

Yes, I thought about this and I think I went too far with making sure the gas cost is minimal.
So, I would stick to using require strictly for input values, at the beginning of the function (as the official Solidity docs recommend anyway) and move all other invariants in asserts at the bottom of the function.
Does this sound good?

[LefterisJP]

Grouping the invariants is fine but you should only use asserts for internal errors and things that should never ever happen. I am commenting on #64 about it.

Docs which help us understand the different between require() and `assert(): http://solidity.readthedocs.io/en/v0.4.23/control-structures.html?highlight=assert#error-handling-assert-require-revert-and-exceptions

[hackaugusto]

Just to clarify my point, code should be written to be readable first. I don't have a hard rule for how to format code for readability, specially because of solidity's constraints on the number of variables in a stack, which require intermediary functions to be introduced, which may hide validation inside these functions.

But I would strive for a contract-based approach, where the beginning of a function starts with the requirements, first validating/requiring the value inputs, then the code is followed by the pure computation, which is followed by the validation of the computation and side-effects, and the code finishes with the asserts/invariants that must be true after the execution of the code.

[loredanacirstea]

But I would strive for a contract-based approach, where the beginning of a function starts with the requirements, first validating/requiring the value inputs, then the code is followed by the computation, which is followed by the validation of the computation and external side-effects, and the code finishes with the asserts/invariants that must be true after the execution of the code.

I fully agree with this.
In my opinion this is how you get more secure code & more readable.
Especially more secure code. When you group all the post-state-change (computation) checks to the end of the function it is way harder to make sure that that variables did not change their state in between the computation and the check. Sometimes you need to change them and you end up needing to store the initial state in another local variable (very ugly).

I think @berndbohmeier also agrees with this.

[loredanacirstea]

I reopened the issue. The checks should follow the above proposed format:

• check function inputs
• check other state changes preconditions
• perform actions and immediately check if these action succeeded
• check all other invariants (asserts) at the end

This issue will be closed when the above format is implemented.

[loredanacirstea]

#248 implements the agreed changes.