Modifier Areas

[chriseth]

Especially in contracts that model a state machine, it would be convenient to group functions that can be called in a specific state. Currently, the modifiers have to be applied to all these functions individually. This enhancement proposes to provide a syntactic means to apply modifiers to several functions at the same time. This does not only apply to modifiers but also to visibility specifiers. Examples:

contract c {
   // ...
  modifier inState(state) { if (currentState != state) throw; _ }
  using modifier inState(State.transfer) {
    function f() { ... }
    function g() { ... }
  }
  using modifier inState(State.cleanup) {
    function h() { ... }
    function i() { ... }
  }
}

This is also convenient to disallow ether transfer using Emmy Noether's famous modifier (derived from Noether's theorem that describes the connection between preservation of ether and the symmetry of the blockchain):

contract c {
  modifier noether { if (msg.value > 0) throw; _ }
  using modifier noether {
    // put all functions in here that are not supposed to receive ether
    function f() { ... }
  }
}

If using modifier x; (i.e. without curly braces) is used, it applies to the whole contract (including functions before that statement).

[redsquirrel]

Modifier areas would be useful in some cases, particularly short, simple contracts. In other cases, though, they would reduce readability, as well as increasing complexity.

Some contracts are large, and would have correspondingly large modifier areas. This means that while reading a function, the using modifier declaration could be several pages off-screen. In order to understand the behavior of the function, one would have to scroll up, and keep the using modifier context in mind.

Also, for a contract that has many modifiers, it will be tempting to nest modifier areas, increasing the contract's complexity. It would also be necessary to intersperse inline modifiers with modifier areas, adding to the mental overhead of the contract reader.

Ultimately, modifier areas would be a convenience to the writer of the contract, but often a hindrance to the reader.

[chriseth]

@redsquirrel although I agree with your concerns about "having to scroll up", I think it is much easier to verify the semantics for a group of functions that all have a certain modifier than to check that the modifier is present in every single function.
In the end, this all boils down to the fact that authors have to write contracts responsibly. The inheritance feature can also be exploited to pull in effects in a non-local way.

[redsquirrel]

In the end, this all boils down to the fact that authors have to write contracts responsibly.

No doubt about that! 👍

I'm thinking of the authors that are less responsible. This feature will make it easier for them to create complexity aka bugs. 🐜

[redsquirrel]

@chriseth Would you recommend people use modifier areas like this?

contract Foo {
    modifier a() {}
    modifier b() {}

    function x() a() {}
    function y() b() {}
    function z() a() b() {}
}

becomes

contract Foo {
    modifier a() {}
    modifier b() {}

    using modifier a() {
        function x() {}
        function z() b() {}
    }

    function y() b() {}
}

One would have to pick and choose which modifiers get areas and which would have to stay inline. Right?

[kootenpv]

@redsquirrel I think when modifiers are not named a/b but something more concrete, it should quite easily follow which one is the more logical to separate like that. E.g. isOwner vs isState would be divided by isState, and the other would be provided inline. However, it does add some extra thoughts into structuring code. Then again, the gain is to potentially have better structured code and less stacking of modifiers.

It would be nice to also have multiple modifiers at once:

using modifiers inState(State.transfer) isOwner() {
    function f() { ... }
    function g() { ... }
}

Otherwise, we might see horizontal space added and vertical space added when going for larger groups (where the gain could otherwise potentially be even bigger, this looks even worse in my opinion).

using modifier inState(State.transfer) {
    using modifier isOwner() {
        function f() { ... }
        function g() { ... }
    }
}

I'm also concerned about the scrolling issue as well, though.

[redsquirrel]

And what about nested modifier areas?

contract Foo {
    modifier isOwner() {}
    modifier isState() {}
    modifier noEther() {}

    function x() isOwner() {}
    function y() isOwner() isState() {}
    function z() isOwner() isState() noEther() {}
}

...could become...

contract Foo {
    modifier isOwner() {}
    modifier isState() {}
    modifier noEther() {}

    using modifier isOwner() {
        function x() {}
        using modifier isState() {
            function y() {}
            function z() noEther() {}
        }
    }
}

[chriseth]

Of course, both nested areas and multi-modifiers will be possible.

[androlo]

I see this would not allow adding functions with different kinds of return values to the same group modifier. Maybe not a big issue, but usually when I make systems of contracts I like to return values from functions (constant or not) that indicate to calling contracts whether or not the call was successful. A common case would be when contract A calls a method on contract B, then a method on contract C, but only if the call to B was a success.

[redsquirrel]

@androlo I don't think modifier areas have any effect on return values.

[androlo]

Ok. No issue then.

[androlo]

Ah, IC. So basically they are all inlined automatically, or I guess in the case of big nasty modifiers maybe called simply as internal functions. I was thinking in terms of old LLL where you wrapped functions inside modifiers (which were basically just an 'if' or 'when' conditional).

I need to start using modifiers...

[VoR0220]

perhaps there could be something in the natspec that specifies what modifiers the contract is using?

[axic]

Wouldn't it be nice sorting out some of the issues raised in #49 before making it a bit more complex?

[axic]

This can be extended with visibility areas: #2608 (comment)

[github-actions]

This issue has been marked as stale due to inactivity for the last 90 days.
It will be automatically closed in 7 days.

[ekpyron]

I'm giving this one more iteration for deciding if we're likely to do this any time soon before letting it go fully stale. There is cause for this and it came up recently elsewhere - but it'll probably take us a while to get to it.

[github-actions]

This issue has been marked as stale due to inactivity for the last 90 days.
It will be automatically closed in 7 days.

[github-actions]

Hi everyone! This issue has been automatically closed due to inactivity.
If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen.
However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.