Raise exception on internal payable function declaration

[benber86]

What I did

Added an exception when a function is declared as both payable and internal as reported in #3099 (pure functions are also mentioned in the issue but are now already prevented from being declared payable).

How I did it

Check function visibility and state mutability during validation and raise a FunctionDeclarationException exception if a function is found to be both payable and internal.

How to verify it

Run the test_invalid_conflicting_decorators test added to tests/parser/features/decorators/test_payable.py.

Commit message

Raise exception on internal payable function declaration

Description for the changelog

Prevent internal functions from being declared as payable

Cute Animal Picture

[codecov-commenter]

Codecov Report

Merging #3123 (3077d2d) into master (f4deb13) will decrease coverage by 21.12%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           master    #3123       +/-   ##
===========================================
- Coverage   88.49%   67.37%   -21.13%     
===========================================
  Files          95       95               
  Lines       10759    10757        -2     
  Branches     2265     2201       -64     
===========================================
- Hits         9521     7247     -2274     
- Misses        796     2867     +2071     
- Partials      442      643      +201     

Impacted Files	Coverage Δ
vyper/semantics/types/function.py	66.29% <0.00%> (-20.88%)	⬇️
vyper/builtin_functions/utils.py	0.00% <0.00%> (-100.00%)	⬇️
vyper/ir/s_expressions.py	5.88% <0.00%> (-88.24%)	⬇️
vyper/ast/natspec.py	12.34% <0.00%> (-86.42%)	⬇️
vyper/cli/utils.py	16.66% <0.00%> (-71.43%)	⬇️
vyper/compiler/utils.py	30.00% <0.00%> (-70.00%)	⬇️
vyper/cli/vyper_json.py	9.40% <0.00%> (-69.69%)	⬇️
vyper/compiler/output.py	27.74% <0.00%> (-61.26%)	⬇️
vyper/semantics/validation/data_positions.py	36.55% <0.00%> (-54.84%)	⬇️
vyper/builtin_functions/convert.py	39.42% <0.00%> (-51.62%)	⬇️
... and 52 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[charles-cooper]

i'm not convinced this is a good solution. can msg.value be accessed from internal functions?

[charles-cooper]

i think a better solution is to allow @payable on internal functions, and only allow calling payable internal functions from payable external functions.

[pcaversaccio]

IMHO the modifier payable should not be used in combination with internal functions due to semantics. The goal of a payable modifier is to allow or prevent intentional and accidental value transfers, and using it twice, first in an external function and then again in an internal function called by that function, does not really make sense. The payable modifier should be exclusively allowed in combination with the external modifier. Also, wrt composability, you could have internal functions that check for msg.value and if =0 perform another logic than >0. It should be possible to call that internal function if msg.value = 0 without decorating the external function with payable (in order to be able to call the internal one).

[charles-cooper]

I don't think you can access msg.value from internal, nonpayable functions.

EDIT: you can't, so banning @payable on internal functions would disallow usage of msg.value in internal functions completely.

[pcaversaccio]

right - you are absolutely correct, I just tested that as well. But still, it doesn't feel right to use that modifier twice. Also, I would strive for some consistency with Solidity (in order to make onboarding overall easier for new devs) where it's disallowed to use payable for internal & pure functions. What you are proposing @charles-cooper works, of course. The question is whether it's the most ergonomic solution. And for me personally, I would exclusively reserve the payable modifier for external functions and allow the usage of msg.value within internal functions without the payable modifier.

[charles-cooper]

@pcaversaccio since internal is now optional, should @payable imply @external?

[pcaversaccio]

@pcaversaccio since internal is now optional, should @payable imply @external?

Hmm, I personally dislike implied behaviour. I think it could be confusing that functions are now internal by default (no @internal decorator needed) and @payable implies @external, i.e. no @external decorator needed. If you know the rules it's obvious, sure, but I still think a lot of people might be confused.

[pcaversaccio]

This currently compiles and would imply for both functions an internal behaviour. If we imply via @payable external behaviour this might be a breaking change, no?

@payable
def _sending() -> uint256:
    return 1

def sending() -> uint256:
    return 1

[pcaversaccio]

For me payable is an external feature. The semantics behind payable is that it's "the possibility to receive value via an external function call". An internal function doesn't directly receive ETH but is just some logic as part of the external function that might or might not deal with a ETH value. This is e.g. different to the @nonreentrant decorator which you can use for internal functions as this prevents from accessing specific logic if needed.

[cyberthirst]

transfer of ether doesn't target an external function, it targets an account. the transfer happens outside of bytecode interpretation.

if an external function is nonpayble, we check that the associated message has zero associated value, if it's payable, it's nop

so it's like an assert which can be used to sanity check the given flow through the contract

i think that the problem partly lies in the naming - i'd prefer modifiers like @allow_ether and @reject_ether which imo are more clear and would be compatible with internal functions

also, i think that payble semantics shouldn't be tied to whether you can access msg.value in the given function or not, i think it's valid to access it internal functions

from UX i probably prefer @nonpayable by default for external functions and @payable for internal function by default (ie no gas overhead for internal).

[cyberthirst]

This currently compiles and would imply for both functions an internal behaviour. If we imply via @payable external behaviour this might be a breaking change, no?

@payable
def _sending() -> uint256:
    return 1

def sending() -> uint256:
    return 1

yea, it would be breaking - calling an internal function would now fail as it would be promoted to external one