-
-
Notifications
You must be signed in to change notification settings - Fork 806
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: implement "stateless" modules #3663
feat: implement "stateless" modules #3663
Conversation
- refactor: repurpose GlobalContext into ModuleT - simplify cfg analysis
"modules" are kind of like types and kind of like exprs/varinfos. make them more like exprs/varinfos since we may want to tag them with analysis (like import location, info about the import module, etc)
also: - distinguish between module.name and module.path during AST parsing - split InterfaceT.from_ast to from_Module and from_InterfaceDef
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #3663 +/- ##
==========================================
+ Coverage 83.82% 83.96% +0.14%
==========================================
Files 91 92 +1
Lines 12778 13044 +266
Branches 2861 2928 +67
==========================================
+ Hits 10711 10953 +242
- Misses 1643 1658 +15
- Partials 424 433 +9 ☔ View full report in Codecov by Sentry. |
it could be different, depending on its alias at import time. just remove it, so there is a 1:1 correspondence between module ast and source code.
5773c98
to
21a04c3
Compare
raising exception.with_annotation(node) -- just catch-all at in the parent implementation and then don't have to worry about it at the exception site.
i had to cut the scope somewhere and decided to leave those for later since they use the same mechanism as will be used for stateful modules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another 2 comments after testing:
- My first comment is about unused imports and devex (it's related to my issue Warning about unused function parameters and imports #3272). There is no compilation warning issued currently if I import libraries that are not used. I think it would be a good idea to do that for unused imports in order to improve the quality and cleanliness of the code. This must not be part of that PR, but we should not forget about it.
- I'm wondering whether we should allow nested module calls:
# Lib1.vy
@internal
@view
def blocknumber() -> uint256:
return block.number + 1
# Lib2.vy
import Lib1
@internal
@view
def blocknumber() -> uint256:
return Lib1.blocknumber()
# Main.vy
import Lib2
@external
@view
def blocknumber() -> uint256:
return Lib2.Lib1.blocknumber()
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another 2 feedback:
- We should add more tests using Vyper built-in functions.
- The current implementation allows for stateful changes, if you apply some tricks:
# Lib
@internal
@payable
def call(target: address) -> Bytes[32]:
response: Bytes[32] = raw_call(self, method_id("hello()"), max_outsize=32, value=msg.value)
return response
# Main.vy
import Lib
var: uint256
@external
@payable
def hello():
self.var = msg.value
@external
@payable
def call(target: address) -> Bytes[32]:
return Lib.call(target)
This is more a discussion about the scope of the PR. If it's pure
and view
libraries, we should disallow raw_call
's IMO.
no, "stateless" refers more to the fact that importing variables (of any kind) is not supported, not the mutability properties of the imported functions. |
yes this is probably useful, but i'm declaring it out-of-scope for this PR |
fixed in 64bc3a5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm approving with the following caveat: I have tested some of the logic but not all, and I also haven't reviewed every single line of code. Under normal circumstances, I wouldn't approve the PR, BUT this PR is an important foundational layer for upcoming PRs that need to be worked on. Furthermore, we have to fully audit/review the stateless and stateful modules implementation, and the sooner we get there (hopefully in January) the better. For anyone going through this PR in the future: Please be aware that there might be bugs, but those bugs will be fixed in upcoming PRs. We don't compromise on any security here, but we have to first get the features done in a proper way, and ensure the complete security based on a feature-complete (in that case library modules) state of the compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still some code quality issues to address, but otherwise looks fine
Happy with @pcaversaccio's manual testing
@pcaversaccio re. builtins see bea05ca |
resolves #2431, #2670, #3294
partially implements
.vyi
files #2814 (but does not handle ellipsis for bytestring/dynarray length, that will come in a follow up PR)partially finishes #2852 (although it does not automatically export events in the ABI)
What I did
How I did it
How to verify it
Commit message
Description for the changelog
Cute Animal Picture