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

Fixing a bug for function definition #623

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

apanichella
Copy link

The function defintion should start one posiiton after the end of the modifier.
Given a function "function method() onlyOwner {...}', the current version of the return the definition "r{...}" instead of "{...}". I notice the same error happens when a function uses a modifier: the last character of the modifier is added to the parsed definiton.

This commit fixes the bug.

The function defintion should start one posiiton after the end of the modifier.
Given a function "function method() onlyOwner {...}', the current version of the return the definition "r{...}" instead of "{...}". I notice the same error happens when a function uses a modifier: the last character of the modifier is added to the  parsed definiton.

This commit fixes the bug.
@codecov-io
Copy link

codecov-io commented Mar 26, 2021

Codecov Report

Merging #623 (a5440d5) into master (3c0f3a5) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #623   +/-   ##
=======================================
  Coverage   98.68%   98.68%           
=======================================
  Files          22       22           
  Lines         986      986           
=======================================
  Hits          973      973           
  Misses         13       13           
Impacted Files Coverage Δ
lib/registrar.js 100.00% <100.00%> (ø)

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 3c0f3a5...a5440d5. Read the comment docs.

@cgewecke
Copy link
Member

Thanks so much @apanichella! (Please ignore the netlify failures ... that service doesn't work with PRs from forks atm).

Could you give an example of a solidity statement which triggers this error so I can add it to the test suite and double check this?

(I'm very surprised this hasn't already been reported if it's crashing function definitions with modifiers)

@apanichella
Copy link
Author

apanichella commented Mar 26, 2021

Hi,
I am using the following example:


pragma solidity = 0.5.16;

contract Example {
address private _owner;

constructor () public {}

modifier onlyOwner() {
    require(_owner == msg.sender, "YouSwap: CALLER_IS_NOT_THE_OWNER");
    _;
}

function example (uint a) public onlyOwner {
    if (a == 0)
        bool b = true;
}

}


At runtime, I get three function definitions:

  1. constructor () public
  2. modifier onlyOwner(uint a)
  3. r

Actually, I think my commit does not fix the whole problem. What is the code suppsed to return for th third one?
If the answer is function example (uint a) public onlyOwner, than the whole if conditions for modifers (the one that updates the starting point) is not needed.

If the modifers should be simply removed, than moving the value start forward won't work either.

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.68%. Comparing base (3c0f3a5) to head (a5440d5).
Report is 160 commits behind head on master.

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

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #623   +/-   ##
=======================================
  Coverage   98.68%   98.68%           
=======================================
  Files          22       22           
  Lines         986      986           
=======================================
  Hits          973      973           
  Misses         13       13           

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

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