feat[lang]: support flags from imported interfaces by tserg · Pull Request #4253 · vyperlang/vyper

tserg · 2024-09-22T09:14:59Z

What I did

Resolves #4121

How I did it

How to verify it

Commit message

this commit allows flag types to be imported from `.vyi` interface
files.

Description for the changelog

Cute Animal Picture

vyper/semantics/types/module.py

codecov · 2024-09-22T09:34:17Z

Codecov Report

Attention: Patch coverage is 56.52174% with 10 lines in your changes missing coverage. Please review.

Project coverage is 51.30%. Comparing base (6bcdb00) to head (482f306).

Files with missing lines	Patch %	Lines
vyper/semantics/types/module.py	56.52%	8 Missing and 2 partials ⚠️

❗ There is a different number of reports uploaded between BASE (6bcdb00) and HEAD (482f306). Click for more details.

HEAD has 117 uploads less than BASE

Flag BASE (6bcdb00) HEAD (482f306)

140 23

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #4253       +/-   ##
===========================================
- Coverage   91.40%   51.30%   -40.10%     
===========================================
  Files         112      112               
  Lines       15919    15925        +6     
  Branches     2694     2698        +4     
===========================================
- Hits        14551     8171     -6380     
- Misses        934     7135     +6201     
- Partials      434      619      +185

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

charles-cooper · 2024-09-22T18:20:59Z

vyper/semantics/types/module.py

