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

Fix/disallow nonreentrant decorator on constructor #2426

Merged

Conversation

skellet0r
Copy link
Contributor

What I did

Simple fix in module level validation. When visiting all the functions, just check if the constructor has the nonreentrant attribute set to a value other than None.

Fix: #1755

How I did it

Added a check in the module validation stage.

How to verify it

  • Check the test + the line change

Description for the changelog

  • fix: disallow nonreentrant decorator on constructor

Cute Animal Picture

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

@codecov-commenter
Copy link

codecov-commenter commented Aug 18, 2021

Codecov Report

Merging #2426 (3a89723) into master (f1c65b7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2426   +/-   ##
=======================================
  Coverage   85.47%   85.48%           
=======================================
  Files          91       91           
  Lines        9027     9030    +3     
  Branches     2151     2152    +1     
=======================================
+ Hits         7716     7719    +3     
  Misses        806      806           
  Partials      505      505           
Impacted Files Coverage Δ
vyper/semantics/validation/module.py 88.00% <ø> (ø)
vyper/semantics/types/function.py 86.20% <100.00%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1c65b7...3a89723. Read the comment docs.

Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

Nice. One thing I noticed is that the error message highlights the __init__ function rather than the nonreentrant decorator. Do you think it's worth highlighting the decorator instead?

Screenshot from 2021-08-18 13-01-03

@skellet0r
Copy link
Contributor Author

hmmm ... so I could see the case of highlighting either or, but I think for new users in particular, highlighting the decorator will be more clear (even though the message on the exception states it's not allowed on the constructor)

In any case, tl;dr -> I'll switch it to highlight the decorator instead

@charles-cooper
Copy link
Member

hmmm ... so I could see the case of highlighting either or, but I think for new users in particular, highlighting the decorator will be more clear (even though the message on the exception states it's not allowed on the constructor)

In any case, tl;dr -> I'll switch it to highlight the decorator instead

I looked briefly at how one would do this and I'm not sure it's worth the refactoring work. Can you take a look and try to make a judgment call on whether it's worth it?

@charles-cooper
Copy link
Member

I looked briefly at how one would do this and I'm not sure it's worth the refactoring work. Can you take a look and try to make a judgment call on whether it's worth it?

Otherwise -- if we highlight the function signature line, not really the end of the world

Simply catch usage of the nonreentrant decorator on __init__ when the
ContractFunction object is being generated during module validation.

Specifically just add an additional if clause checking if node.name ==
__init__, during the nonreentrant decorator checks.

Now correctly highlights the decorator instead of the __init__ function.
@skellet0r skellet0r force-pushed the fix/disallow-init-nonreentrant branch from 84001e3 to 3a89723 Compare August 18, 2021 21:17
@skellet0r
Copy link
Contributor Author

oh lol, I just moved the logic over to the contract function generation, when we do the nonreentrant usage validation

Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

that was easy. thanks!

@charles-cooper charles-cooper merged commit 755d9fa into vyperlang:master Aug 18, 2021
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.

Disallow @nonreentrant on __init__
3 participants