feat: relax restrictions on internal function signatures (#3573)

relax the restriction on unique "method ids" for internal methods. the
check used to be there to avoid collisions between external method ids
and internal "method ids" because the calling convention for internal
functions used to involve the method id as part of the signature, but
that is no longer the case. so we can safely allow collision between
internal "method ids" and external method ids.

cf. issue #1687 which was resolved in in 9e8c661.

chainsec june 2023 review 5.22
---------

Co-authored-by: tserg <[email protected]>
Co-authored-by: trocher <[email protected]>

tests/parser/syntax/utils/test_function_names.py

@@ -23,6 +23,22 @@ def wei(i: int128) -> int128:
     temp_var : int128 = i
     return temp_var1
     """,
+    # collision between getter and external function
+    """
+foo: public(uint256)
+
+@external
+def foo():
+    pass
+    """,
+    # collision between getter and external function, reverse order
+    """
+@external
+def foo():
+    pass
+
+foo: public(uint256)
+    """,
 ]
 
 
@@ -77,6 +93,30 @@ def append():
 def foo():
     self.append()
     """,
+    # "method id" collisions between internal functions are allowed
+    """
+@internal
+@view
+def gfah():
+    pass
+
+@internal
+@view
+def eexo():
+    pass
+    """,
+    # "method id" collisions between internal+external functions are allowed
+    """
+@internal
+@view
+def gfah():
+    pass
+
+@external
+@view
+def eexo():
+    pass
+    """,
 ]
 
 

tests/signatures/test_method_id_conflicts.py

@@ -48,26 +48,6 @@ def OwnerTransferV7b711143(a: uint256):
     pass
     """,
     """
-# check collision between private method IDs
-@internal
-@view
-def gfah(): pass
-
-@internal
-@view
-def eexo(): pass
-    """,
-    """
-# check collision between private and public IDs
-@internal
-@view
-def gfah(): pass
-
-@external
-@view
-def eexo(): pass
-    """,
-    """
 # check collision with ID = 0x00000000
 wycpnbqcyf:public(uint256)
 

vyper/semantics/analysis/module.py

@@ -22,11 +22,7 @@
 from vyper.semantics.analysis.base import VarInfo
 from vyper.semantics.analysis.common import VyperNodeVisitorBase
 from vyper.semantics.analysis.levenshtein_utils import get_levenshtein_error_suggestions
-from vyper.semantics.analysis.utils import (
-    check_constant,
-    validate_expected_type,
-    validate_unique_method_ids,
-)
+from vyper.semantics.analysis.utils import check_constant, validate_expected_type
 from vyper.semantics.data_locations import DataLocation
 from vyper.semantics.namespace import Namespace, get_namespace
 from vyper.semantics.types import EnumT, EventT, InterfaceT, StructT
@@ -90,6 +86,7 @@ def __init__(
                 err_list.raise_if_not_empty()
 
         # generate an `InterfaceT` from the top-level node - used for building the ABI
+        # note: also validates unique method ids
         interface = InterfaceT.from_ast(module_node)
         module_node._metadata["type"] = interface
         self.interface = interface  # this is useful downstream
@@ -102,11 +99,7 @@ def __init__(
         module_node._metadata["namespace"] = _ns
 
         # check for collisions between 4byte function selectors
-        # internal functions are intentionally included in this check, to prevent breaking
-        # changes in in case of a future change to their calling convention
         self_members = namespace["self"].typ.members
-        functions = [i for i in self_members.values() if isinstance(i, ContractFunctionT)]
-        validate_unique_method_ids(functions)
 
         # get list of internal function calls made by each function
         function_defs = self.ast.get_children(vy_ast.FunctionDef)