Skip to content

feat[lang]: auto-export events in ABI#3808

Merged
charles-cooper merged 24 commits intovyperlang:masterfrom
charles-cooper:feat/event-exports
Feb 26, 2024
Merged

feat[lang]: auto-export events in ABI#3808
charles-cooper merged 24 commits intovyperlang:masterfrom
charles-cooper:feat/event-exports

Conversation

@charles-cooper
Copy link
Copy Markdown
Member

@charles-cooper charles-cooper commented Feb 24, 2024 •
edited
Loading

What I did

depends #3786

after this, the standard token demo looks like this:

from snekmate.tokens import ERC20 as base_token

implements: base_token.IERC20
initializes: base_token

exports: (
    base_token.transfer,
    base_token.transferFrom,
    base_token.approve,
    base_token.allowance,
    base_token.totalSupply,
    base_token.balanceOf,
)

@deploy
def __init__():
    base_token.__init__("My Token", "TOK", 100_000_000, "My DApp", "v0.0.1")

How I did it

How to verify it

Commit message

this commit modifies ABI json generation so that it automatically
exports events.

since this additionally obviates the need for `implements: <interface>`
to check that event declarations conform with the interface, the event
conformance check has been removed from the interface implements check.

the methodology used here is to export only those events which are
detected as used, as opposed to any event imported in the import graph.
this is intended to minimize namespace pollution, since if multiple
imported modules define the same event, they can result in a namespace
collision when computing the interface. this restriction could be
reconsidered in the future.

this commit also implements some refactors to the analysis pipeline:
- move ModuleT computation to *after* call graph analysis, so that
  ModuleT can compute all events used in the call graph
- additionally, move ModuleT computation to after getter expansion,
  so that `exposed_functions` can return `FunctionDef`s so that
  downstream code doesn't need to do the work to resolve the expanded
  getter from `VariableDecl`.
- move call graph analysis outside of `ModuleAnalyzer.analyze()`
- rename `ModuleAnalyzer.analyze()` to `.analyze_module_body()` to
  better reflect what it does

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Feb 24, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 91.76471% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 84.87%. Comparing base (a91af13) to head (f8b27ee).
Report is 1 commits behind head on master.

Files Patch % Lines
vyper/semantics/analysis/module.py 90.00% 3 Missing and 1 partial ⚠️
vyper/semantics/types/module.py 90.32% 2 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3808      +/-   ##
==========================================
- Coverage   85.08%   84.87%   -0.21%     
==========================================
  Files          92       92              
  Lines       13875    13907      +32     
  Branches     3113     3116       +3     
==========================================
- Hits        11805    11804       -1     
- Misses       1576     1607      +31     
- Partials      494      496       +2

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

@charles-cooper
move Log ast validation to nodes.py
32c2523
@charles-cooper
minor refactors of module analysis
d735fb7 
- improve call graph analysis
  - called_functions and reachable_internal_functions start out as None
  - move analyze_call_graph into standalone function instead of method
- tinker with _add_exposed_function to ensure we have all exposed
  functions by the time we get to ImplementsDecl
@charles-cooper
redo events abi export
69ef4b0 
rely on analysis, not declared events
@charles-cooper
use initialized/used modules instead of analyzing functions
d50ac28
@charles-cooper
update example tokens to use imported events
b20375f
@charles-cooper
export events from implemented interfaces
8cccb93
@charles-cooper
update/remove old tests for events implementation checks
6f6caf9
@charles-cooper
fix some more tests
8d351d1
@charles-cooper
update lark grammar
7e34731
@charles-cooper
fix some tests
5da22d5 
change the event member names in the builtin interfaces
@charles-cooper
add some tests
31a9a8f 
rename a test file
@charles-cooper
add more tests
c510445
@charles-cooper
Revert "use initialized/used modules instead of analyzing functions"
dc183f3 
This reverts commit 0747a5c.
@charles-cooper
add another hint for IERC20
e86b862
@charles-cooper
fix reachable analysis for nested internal functions
18a6112
@charles-cooper
fixup tests
a1cbb05
@charles-cooper
update tests
af65b19
@charles-cooper
add more codegen tests
337348e
@charles-cooper
update a test
847a35f
@charles-cooper
lint
4c7074a
@charles-cooper
add a structure exception test
bd6aba0
@charles-cooper
move a test
dde7d81
@charles-cooper charles-cooper marked this pull request as ready for review February 25, 2024 20:48
@charles-cooper charles-cooper changed the title feat: event exports feat[lang]: improve event exports Feb 25, 2024
pcaversaccio
pcaversaccio approved these changes Feb 26, 2024
View reviewed changes
cyberthirst
cyberthirst approved these changes Feb 26, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@cyberthirst cyberthirst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name collisions with imported events are performed based on used_events, thus NamespaceCollision isn't thrown in the cases like the following one:

# ITest
event Foo:
    a: uint256
import ITest

implements: ITest

event Foo:
    a: address

@external
def foo(u: uint256):
    log Foo(empty(address))
    #log ITest.Foo(u)

Testing with an attribute access to i throws IndexError: list index out of range:

import ITest

implements: ITest

event Foo:
    a: address

@external
def foo(u: uint256):
    log Foo(empty(address))
    log i.Foo(empty(address)

@charles-cooper
Copy link
Copy Markdown
Member Author

charles-cooper commented Feb 26, 2024

Name collisions with imported events are performed based on used_events, thus NamespaceCollision isn't thrown in the cases like the following one:

# ITest
event Foo:
    a: uint256
import ITest

implements: ITest

event Foo:
    a: address

@external
def foo(u: uint256):
    log Foo(empty(address))
    #log ITest.Foo(u)

this is intended, it's an attempt to reduce namespace pollution (instead of just exporting all events found in all imports)

the other option is just to export all events in all imports, but that leads to some weird interactions with the namespace, e.g. you import ERC721 and import ERC20, we can either let there be a namespace collision (which is terrible UX here!) or we do some "refactoring", where the Transfer event is exposed in the ABI, but not in the namespace inside of vyper.

Testing with an attribute access to i throws IndexError: list index out of range:

import ITest

implements: ITest

event Foo:
    a: address

@external
def foo(u: uint256):
    log Foo(empty(address))
    log i.Foo(empty(address)

weird, will investigate

@charles-cooper
Copy link
Copy Markdown
Member Author

charles-cooper commented Feb 26, 2024 •
edited
Loading

after investigating the parser issue i think it's an unrelated, latent error. it should be fixed, but reviewed in a separate PR

@charles-cooper charles-cooper changed the title feat[lang]: improve event exports feat[lang]: auto-export events in ABI Feb 26, 2024
@charles-cooper charles-cooper merged commit b53a020 into vyperlang:master Feb 26, 2024
This was referenced Mar 4, 2024
Closed
Merged
@charles-cooper charles-cooper mentioned this pull request Jan 1, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cyberthirst cyberthirst cyberthirst approved these changes

@pcaversaccio pcaversaccio pcaversaccio approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@charles-cooper @codecov-commenter @cyberthirst @pcaversaccio