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 function state mutability warning #2327

Merged
merged 1 commit into from
Aug 12, 2020

Conversation

zemse
Copy link
Contributor

@zemse zemse commented Aug 12, 2020

PR #1739 hardcodes ERC777 granularity to 1, this causes the solc compiler generates the following warning:

WARNING
  @openzeppelin/contracts/token/ERC777/ERC777.sol:116:5: Warning: Function state mutability can be restricted to pure
      function granularity() public view override returns (uint256) {
      ^ (Relevant source part starts here and spans across multiple lines).

This PR fixes the above warning by changing the state mutability of granularity function from view to pure.

@zemse zemse changed the base branch from master to solc-0.7 August 12, 2020 18:17
@zemse zemse marked this pull request as draft August 12, 2020 18:18
Changes state mutability of granularity function from view to pure.
@zemse zemse force-pushed the fix/state-mutability-warning branch from 9620f6b to 4b25047 Compare August 12, 2020 18:21
@zemse
Copy link
Contributor Author

zemse commented Aug 12, 2020

This gives error in 0.6, but in the newly released 0.7 first language feature, it is mentioned that pure can override view. Source: https://github.com/ethereum/solidity/releases/tag/v0.7.0

As seen the linter test is failing, looks like it also needs to be upgraded. I'll take a look if I can do that.

Edit: I tried upgrading solhint to 3.1.0 (from 3.0.0), but seems like the version in which the constructor visibility issue is fixed would be released after protofire/solhint#241 gets merged. I think this branch already has the lint CI failing (since this PR changes function state mutability while lint error is regarding constructor visibility), so marking this ready.

@zemse zemse marked this pull request as ready for review August 12, 2020 18:56
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Awesome @zemse! Thank you.

@frangio frangio merged commit 48072e4 into OpenZeppelin:solc-0.7 Aug 12, 2020
@fvictorio
Copy link
Contributor

I'm about to publish a version of solhint with support for 0.7. Should I test it in the OpenZeppelin:solc-0.7 branch?

@frangio
Copy link
Contributor

frangio commented Aug 13, 2020

@fvictorio Yes, please! Let me know how it goes.

@fvictorio
Copy link
Contributor

@frangio Just published solhint@3.2.0, that works fine in that branch (there's a bazillion warnings though). I tried to make a PR upgrading it but node-gyp is messing with me 😢

@frangio
Copy link
Contributor

frangio commented Aug 24, 2020

@fvictorio Thanks! Seems to work well. Just updated our 0.7 branch.

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.

3 participants