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]!: make internal decorator optional #4040

Merged

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented May 21, 2024

What I did

per title

How I did it

How to verify it

updated a couple tests for it

Commit message

make `@internal` decorator optional. since functions can be imported
now, the `@internal` decorator could be confusing UX, since it is not
clear (without referencing docs) whether `@internal` means "hidden from
importers of this module", or "not included in the selector table". with
this commit, functions don't need a visibility marker, and are just
internal by default.

Description for the changelog

Cute Animal Picture

image

@cyberthirst
Copy link
Collaborator

could this be any problem wrt to integrity hash in archives?

@charles-cooper
Copy link
Member Author

charles-cooper commented May 21, 2024

could this be any problem wrt to integrity hash in archives?

i don't think so, no. the integrity hash is based on source hash. so you can have multiple sources which compile to the same bytecode, but this is already the case (e.g. source code comments change hash but not bytecode)

@cyberthirst
Copy link
Collaborator

why do we allow @nonreentrant + @view? is it read-only reentrancy?

@cyberthirst
Copy link
Collaborator

why not remove

# assert function_visibility is not None # mypy
# assert state_mutability is not None # mypy

@cyberthirst
Copy link
Collaborator

cyberthirst commented May 25, 2024

contracts like this

def __init__():
    pass

will throw

 vyper.exceptions.FunctionDeclarationException: Constructor must be marked as `@deploy`, not `@internal`

which is a bit wierd

@cyberthirst
Copy link
Collaborator

did we update the docs to reflect this feature?

@charles-cooper
Copy link
Member Author

did we update the docs to reflect this feature?

yes, in #3947

@charles-cooper charles-cooper added this to the v0.4.0 milestone May 25, 2024
@charles-cooper
Copy link
Member Author

contracts like this

def __init__():
    pass

will throw

 vyper.exceptions.FunctionDeclarationException: Constructor must be marked as `@deploy`, not `@internal`

which is a bit wierd

0a48a56

@charles-cooper
Copy link
Member Author

why not remove

# assert function_visibility is not None # mypy
# assert state_mutability is not None # mypy

9f679b2

@charles-cooper
Copy link
Member Author

why do we allow @nonreentrant + @view? is it read-only reentrancy?

yes, #2921

Copy link

codecov bot commented May 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.61%. Comparing base (abf795a) to head (8b4a023).
Report is 55 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4040      +/-   ##
==========================================
- Coverage   91.12%   88.61%   -2.51%     
==========================================
  Files         106      106              
  Lines       15309    15307       -2     
  Branches     3366     3366              
==========================================
- Hits        13950    13564     -386     
- Misses        925     1234     +309     
- Partials      434      509      +75     

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

@charles-cooper charles-cooper merged commit 5445960 into vyperlang:master May 25, 2024
158 of 159 checks passed
@charles-cooper charles-cooper deleted the feat/optional-internal branch May 25, 2024 13:09
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.

None yet

3 participants