Allow nonpayable functions to override payable functions #11253

frangio · 2021-04-13T19:39:33Z

Abstract

If an interface defines a view function, it is possible for a derived contract to provide a pure implementation of it.

Could it be possible to extend this so that payable functions can be implemented with non payable functions? It is the same kind of state mutability narrowing.

Motivation

OpenZeppelin/openzeppelin-contracts#2610

ERC721's transferFrom is defined as payable in the spec. Our standard implementation is not payable, because if it were payable we would have to define what to do with the message value, which isn't standardized so we don't want to define a specific way to handle it.

Because our implementation isn't payable, we've also defined the interface as non-payable, but people have pointed out that 1) this is more restrictive than the spec, and 2) there is no way to extend our implementation (through inheritance) to make it payable and add logic to handle msg.value.

Specification

A nonPayable function should be a valid implementation of a payable function.

A child contract should be able to extend our nonPayable implementation with a payable function to add a way to handle msg.value before forwarding to super.transferFrom.

The text was updated successfully, but these errors were encountered:

fulldecent · 2021-05-25T02:32:43Z

This is a duplicate of #3412. But that issue was apparently closed prematurely, so I'm glad to see this back from the dead.

chriseth · 2021-05-25T14:23:47Z

I remember that we discussed this, but thinking about it for a minute, I cannot see a reason why this should not be allowed. @ekpyron what do you think?

ekpyron · 2021-05-25T15:07:03Z

I remember that we discussed this, but thinking about it for a minute, I cannot see a reason why this should not be allowed. @ekpyron what do you think?

Apparently, I was for allowing it at some point in #3412 (comment) and @axic half-agreed #3412 (comment), but was still sceptical :-). Without thinking about it too long, I also still don't see a reason why not.

axic · 2021-06-02T13:10:41Z

Tentatively agreed on today's meeting to allow narrowing from payable to non-payable, but keep widening from non-payable to payable disallowed.

Narrowing is similar to the case of view vs pure, with the difference that the compiler inserts code (the check for msg.value == 0), however that code can be already added manually by the user into payable functions.

Widening on the other hand would mean that the expected behaviour (fail on value) is not enforced.

axic · 2021-06-02T13:13:46Z

As a non-exhaustive list of auditors, pinging @GNSPS @montyly @Austin-Williams @stbeyer for feedback.

cameel · 2021-06-02T15:05:56Z

We also discussed marking it as "good first issue". I'm adding the label with "difficulty: medium". The implementation is simple so it should be doable for someone new but will probably require some guidance.

frangio · 2021-06-03T14:19:14Z

How would it work if I defines a payable function, A is I implements it as non-payable, and C is I, A implements it as payable? Is that considered widening?

My first impression is that this should be allowed. C is declaring that it wants to allow receiving value in that function.

I would agree that C is A should not be able to widen it from non-payable to payable.

chriseth · 2021-06-03T14:34:44Z

I would say it's the same as in the case of non-view/view/pure: If a contract strengthens the requirement in the inheritance graph, all derived contracts must also implement the stronger version. In your example, C must implement it as non-payable.

If you declare the function in A as non-payable, then you say that if you are implementing a derived contract that claims to conform to the requirements of A's interface, the function also has to reject all ether transfers.

C is I, A essentially says that C conforms to both I and A.

Why would you say that C is A is different from C is I, A?

frangio · 2021-06-04T20:26:07Z

I see... I think this is due to a different interpretation of what payable and non-payable mean. The situation is not exactly the same as non-view/view/pure, because in that case the default is the least strict (non-view), and with non-payable/payable the default is the most strict (non-payable).

In my mind, payable is the meaningful one and is a way to declare that you want to accept value. But if the compiler sees non-payable as a way to declare that you want to reject value, these are two perspectives that will arrive at different expectations for how overrides should behave.

C is I, A says C conforms to both I and A, but then you could say: a) at least one of I,A wants to accept value so C will accept it, or b) at least one of I,A wants to reject value so C will reject it.

I don't really have a strong opinion here... Although the motivation to open this issue was so that we could have IERC721.transferFrom as payable, then implement the core logic in ERC721.transferFrom as non-payable, while still allowing users to use the core logic and add value-handling logic on top. If this is considered widening and will not be allowed, it won't fix this use case. It may be complicated to allow that scenario in a solid way, so I would understand it.

montyly · 2021-06-10T08:30:48Z

What are your opinions if we add a nonPayable attribute, that would be required a compilation to narrow a function shadowed from payable to non-payable?

Because I can see the case of a developer using a library, shadowing a payable function, and forgetting to keep it payable. So having it explicit helps to understand that its the intent of the developper.

So the rules would be something like:

default: non-payable
payable: only possible if it does not shadow a non-payable function
nonPayable: only way to shadow a payable function without being payable (otherwise same behavior as the default)

However, as the current proposal does not solve OZ problem, and I am not aware of any large project that needs this feature, I am not sure it's worth adding complexity here, and we can just keep the existing rules ;)

fulldecent · 2021-11-13T16:29:14Z

Adding a keyword for nonpayable is not necessary.

Nonpayable is more restrictive than payable and does not introduce dangers. Therefore overriding from payable to nonpayable will not result in people accidentally getting something more dangerous than they expected.

The original issue here is good and should be implemented. Even if "only" OpenZeppelin Contracts and the ERC-721 reference implementation are using this then that should be reason enough to consider this change as impactful.

During the ERC-721 standardization process we identified about 30 language design issues (including this) and most were fixed.

github-actions · 2023-04-05T12:05:12Z