Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ideally, we should use modifiers for function input checks #197

Open
loredanacirstea opened this issue Jul 30, 2018 · 2 comments
Open

Ideally, we should use modifiers for function input checks #197

loredanacirstea opened this issue Jul 30, 2018 · 2 comments
Labels
enhancement New feature or request needs discussion

Comments

@loredanacirstea
Copy link
Contributor

loredanacirstea commented Jul 30, 2018

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: ethereum/solidity#3060 (comment)

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 loredanacirstea added the enhancement New feature or request label Jul 30, 2018
@loredanacirstea loredanacirstea added this to the Red Eyes milestone Jul 30, 2018
@loredanacirstea loredanacirstea self-assigned this Aug 2, 2018
@LefterisJP LefterisJP added the P3 keeping somebody waiting possibly label Aug 3, 2018
@loredanacirstea
Copy link
Contributor Author

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 #215

@LefterisJP ?

@LefterisJP
Copy link
Contributor

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

@LefterisJP LefterisJP modified the milestones: Red Eyes, Ithaca Aug 10, 2018
@pirapira pirapira removed this from the Ithaca milestone Jan 22, 2019
@pirapira pirapira added needs discussion and removed P3 keeping somebody waiting possibly labels Jan 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs discussion
Projects
None yet
Development

No branches or pull requests

4 participants