Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 24 commits into from
Feb 26, 2024

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Feb 24, 2024

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

codecov-commenter commented Feb 24, 2024

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 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
Copy link
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
Member Author

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
Member Author

charles-cooper commented Feb 26, 2024

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
84 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants