Ideally, we should use modifiers for function input checks

[loredanacirstea]

Using modifiers is also recommended by the official Solidity docs: https://solidity.readthedocs.io/en/v0.4.24/miscellaneous.html?highlight=modifier#tips-and-tricks https://solidity.readthedocs.io/en/v0.4.24/common-patterns.html?highlight=modifier#restricting-access

Using modifiers clarifies in what contract state can a function be called and makes it easier to verify if a function has all the needed safe guards implemented. Same thing applies for common input checks across functions.

In theory, we could have the following modifiers for example:

// These checks are buried in `getChannelIdentifier` instead of being clear on every function that needs it.
modifier areParticipantsValid(address participant, address partner) {
        require(participant != 0x0);
        require(partner != 0x0);
        require(participant != partner);
        _;
}

modifier isValidChannelIdentifier(uint256 channel_identifier) {
        require(channel_identifier > 0 && channel_identifier <= channel_counter);
        _;
}

// For making sure the channel targeted by the transaction is the one currently used and valid
// After  https://github.com/raiden-network/raiden-contracts/pull/198 is merged
modifier isCurrentChannelIdentifier(uint256 channel_identifier, address participant, address partner) {
        require(channel_identifier == getCurrentChannelIdentifier(participant, partner));
        _;
}

modifier isChannelOpen(uint256 channel_identifier) {
        require(channels[channel_identifier].state == 1);
        _;
}

modifier isChannelClosed(uint256 channel_identifier) {
        require(channels[channel_identifier].state == 2);
        _;
}

If we use modifiers, they must be used consistently in the code. This is not possible at the moment, due to the stack too deep error that happens in settleChannel, updateNonClosingBalanceProof, cooperativeSettle. This is because the stack is not cleared after the modifier code has ran. However, this might be mitigated up to a point in the near future: https://github.com/ethereum/solidity/issues/3060#issuecomment-408830192

Even with these constraints, I think it is worth looking into the aforementioned functions to see if we can improve the code (use the stack more efficiently), in order to have these modifiers.

[loredanacirstea]

As explained in the above description, we cannot use modifiers with the current contracts. I would move this issue to Ithaca. It will probably be solved after https://github.com/raiden-network/raiden-contracts/issues/215

@LefterisJP ?

[LefterisJP]

Sounds good @loredanacirstea. So seems the splitting detailed in #215 is gonna be rather needed.