Support modifiers defined by user

[xgreenx]

It will be cool to have some macro attribute in #[ink(message)] declaration which will automatically paste the code which will do some restriction check like in modifiers in Solidity.

We can discuss here how it can be implemented=)

[Robbepop]

I am not against this feature if we can make sure that we can deliver an implementation for ink! that is actually useful and does not introduce new footguns for users.
I generally like the idea of descriptive programming and therefore think that modifiers like the ones in Solidity can improve code quality.
However, since ink! is not a whole compiler but just a set of macros and a set of libraries it will be harder to properly implement this feature I suppose.

Still, I would like you to make suggestions about design and we will see how far we will eventually go along this road.

[xgreenx]

In Solidity, the user must define a modifier. It means that the user also must do that in case of ink!. We can require him to define the private modifier(function) in impl section. It means that we can call this function self.#ident_of_fundtion; before the execution of any method.

We created an example of an independent macro, which will paste self.#ident_of_fundtion; at the beginning of the function's block(in macro you can pass the array of ident of functions). If the function's block is absent, it will do nothing. I think it must be enough for many cases, but we need to add the right support for the case of usage in trait definition. If the user marked function with a modifier in a trait definition, we also would check that he used the same modifiers in impl section(I think the same behavior must be for the payable function).

Also maybe we need to think about cases when modifier returns a result with error and paste self.#ident_of_fundtion?;. But we decided to use assert and panic in our library based on this thread, so it is ok for us but maybe its not for ink!.

[xgreenx]

But it is an example of an independent modifier and it has low functionality. We can't mark where we want to paste the body of the function(In solidity you can use '_;' construction to indicate that).

Suggested solution

We can require the user to define the modifier in the body of the contract and mark it like ink(modifier).
The user must provide inside of the function a #[ink(body_of_function)] attribute, which indicates where we need to paste the main code of the function.
After, when ink! parses the mod, it first will process every ink(modifier) and extract the code of it to the hash map. Then during parsing of methods if some method has attribute #[ink(modifiers(...))], it will parse it and try to use the code of modifier from the hash map for each ident in macro. If some modifier is absent, it will throw an error.
A solution like this will resolve problems of independent modifier + will provide full functionality from the Solidity.

It is an example of how ReentrancyGuard can be implemented with this feature.

#[ink(modifier)]
fn non_reentrant(&mut self) {
    assert_eq!(self._status, _NOT_ENTERED, "ReentrancyGuard: reentrant call");
    self._set_status(_ENTERED);
    #[ink(body_of_function)];
    self._set_status(_NOT_ENTERED);
}


#[ink(message)]
#[ink(modifier(non_reentrant))]
fn swap(&mut self) {
    ...
}

[xgreenx]

But it is an example of an independent modifier and it has low functionality. We can't mark where we want to paste the body of the function(In solidity you can use '_;' construction to indicate that).

Suggested solution

We can require the user to define the modifier in the body of the contract and mark it like ink(modifier).
The user must provide inside of the function a #[ink(body_of_function)] attribute, which indicates where we need to paste the main code of the function.
After, when ink! parses the mod, it first will process every ink(modifier) and extract the code of it to the hash map. Then during parsing of methods if some method has attribute #[ink(modifiers(...))], it will parse it and try to use the code of modifier from the hash map for each ident in macro. If some modifier is absent, it will throw an error.
A solution like this will resolve problems of independent modifier + will provide full functionality from the Solidity.

It is an example of how ReentrancyGuard can be implemented with this feature.

#[ink(modifier)]
fn non_reentrant(&mut self) {
    assert_eq!(self._status, _NOT_ENTERED, "ReentrancyGuard: reentrant call");
    self._set_status(_ENTERED);
    #[ink(body_of_function)];
    self._set_status(_NOT_ENTERED);
}


#[ink(message)]
#[ink(modifier(non_reentrant))]
fn swap(&mut self) {
    ...
}

We implemented the described modifiers in the library. I hope the comments are clear enough to understand how to use them.

It is the implementation of ReentrancyGuard modifier and example of usage.

If you are okey with this concept, we can adapt the change for ink! and create a PR.

[tk-o]

On the other hand, a modifier is just a function that asserts certain state, and panics if it's not an expected one.

Given that characteristic, why can't we just have an explicit function call?

fn non_reentrant(&mut self) {
    assert_eq!(self._status, _NOT_ENTERED, "ReentrancyGuard: reentrant call");
    self._set_status(_ENTERED);
    #[ink(body_of_function)];
    self._set_status(_NOT_ENTERED);
}

#[ink(message)]
fn swap(&mut self) {
    self.non_reentrant();
    ...
}

In case there's a need for enforcing calling a modifier function with swap, we could have an abstract implementation provided on a trait, that would call the expected modifier functions first, and then call the private/non-public function with the target business logic.

The only problem here is that ink! does not support trait functions carrying their body.

I did try implementing the Ownable trait, and that was an issue I faced: I did want certain function (a.k.a. a modifier) to be called before another function, but I couldn't do that, as ink! would not allow it.

Here's a code example: _only_owner is the function I wanted to call first before going down the execution path of the renounce_ownership, or transfer_ownership functions.

What do you think of having this approach (trait-level default methods requiring using some modifier function first)?

[xgreenx]

Given that characteristic, why can't we just have an explicit function call?

We can do an explicit function call, but the problem is how you will pass the body of your main function inside of the modifier?

The first simple suggestion about modifiers in this issue was about adding of explicit call at the beginning of the function.

#[ink(message)]
#[ink(modifiers(non_reentrant))]
fn swap(&mut self) {
    ...
}

Expanded into:

#[ink(message)]
fn swap(&mut self) {
    self.non_reentrant();
    ...
}

But feature to specify the place where we want to call the code of the main function is very important. So the initial approach is not good as a final solution.

We implemented the described modifiers in the library. I hope the comments are clear enough to understand how to use them.
It is the implementation of ReentrancyGuard modifier and example of usage.
If you are okey with this concept, we can adapt the change for ink! and create a PR.

We redesigned the modifiers and now they are independed and we will do explicit call of modifiers after code expantion.
Here is example of how modifier must be declareted. And here we described the list of rules which you need to follow during declaration.

Also you can pass arguments into modifier.
The implementation is using closures and here you can check how macro will expand the code.

The only problem here is that ink! does not support trait functions carrying their body.

Yes, ink! doesn't support default implementation inside of traits at the moment. But we hope that they will fix it in a new version of trait definition. We also want to use a default implementation inside of traits and we are tracking it in issue.

As a workaround, we added support of default implementation in OpenBrush's macros. But we found some problems with the current implementation and we will re-implement that next week. But usage of our default trait definition will require you to use our version of ink!(and macros), because it is using this feature.

[SkymanOne]

Hey. Can someone post an update on the issue, please?