ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
23.03k stars 5.7k forks source link

Don't restrict use of callvalue in assembly #6581

Closed rainbreak closed 5 years ago

rainbreak commented 5 years ago

From the 0.5.7 changelog:

Inline Assembly: Issue error when using callvalue() inside nonpayable function (in the same way that msg.value already does).

This is a fairly annoying change for libraries that use callvalue in a general setting as they now have to specialise into payable and non-payable forms.

The restriction on use of msg.value was perhaps reasonable, but restricting specific opcodes in inline-assembly is overly restrictive.

chriseth commented 5 years ago

I tend to agree here, also not checking callvalue does not "hurt" too much.

axic commented 5 years ago

I think there are two cases here: 1) checking call value in an externally visible function 2) checking call value in an internal helper

In case 1) it is probably not what you want to do, because the function guarantees (when externally executed) to always have a call value equal to 0.

In case 2) it may be a helper (including a modifier) where it may be useful. Perhaps we should allow this for case 2) -- in internally visible functions and modifiers.

As noted by @chriseth on the weekly meeting, this feature is about warning the user about doing something unintended. It would also be a good option to only issue warning for call values, but a long standing debate is that if warnings are issued there must be a way to disable them and we do not have a way to disable warnings.

chriseth commented 5 years ago

If we move the check to "external function only" - does it also apply to public functions? We could do that and mention in the message to create an internal version of the function that is called by the public (or then external) one.

rainbreak commented 5 years ago

@axic a warning would be ok, although it is still fairly annoying being warned about assembly blocks and a way to disable assembly related warnings would be great. The problem currently here is that it is an error.

I'm not sure I see a simple way to split cases 1 and 2, without introducing some restrictions on the use of assembly callvalue.

chriseth commented 5 years ago

@rainbreak I agree that a warning is not much better than an error. I didn't quite understand the second part of your message - would it solve your problem if we only issue an error in assembly blocks in external functions (and not in modifiers)?

rainbreak commented 5 years ago

@chriseth my problem would be solved if use of callvalue in assembly was possible in all cases. I think that payable / nonpayable should be enforced at run-time, not compile-time.