payable decorator in interface functions not enforced by implements

[pcaversaccio]

Version Information

• vyper Version (output of vyper --version): 0.3.11+commit.819de669
• OS: linux
• Python Version (output of python --version): 3.11.3

What's your issue about?

If an interface function (in a .vyi file) is marked as payable, it is not enforced via implements keyword.

Example

# TestI.vyi
@external
@payable
def foo() -> uint256:
    ...

# Test.vy
import TestI as testI
implements: testI

@external
def foo() -> uint256:
    return 0

vyper Test.vy

0x61002661000f6000396100266000f35f3560e01c63c2985578811861001e5734610022575f60405260206040f35b5f5ffd5b5f80fd8418268000a16576797065728300030b001

This should not compile actually.

How can it be fixed?

Enforce payable decorator via implements keyword.

[pcaversaccio]

EDIT: Non-issue for view functions. My example lacked a implements: Self declaration.

Further note: there is a similar bug for view functions. The following contract should not compile:

interface Self:
    def protected_view_fn() -> String[100]: view

_counter: uint256

@external
def bar() -> String[100]:
    return Self(self).protected_view_fn()

@external
def protected_view_fn() -> String[100]:
    # state-changing function vs the `view` interface definition
    self._counter += 1
    return empty(String[100])

[tserg]

interface Self:
    def protected_view_fn() -> String[100]: view

_counter: uint256

@external
def bar() -> String[100]:
    return Self(self).protected_view_fn()

@external
def protected_view_fn() -> String[100]:
    # state-changing function vs the `view` interface definition
    self._counter += 1
    return empty(String[100])

Is there a missing implements: Self declaration?

[pcaversaccio]

Is there a missing implements: Self declaration?

Good catch, thanks for pointing out @tserg! It's indeed a non-issue for view functions in that case.

[trocher]

In my opinion, if this is prevented, we should go even further and enforce that the modifiability of the interface function matches exactly the modifiability of the defined function. For example, preventing also such cases:

interface Self:
    def protected_view_fn() -> String[100]: nonpayable

implements: Self

@external
@pure
def protected_view_fn() -> String[100]:
    return empty(String[100])

interface Self:
    def protected_view_fn() -> String[100]: view

implements: Self

@external
@pure
def protected_view_fn() -> String[100]:
    return empty(String[100])

Another possibility would be to allow implementing an interface function with modifiability X with a function with modifiability Y as long as Y<X (PURE<VIEW<NONPAYABLE<PAYABLE). This relies on the fact that each modifiabilities inherits from the other (for example a nonpayable function is a payable function that just decides to revert when receiving ETH).
However, in practice, the implements statement is used to adhere strictly to some specification/EIPs and should hence be 1 to 1 I believe.

An example of this from @pcaversaccio where for ERC721 he could implement supportsInterface as pure since the array is a constant but keeps it as a view since this is stated by the EIP: https://github.com/pcaversaccio/snekmate/blob/main/src/snekmate/tokens/ERC721.vy#L333

As a side note this will behaviour will conflict with a potential fix to #3743 since the compiler will warn that the modifiability of a function can be reduced but also prevent the user from doing that.

[pcaversaccio]

Great observation @trocher! Yeah, my general point of view has always been that we must optimise for maximum correctness and thus I second enforcing the modifiability of the interface function to match exactly the modifiability of the defined function.