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

Feature to apply modifier to all functions #4990

Closed
chriseth opened this issue Sep 17, 2018 · 5 comments
Closed

Feature to apply modifier to all functions #4990

chriseth opened this issue Sep 17, 2018 · 5 comments
Labels
high impact Changes are very prominent and affect users or the project in a major way. language design :rage4: Any changes to the language, e.g. new features medium effort Default level of effort needs design The proposal is too vague to be implemented right away

Comments

@chriseth
Copy link
Contributor

Certain use-cases require a check to be applied to all or at least many functions. Most notably, this includes a general smart contract deactivation function (as replacement for dangerous selfdestruct), a "pausable" pattern and so on.

Modifiers already provide basic support, but it is too easy to forget a modifier to be mentioned with a single function, also it requires quite some typing and can get cluttered fast.

Modifier areas ( #623 ) are a similar solution, but they do not work with inheritance.

This proposal tries to start a discussion about how modifier areas could work better with inheritance. The drawback of this proposal is that it provides less choice about which functions to apply.

Syntax proposal:

using modifier <modifier> for *;

This applies the given modifier to all functions in this contract, including inherited functions and all functions in potential derived contracts.

It might make sense to restrict this to functions that are non-view functions.

Potential syntax: using modifier <modifier> for payable, non-payable;

@chriseth chriseth added the language design :rage4: Any changes to the language, e.g. new features label Sep 17, 2018
@0xAnonymous
Copy link

I'm writing a contract that could benefit from this. I was calling the idea "default modifier" before I found this thread via your thread on modifier areas.

@0xAnonymous
Copy link

I no longer need "default modifier" because changed approach. So will likely not have meaningful input to this issue, since not in self-interest for dApp I work on.

@NunoFilipeSantos NunoFilipeSantos added the stale The issue/PR was marked as stale because it has been open for too long. label Nov 25, 2022
@cameel
Copy link
Member

cameel commented Jan 6, 2023

I'm removing the stale tag because I think we should really come back to this topic when we have more time. Another important use case for this would be applying a reentrancy guard to all functions in a contract (as mentioned in #13845).

@cameel cameel added medium effort Default level of effort high impact Changes are very prominent and affect users or the project in a major way. needs design The proposal is too vague to be implemented right away and removed stale The issue/PR was marked as stale because it has been open for too long. labels Jan 6, 2023
@alex-ppg
Copy link

alex-ppg commented Jan 7, 2023

I have added some additional background on #13845 as to why this change would not satisfy a blanket reentrancy guard properly, however, if the modifiers can be applied with a proper distinction between mutable and non-mutable functions then this would be a good way to start.

A core problem I see with this syntax is its compatibility with inheritance. Libraries (such as OpenZeppelin) will be hesitant to apply a global re-entrancy protection across all their functions, meaning that users would have to specify the using modifier <modifier> for * syntax as well as override each and every function of the dependency they wish to apply the modifier for.

On the other hand, due to dependency conflicts certain functions would need to be overridden without any functional changes (i.e. using super) but the using modifier <modifier> for * syntax would affect them inadvertently, a side-effect most developers will not expect.

For the re-entrancy use case in particular I believe this feature would be improperly used and cause more harm than good (i.e. applying using modifier nonReentrant for * and assuming that all functions are non-reentrant when they are not).

Additionally, the feature as it is would cause nuisances when it comes to inheritance as any overridden function would apply the blanket modifier even when that may not be the intention (i.e. the developer is forced to override due to an interface & implementation conflict).

Any solution to the inheritance problem would significantly increase the complexity of how the using modifier <modifier> for * is applied and would lead to the syntax becoming difficult to grasp and seldom used as developers would be unsure as to how it behaves.

As a final note, the issue also suffers from how modifier conflicts are resolved when it comes to the visibility of functions. Traditional modifiers can be applied to both non-external and external functions, however, this syntax assumes that they are only applied to external functions.

If we extend the syntax to also accept explicit visibilities, it would significantly complicate how it is used (i.e. using modifier nonReentrant for mutable,external,public; and using modifier nonReentrantView for nonmutable,external,public;) as well as cause conflicts when an external function invokes a public function and so on.

To conclude, we can alleviate all concerns above by specifying thorough documentation as to how the syntax behaves (i.e. visibility needs to be explicit, both external and public will apply the modifier if invoked in a single context, etc.), however, it would be significantly complex to use correctly leading to more developer pitfalls being introduced to the language rather than covering them up.

@ekpyron
Copy link
Member

ekpyron commented Feb 7, 2023

If anything I think we'll rather go the route of #623 for solving the underlying request, and would allow to properly distinguish between functions explicitly, which I take it would satisfy the concerns by @alex-ppg.
That being said, I don't see us doing that any time soon either.
Closing this issue for now in any case, since this is not planned.

@ekpyron ekpyron closed this as completed Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high impact Changes are very prominent and affect users or the project in a major way. language design :rage4: Any changes to the language, e.g. new features medium effort Default level of effort needs design The proposal is too vague to be implemented right away
Projects
None yet
Development

No branches or pull requests

6 participants