7

I've read this answer, which is good in general, but it doesn't quite tell me when to use a modifier (at the function declaration) and when to use assert/require (inside the function).

I do understand that this is basically a semantic question, since both options will yield the exact same runtime impact (because modifier is essentially a syntactic sugar).

But because I wish to avoid comments when publishing my contracts, I would like to know whether or not there is a widely-accepted convention for when to use each one of the two options.

I have read somewhere that one should use modifiers only for state-verification and never for input-verification, but I'm not really sure that this is indeed the convention.

My current approach is to use modifiers only when it enhances readability, since they have no other impact whatsoever (in particularly no runtime impact). My thumb-rule for enhanced readability is when the overall code size in text becomes smaller (i.e., when the number of usages "justifies" the modifier implementation).

Thank you!!!

1
  • Looking for...... Commented Feb 25, 2019 at 12:48

2 Answers 2

4
+100

1) Your consideration about readability improvement is a very much important aspect of modifiers use. As far as I know, it is THE most important.

2) They are currently used for inputs and state variables check, without differentiation.

3) Given this, another consideration you can add is that modifiers are also inheritable contract properties, that can be overriden by contracts deriving from the root contract.

This means that everytime you need to implement a repetitive pattern of check (without regarding about state variable or input, they are used for both) but you need a slight different implementation detail in particular areas of your code, you can have a very positive impact on code readability using slight different implementation in the various contracts.

For instance if you store the "owner" variable in a root contract, the same "onlyOwner" modifier can be implemented in the root contract itself and overridden in the leaf contracts, aiming to have the check vs the local "owner" variable in the root contract, vs. the root.owner() public getter return value in the leaf contracts.

This aspect can be found described here (bitdegree.org)

-> EDIT (AFTER COMMENTS):

1) you should not read the rule as regarding text size, but readability. Often you can use a slight longer definition for readability, it is not something strictly related to text characters count:

uint currentHeapSize; // is more readable than
uint cHSz;            // but it is longer by far.

In the same way if the modifier impact is POSITIVE in terms of readability, it is the case to use it. If it is negative or neutral, better to omit it. For instance Consensys best practices simply says:

"Use modifiers to replace duplicate condition checks in multiple functions, such as isOwner(), otherwise use require or revert inside the function. This makes your smart contract code more readable and easier to audit."

2) The problem is not related to the kind of data you are checking, but the side effect of the checking. You can find any kind of check in standard implementation, even if the more frequent is the environmental/transactional check (who is the msg.sender? is the msg.value > 0? is the current block number > of..?). For instance Consensys reccomend to use modifiers only for assertions. The fact is that the code inside a modifier is usually executed before the function body, so any state changes or external calls will violate the Checks-Effects-Interactions pattern. Moreover, these statements may also remain unnoticed by the developer, as the code for modifier may be far from the function declaration. For example, an external call in modifier can lead to the reentrancy attack:

contract Registry {
    address owner;

    function isVoter(address _addr) external returns(bool) {
        // Code
    }
}

contract Election {
    Registry registry;

    modifier isEligible(address _addr) {
        require(registry.isVoter(_addr));
        _;
    }

    function vote() isEligible(msg.sender) public {
        // Code
    }
}

In this case, the Registry contract can make a reentracy attack by calling Election.vote() inside isVoter().

See "Use modifiers only for assertions" in Consensys Best Practices

3) I appreciate very much a simple article how-to-write-clean-elegant-solidity-code-using-function-modifiers, which concludes: "It might take a few iterations to achieve and using modifiers won’t magically make your code legible. But when used with care, they become a powerful tool to tame complexity and leverage reusability, making your code cleaner and safer."

4) About assertion (see comments): you could have any kind of code in modifiers. For instance you could think to have a (not reccomended!) modifier which calculates the mean value of a vector of elements without check nothing.

For instance you could think to define a

modifier makeMeanBefore {
// here code which calculates the mean value of the current values in a vector elements and do not checks
// any condition, just calculate the mean value and set it in a variable
_;
}

and to define somewhere

function readVectorMean() makeMeanBefore {
...
}

where makeMeanBefore is a modifier, but not implements any assertion; this is what is to be avoided (by newbie, I suppose...)

10
  • 1
    Edited answered to reply.
    – Rick Park
    Commented Feb 25, 2019 at 15:51
  • 1
    Are you satisfied by my answer? 😉
    – Rick Park
    Commented Feb 27, 2019 at 14:13
  • 1
    Yes, very satisfied to be honest! Commented Feb 27, 2019 at 14:32
  • 1
    I’m very happy to gain your bounty, then! At the moment apparently you did not mark the answer as accepted... 😬
    – Rick Park
    Commented Feb 27, 2019 at 19:33
  • 1
    Let's wait for it to (almost) end; You're my first candidate. Commented Feb 28, 2019 at 7:08
2

I typically role up repetitive code in either

a) a modifier

or

b) a function

If I want to follow the best practices of failing early (to save gas costs) a modifier it is.

This enhances readability and reduces LOC.

If I find that I'm using the same modifiers a lot, I role those up into a common Utils.sol contract and import that.

I suppose, though I haven't analyzed it myself, a function introduced at the first line of another's function body would serve the same purpose.

I would take a look at OpenZeppelin's code base if you want concrete examples if you haven't already done so.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.