Remove payable modifier for functions

[hrkrshnn]

Specification

• Remove the payable modifier for functions.
• All functions, starting from, say 0.9.0, can receive ETH by default (i.e., the current semantics of payable functions).
• Backwards compatibility: Users may define and use a nonPayable modifier to make functions revert when ETH is attached to the call.

   modifier nonPayable {
         require(msg.value == 0)
         _;
   }

  Note: there is a subtle difference between nonPayable modifier and the current (< 0.9.0) functions without payable. See following section.

Context

1. The payable modifier is getting less relevant nowadays and is being used to save gas (some context from twitter).

2. The default payable modifier has subtle semantic difference with modifiers.
   For example, if a public function f has the modifier m, calling it externally and internally will use the modifier. However, for functions that are not payable, the check is only relevant when calling externally. In the following example:

       contract C {
             function f() public {
             }
             function test() external payable {
                   // an internal call to f. Will not check `msg.value == 0`.
                   f();
             }
       }

   This can introduce subtle bugs. For example, callvalue in inline assembly (same as msg.value) is typically used to save 1 gas, when compared to push 0. It is false that you can replace 0 by callvalue in inline assembly for public non-payable functions, as one can call it internally from a payable function.

[SamWilsn]

As an alternative, what if payable continues to exist, but not using it no longer inserts the check for external functions?

The behaviour would become consistent between internal/external calls.

Additionally, you could ban msg.value and the CALLVALUE opcode in non-payable functions (or just emit a warning.)

[ekpyron]

For the record: payable or nonpayable does not apply to internal functions, but only to external ones. I.e. there are no payable internal functions. That also justifies the behaviour of internal calls to public functions. msg.value and callvalue() are already only allowed in either internal functions or payable functions, so that also makes sense.

Also I see no reason to consider the check any more or less relevant than it used to be. It still solves an actual problem - you can still send and lock in ether without the check, so it is still the safe default behaviour, especially if you're always free to choose the unsafe alternative. Given all that I don't think this proposal will get majority support.

[cameel]

We discussed it on the call today:

• The gas saving is really minuscule. Even if raw ether usage is going down, people can still lose it by sending to the wrong address and this protection does not seem overly expensive for what it provides. The prevailing opinion in the team was that, if the cost is the only reason, we're better off leaving it as is.
  • Perhaps it's getting so much attention because it's just a very easy saving, even if small.
• Still, if people really want to bypass the check, they will, one way or another. We see that as a bigger problem. We don't want the popular recommendation to be to just apply payable to everything. The check should not be forced on devs who really don't want it but at the same time we don't want to take away the protection from devs who still do. Defaults are powerful.
  • We do not have an agreement on how to best deal with that though. We could provide a mechanism to explicitly disable the check, but that would still leave the check by default.
  • The idea originally came from Twitter and the sentiment was generally that the feature is useless and also a bit dismissive towards users who might face this problem. We're not sure if that's really representative of the general opinion.
• Removing payable just from constructors might be less controversial. In that case payable is much less useful because it's protecting only the deployer and the call can happen only once. The biggest downside is that built-in modifiers would work differently on constructors than on other functions but we have already deviated from that by removing constructor visibility.

[drortirosh]

can add payable at the contract level, to imply all methods explicitly defined in this contract are payable

[frangio]

We're not sure if that's really representative of the general opinion.

I can speak for myself: it is absolutely not representative of my opinion. Opt-in payable serves a purpose, it is not a dumb waste of gas that the compiler is inflicting on us. It should never be used as a gas-saving measure, except by MEV searchers or in other niche areas.

This is 24 gas we're talking about. In the context of a 50000 gas ERC20 transfer, this is 0.05% of the entire transaction, and this is not an optimization that scales with the complexity of the code, so in many cases it will be even lower than that.

[kraikov]

To put it in a different perspective.

~1M transactions * 24 gas = ~1.2 Ethers saved each day (considering the gas price is 50 gwei)

[maxsam4]

We could provide a mechanism to explicitly disable the check, but that would still leave the check by default.

A compiler flag would be ideal IMO.

The prevailing opinion in the team was that, if the cost is the only reason

Another major reason (why I originally did this and realized what's going on) is the batch call functionality that is being adopted more and more in the real world. eg Uniswap's multicall. Here, if you want to send ether to a batch call, all functions must be declared payable, not just the function that uses the ether. Like if I want to batch the hypothetical functions buyTokenWithEther and stakeToken, both buyTokenWithEther and stakeToken must be defined as payable though only buyTokenWithEther actually uses the Ether.

The idea originally came from Twitter and the sentiment was generally that the feature is useless and also a bit dismissive towards users who might face this problem.

Twitter is all about polarizing opinions so don't mind the aggressive behavior. However, I still believe that this protection does not really do much. I stand by my point that it is super hard to transfer ether to a contract while calling a function. Even harder to do it mistakenly :). I agree that the fallback should be non-payable by default so if someone just sends ether to it, it reverts. I have not come across a case where someone mistakenly sent ether to a contract while calling a function that should not be taking ether.

[k06a]

Don't forget if someone would mistakenly send some ETH by pressing the wrong button in MetaMask, this ether will not only be stuck on the contract, but this person will also pay 9000 of gas extra for the call with msg.value > 0. I am not sure 24 gas is worth introducing such a potential issue. I am afraid all the developers will be forced to use nonPayable for most of the methods instead of having it by default.

[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.