+        constants = [
+            (node.target.id, node.target._metadata["varinfo"])
+            for node in module_t.variable_decls
+            if node.target._metadata["varinfo"].modifiability is Modifiability.CONSTANT


can just check node.is_constant

charles-cooper · 2024-09-22T18:21:36Z

vyper/semantics/types/module.py

        validate_unique_method_ids(list(functions.values()))

-        members = functions | events | structs
+        constants = constants if constants else {}


constants or {}

tbh we should do this for all the other things too, since in most cases we end up passing an empty dict for most of these parameters

do you mean to set the default for these params to None? should I do it in this PR?

Yes and yes!

I am thinking that since _from_lists is already passing a dictionary (either empty or with values) for all the params, and it is the only caller of the ctor for InterfaceT, maybe we can require a dict instead for all the same params in the ctor for InterfaceT?

I made the changes in 69e5b3e to show what I meant in the previous comment.

charles-cooper

thanks! can we also add flags into scope? #4121

vyper/semantics/types/module.py

…/interface_const

pcaversaccio · 2024-10-03T09:23:02Z

tests/functional/codegen/modules/test_interface_imports.py

+    """
+
+    contract = """
+import ifaces


I have a question here (without having read the full changelog). If you define a constant in an interface, this constant has the visibility public by default I think (see #4178) and thus if you do implements you must create a getter in the main contract to be compliant? It just confuses me how we handle this properly.

I didn't consider that, but as it is at the moment, constants in an interface are not external by default`. Do we want the behaviour to be public by default?

I can see why that makes sense for consistency but IMO it would be better to not treat constants the same way. The mode of declaring a constant as public is different from that for a function, and also we would not expect the getter of a constant to be part of the interface if it was not explicitly declared as public.

Well, that's exactly why I raise this point to highlight the inconsistency here. And I agree with you that it should not be handled the same way, but given the current semantics, it could be interpreted like this. Also, that's why I think having optional external visibility in interfaces is not appropriate really; explicitness is better.

yes, that is a good point. the idea here is that constants are like types, they can be imported for use in your contract, but they shouldn't be allowed to be marked external

can we maybe make this more clear via some syntax sugar? An interface, in the context of smart contract programming, signals something "external", and given that now the visibility modifier is even only optional, we should make the scope clear for constant values to avoid any ambiguity.

That feels a little hacky.

How about we skip this for 0.4.1 and introduce type aliases in the next release?

something in this spirit:

NUM_ITEMS: alias(uint256) = 5 arr: alias(DynArray[uint256, NUM_ITEMS]) def foo(a: arr): pass

in the past, Charles suggested that aliases might also help with this (a bit ugly 😉 ) interface syntax
#4090 (comment)

i mean i think if you can use it in type declarations, you should be able to use it like any other constant. e.g.

for i: uint256 in range(MyInterface.NUM_ITEMS): ...

Based on offline discussion I think we should keep the changes for flags here, roll back the changes for constants here and punt to future work / a separate PR

charles-cooper · 2024-10-03T13:04:27Z

vyper/semantics/types/module.py

-        structs: list = []
-
-        return cls._from_lists(node.name, node, functions, events, structs)
+        return cls._from_lists(node.name, node, functions)


much cleaner now, thanks

charles-cooper · 2024-10-03T13:05:47Z

vyper/semantics/types/module.py

+                    isinstance(prev, ContractFunctionT)
+                    and isinstance(item, VarInfo)
+                    and item.decl_node.is_public
+                ):


hmm, why is this needed?

Without this, a public constant would result in a duplicate because it would be in constants and its public getter would be in functions.

charles-cooper · 2024-10-03T13:06:32Z

vyper/semantics/types/module.py

-        return TYPE_T(self._helper.get_member(attr, node))
+        # get an event, struct or constant from this interface
+        type_member = self._helper.get_member(attr, node)
+        if isinstance(type_member, (EventT, FlagT, StructT)):


maybe

Suggested change

if isinstance(type_member, (EventT, FlagT, StructT)):

if isinstance(type_member, VyperType):

or UserType might be better

…into feat/interface_const

…/interface_const

vyper/semantics/types/module.py

 from vyper.semantics.types.function import ContractFunctionT
 from vyper.semantics.types.primitives import AddressT
-from vyper.semantics.types.user import EventT, StructT, _UserType
+from vyper.semantics.types.user import EventT, FlagT, StructT, _UserType


charles-cooper

thank you!

support .vyi const

6a87f54

github-advanced-security bot found potential problems Sep 22, 2024

View reviewed changes

vyper/semantics/types/module.py Fixed Show fixed Hide fixed

tserg added 2 commits September 22, 2024 17:26

fix lint

9edcd52

fix more lint

2065c23

charles-cooper reviewed Sep 22, 2024

View reviewed changes

tserg added 4 commits September 23, 2024 21:56

apply bts suggestions

c718e28

fix public constants case

8a932a3

change signatures

69e5b3e

add flags

8439d5b

github-advanced-security bot found potential problems Sep 28, 2024

View reviewed changes

vyper/semantics/types/module.py Fixed Show fixed Hide fixed

tserg added 2 commits October 2, 2024 16:27

add tests

2539997

Merge branch 'master' of https://github.com/vyperlang/vyper into feat…

20d1d33

…/interface_const

tserg marked this pull request as ready for review October 2, 2024 08:48

pcaversaccio reviewed Oct 3, 2024

View reviewed changes

charles-cooper reviewed Oct 3, 2024

View reviewed changes

tserg and others added 8 commits October 4, 2024 16:05

apply cc suggestions

d2e8aea

Merge branch 'master' into feat/interface_const

f9e4004

rollback constants

76824a1

Merge branch 'feat/interface_const' of https://github.com/tserg/vyper …

98e4925

…into feat/interface_const

remove debug prints

be60015

more rollbacks

223d6b3

remove more debug prints

39e955d

clean up

785b606

tserg changed the title ~~feat[ux]: support constants from imported interface~~ feat[ux]: support flags from imported interface Oct 12, 2024

Merge branch 'master' of https://github.com/vyperlang/vyper into feat…

94b70fc

…/interface_const

Merge branch 'master' of https://github.com/vyperlang/vyper into feat…

482f306

…/interface_const

github-advanced-security bot found potential problems Oct 15, 2024

View reviewed changes

small cleanup

d936b86

charles-cooper changed the title ~~feat[ux]: support flags from imported interface~~ feat[ux]: support flags from imported interfaces Oct 15, 2024

charles-cooper changed the title ~~feat[ux]: support flags from imported interfaces~~ feat[lang]: support flags from imported interfaces Oct 15, 2024

charles-cooper enabled auto-merge (squash) October 15, 2024 01:33

charles-cooper approved these changes Oct 15, 2024

View reviewed changes

charles-cooper merged commit 990a6fa into vyperlang:master Oct 15, 2024

	if isinstance(type_member, (EventT, FlagT, StructT)):
	if isinstance(type_member, VyperType):

Uh oh!

Conversation

tserg commented Sep 22, 2024 • edited by charles-cooper Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What I did

How I did it

How to verify it

Commit message

Description for the changelog

Cute Animal Picture

Uh oh!

Uh oh!

codecov bot commented Sep 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charles-cooper left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pcaversaccio Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyberthirst Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charles-cooper Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charles-cooper Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check notice

charles-cooper left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

tserg commented Sep 22, 2024 •

edited by charles-cooper

Loading

codecov bot commented Sep 22, 2024 •

edited

Loading

pcaversaccio Oct 3, 2024 •

edited

Loading

cyberthirst Oct 11, 2024 •

edited

Loading

charles-cooper Oct 11, 2024 •

edited

Loading

charles-cooper Oct 11, 2024 •

edited

Loading