Remove all Solidity warnings #1346

nventuro · 2018-09-24T18:02:41Z

Some of our contracts compile with warnings, mostly due to unused function names of functions that will be overridden, and require the argument to be there for the docstring to not error out.

OZ should compile with no warnings, so we should either make inline exceptions for this, or find a way to work around it.

ChrisCates · 2018-09-24T18:57:36Z

Some warnings include require() functions needing a second parameter to explicitly define what the error was.

Can help contribute if open to a pull request.

nventuro · 2018-09-24T19:04:12Z

Hey there @ChrisCates! Sure, a contribution here would be of great help :)

For reference, these are the warnings of the latest CI run (https://travis-ci.org/OpenZeppelin/openzeppelin-solidity/jobs/430787621#L2884):

Compilation warnings encountered:
/home/travis/build/OpenZeppelin/openzeppelin-solidity/contracts/drafts/Counter.sol:17:3: Warning: This declaration shadows an existing declaration.
  struct Counter {
  ^ (Relevant source part starts here and spans across multiple lines).
/home/travis/build/OpenZeppelin/openzeppelin-solidity/contracts/drafts/Counter.sol:15:1: The shadowed declaration is here:
library Counter {
^ (Relevant source part starts here and spans across multiple lines).
,/home/travis/build/OpenZeppelin/openzeppelin-solidity/contracts/drafts/ERC20Migrator.sol:70:27: Warning: This declaration shadows an existing declaration.
  function beginMigration(ERC20Mintable newToken) public {
                          ^--------------------^
/home/travis/build/OpenZeppelin/openzeppelin-solidity/contracts/drafts/ERC20Migrator.sol:61:3: The shadowed declaration is here:
  function newToken() public view returns (IERC20) {
  ^ (Relevant source part starts here and spans across multiple lines).
,/home/travis/build/OpenZeppelin/openzeppelin-solidity/contracts/crowdsale/Crowdsale.sol:166:5: Warning: Unused function parameter. Remove or comment out the variable name to silence this warning.
    address beneficiary,
    ^-----------------^
,/home/travis/build/OpenZeppelin/openzeppelin-solidity/contracts/crowdsale/Crowdsale.sol:167:5: Warning: Unused function parameter. Remove or comment out the variable name to silence this warning.
    uint256 weiAmount
    ^---------------^
,/home/travis/build/OpenZeppelin/openzeppelin-solidity/contracts/crowdsale/Crowdsale.sol:208:5: Warning: Unused function parameter. Remove or comment out the variable name to silence this warning.
    address beneficiary,
    ^-----------------^
,/home/travis/build/OpenZeppelin/openzeppelin-solidity/contracts/crowdsale/Crowdsale.sol:209:5: Warning: Unused function parameter. Remove or comment out the variable name to silence this warning.
    uint256 weiAmount
    ^---------------^
,/home/travis/build/OpenZeppelin/openzeppelin-solidity/contracts/payment/RefundEscrow.sol:84:30: Warning: Unused function parameter. Remove or comment out the variable name to silence this warning.
  function withdrawalAllowed(address payee) public view returns (bool) {
                             ^-----------^
,/home/travis/build/OpenZeppelin/openzeppelin-solidity/contracts/crowdsale/Crowdsale.sol:150:3: Warning: Function state mutability can be restricted to pure
  function _preValidatePurchase(
  ^ (Relevant source part starts here and spans across multiple lines).
,/home/travis/build/OpenZeppelin/openzeppelin-solidity/contracts/crowdsale/Crowdsale.sol:165:3: Warning: Function state mutability can be restricted to pure
  function _postValidatePurchase(
  ^ (Relevant source part starts here and spans across multiple lines).
,/home/travis/build/OpenZeppelin/openzeppelin-solidity/contracts/crowdsale/Crowdsale.sol:207:3: Warning: Function state mutability can be restricted to pure
  function _updatePurchasingState(
  ^ (Relevant source part starts here and spans across multiple lines).
,/home/travis/build/OpenZeppelin/openzeppelin-solidity/contracts/crowdsale/distribution/FinalizableCrowdsale.sol:44:3: Warning: Function state mutability can be restricted to pure
  function _finalization() internal {
  ^ (Relevant source part starts here and spans across multiple lines).
,/home/travis/build/OpenZeppelin/openzeppelin-solidity/contracts/mocks/MessageHelper.sol:37:3: Warning: Function state mutability can be restricted to pure
  function fail() public {
  ^ (Relevant source part starts here and spans across multiple lines).

ChrisCates · 2018-09-24T19:38:32Z

Having troubles running npm install on the project.
Have switched to v8.9.4 as well too.

Can't run truffle compile either without Scrypt.
Perhaps instructions on how to compile the project to debug/contribute would be nice.

nventuro · 2018-09-24T19:48:23Z

What truffle version are you running? The setup process is literally to run npm install: npm test should just work at that point.

ChrisCates · 2018-09-24T20:00:30Z

Truffle v4.1.14 (core: 4.1.14)
Solidity v0.4.24 (solc-js)

frangio · 2018-09-26T14:20:05Z

Having troubles running npm install on the project.

@ChrisCates can you show us the output you get when trying to install?

ChrisCates · 2018-09-26T23:38:00Z

Using:

npm v6.4.1
node v8.12.0

Resolved the issue. Using node v8.9.1 as specified in the .node-version appears to give errors when I used it 2 days ago...

I will test with v8.9.1 to see if I can reproduce the issue.

ChrisCates · 2018-09-26T23:42:17Z

Interestingly enough,
Seems v8.9.1 is throwing errors...

Here is a logcat of the install: https://gist.github.com/ChrisCates/ff9f91ceefd0c616d90ef4a5334c67eb

Aniket-Engg · 2018-10-02T08:04:14Z

@ChrisCates you working on it?

nventuro · 2018-10-02T10:07:52Z

@ChrisCates it looks like you had trouble installing scrypt. That's bizarre, and I've never seen it reported here, could you share some more info regarding your setup/error messages we're getting, to see if we can figure it out?

@Aniket-Engg I'd say you're free to work on this, since Chris has been having issues setting up the project.

ChrisCates · 2018-10-03T01:37:42Z

@nventuro, very bizarre issue I agree, however, it's only problematic with the node version specified in the root folder. Using the LTS (v8.12.0) is no problem.

I've provided some comments that both you and @Aniket-Engg can give feedback and/or discuss problems.

Here are some concerns I have:

First, Crowdsale.sol has optional overriding functions... Warnings are emitted since the 2 arguments in the function are not used... using an implementation such as below will actually cause revert functionality to fail due to the assertions made with require():

  /**
   * @dev Validation of an executed purchase. Observe state and use revert statements to undo rollback when valid conditions are not met.
   * @param beneficiary Address performing the token purchase
   * @param weiAmount Value in wei involved in the purchase
   */
  function _postValidatePurchase(
    address beneficiary,
    uint256 weiAmount
  )
    internal
  {
    // optional override
    require(beneficiary != address(0));
    require(weiAmount > 0);
  }

Second, RefundEscrow.sol... The payee address is not used with withdrawalAllowed()... I believe that this function is incompatible with the implementation found in ConditionalEscrow.sol

  /**
   * @dev Returns whether refundees can withdraw their deposits (be refunded).
   */
  function withdrawalAllowed() public view returns (bool) {
    return _state == State.Refunding;
  }

The conflict with ConditionalEscrow.sol:

function withdrawalAllowed(address payee) public view returns (bool);

To summarize, because the Solidity language does not allow for function overloading, and, all functions that are extended via an implementation must use the same name and the same modifiers and arguments... We still end up with these side effects stated as warnings in the compiler even with require() assertions implemented for both files in question:

/Users/chriscates/Projects/openzeppelin-solidity/contracts/crowdsale/Crowdsale.sol:151:3: Warning: Function state mutability can be restricted to pure
  function _preValidatePurchase(
  ^ (Relevant source part starts here and spans across multiple lines).
,/Users/chriscates/Projects/openzeppelin-solidity/contracts/crowdsale/Crowdsale.sol:166:3: Warning: Function state mutability can be restricted to pure
  function _postValidatePurchase(
  ^ (Relevant source part starts here and spans across multiple lines).
,/Users/chriscates/Projects/openzeppelin-solidity/contracts/crowdsale/Crowdsale.sol:210:3: Warning: Function state mutability can be restricted to pure
  function _updatePurchasingState(
  ^ (Relevant source part starts here and spans across multiple lines).
,/Users/chriscates/Projects/openzeppelin-solidity/contracts/crowdsale/distribution/FinalizableCrowdsale.sol:45:3: Warning: Function state mutability can be restricted to pure
  function _finalization() internal {
  ^ (Relevant source part starts here and spans across multiple lines).
,/Users/chriscates/Projects/openzeppelin-solidity/contracts/payment/ConditionalEscrow.sol:16:3: Warning: Function state mutability can be restricted to pure
  function withdrawalAllowed(address payee) public view returns (bool) {
  ^ (Relevant source part starts here and spans across multiple lines).

Also, when running tests, there are side effects such as the revert functionality failing in the context of using require() functions as assertions to suppress warnings. This is because the revert functionality does not provide the arguments required for the require() function.

I have also written an EIP in regards to this issue that directly discusses some existing issues: ethereum/EIPs#1465

Aniket-Engg · 2018-10-03T08:29:59Z

@ChrisCates Regarding function overloading, as per the solidity documentation,

A Contract can have multiple functions of the same name but with different arguments.

For more: https://solidity.readthedocs.io/en/v0.4.25/contracts.html#overload-function

ChrisCates · 2018-10-03T13:34:25Z

@Aniket-Engg, my bad in regards to function overloading...
It seems there is actually a side effect for implementations not being used:

/Users/chriscates/Projects/openzeppelin-solidity/contracts/crowdsale/distribution/RefundableCrowdsale.sol:29:15: TypeError: Trying to create an instance of an abstract contract.
    _escrow = new RefundEscrow(wallet());
              ^--------------^
/Users/chriscates/Projects/openzeppelin-solidity/contracts/payment/ConditionalEscrow.sol:16:3: Missing implementation:
  function withdrawalAllowed(address payee) public view returns (bool);
  ^-------------------------------------------------------------------^

I was really tired when looking through the error logs last night... Didn't realize I made such a big logical error (in regards to debugging and the core standard library itself)...

The question then becomes...

If an implementation/abstract function is not used when created... How do we treat the unused functions in the contract?

Should there be a new contract in place? Or do we completely remove dependency to the RefundableEscrow contract?

nventuro · 2018-10-03T16:07:36Z

@ChrisCates

First, Crowdsale.sol has optional overriding functions... Warnings are emitted since the 2 arguments in the function are not used...

True, but we need them for the docstring :/

using an implementation such as below will actually cause revert functionality to fail due to the assertions made with require():

Not sure what you mean with 'causes revert to fail', do the function calls not revert when you add those requires?

Second, RefundEscrow.sol... The payee address is not used with withdrawalAllowed()... I believe that this function is incompatible with the implementation found in ConditionalEscrow.sol

The argument needs to be there to override the correct function, but you're right in that it is unsed in that override (because that escrow doesn't allow or disallow based on the payee).

(multiple logs related to Warning: Function state mutability can be restricted to pure)

This is because the implementations are empty, but the overrides will not, and they will not be pure. Solidity doesn't allow changing this (there is a related issue ethereum/solidity#3412, though it's for allowing stricter mutability, not the other way around). I'm actually not sure we can do much here.

Also, when running tests, there are side effects such as the revert functionality failing in the context of using require() functions as assertions to suppress warnings. This is because the revert functionality does not provide the arguments required for the require() function.

Could you expand a bit on this point, and provide an example if possible?

If an implementation/abstract function is not used when created... How do we treat the unused functions in the contract?

What do you mean by 'not used'? Not implemented? In that case, the contract will be abstract, and won't be able to be deployed (as you saw in that error message when you changed the override).

Aniket-Engg · 2018-10-03T17:14:53Z

@Aniket-Engg, my bad in regards to function overloading...
It seems there is actually a side effect for implementations not being used:
/Users/chriscates/Projects/openzeppelin-solidity/contracts/crowdsale/distribution/RefundableCrowdsale.sol:29:15: TypeError: Trying to create an instance of an abstract contract.
    _escrow = new RefundEscrow(wallet());
              ^--------------^
/Users/chriscates/Projects/openzeppelin-solidity/contracts/payment/ConditionalEscrow.sol:16:3: Missing implementation:
  function withdrawalAllowed(address payee) public view returns (bool);
  ^-------------------------------------------------------------------^
I was really tired when looking through the error logs last night... Didn't realize I made such a big logical error (in regards to debugging and the core standard library itself)...

The question then becomes...

If an implementation/abstract function is not used when created... How do we treat the unused functions in the contract?

Should there be a new contract in place? Or do we completely remove dependency to the RefundableEscrow contract?

No Problem @ChrisCates , I agree with @nventuro . I think we have very less scope of improvement in some of the warnings.

ChrisCates · 2018-10-03T23:32:42Z

@nventuro, @Aniket-Engg:

I'm going to provide a pull request later tonight for you guys to review while also doing some revisions as per the comments from @nventuro. I believe that the issue with reverting transactions will still occur. And will get back to you with a pull request once I get everything sorted.

Also, @nventuro

When a function implementation is not used for example:

function withdrawalAllowed(address payee) public view returns (bool); //This is not used
function withdrawalAllowed() public view returns (bool); //This is used

There will be a compilation TypeError:

/Users/chriscates/Projects/openzeppelin-solidity/contracts/crowdsale/distribution/RefundableCrowdsale.sol:29:15: TypeError: Trying to create an instance of an abstract contract.
    _escrow = new RefundEscrow(wallet());
              ^--------------^
/Users/chriscates/Projects/openzeppelin-solidity/contracts/payment/ConditionalEscrow.sol:16:3: Missing implementation:
  function withdrawalAllowed(address payee) public view returns (bool);
  ^-------------------------------------------------------------------^

Anyway, will get back to you guys soon with some updates!

nventuro · 2019-05-22T14:47:54Z

A couple users have reported concerns upon finding that compiling OpenZeppelin contracts emits warnings.

This is the only warning the latest compier (v0.5.8) emits:

openzeppelin-solidity/contracts/crowdsale/Crowdsale.sol:138:5: Warning: Function state mutability can be restricted to pure
    function _preValidatePurchase(address beneficiary, uint256 weiAmount) internal view {
    ^ (Relevant source part starts here and spans across multiple lines).

That warning is actually valid, and there's currently no way to express what we intend to do (call view functions from that function) in the Solidity language. It'd be great if we could get a compiler feature that allowed us to disable specific warnings for a line (like most linters offer).

nventuro · 2019-09-10T18:23:37Z

We found a workaround to all edge cases, and now have no more warnings!

ChrisCates · 2019-09-10T18:26:58Z

@nventuro, sweet good to hear man!
I was going to restart it. But, the project got a huge update after I made that PR, and that PR was mostly useless.

Keep up the good work man!

nventuro added kind:improvement contracts Smart contract code. labels Sep 24, 2018

ChrisCates mentioned this issue Oct 11, 2018

Removed all eligible solidity warnings #1408

Closed

BlinkyStitt mentioned this issue Jan 7, 2019

Function state mutability can be restricted to pure #608

Closed

nventuro mentioned this issue Jan 15, 2019

Fix warnings #1606

Merged

nventuro removed the improvement label Apr 8, 2019

OpenZeppelin deleted a comment Jun 9, 2019

frangio removed the help wanted label Jul 13, 2019

nventuro closed this as completed Sep 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove all Solidity warnings #1346

Remove all Solidity warnings #1346

nventuro commented Sep 24, 2018

ChrisCates commented Sep 24, 2018

nventuro commented Sep 24, 2018

ChrisCates commented Sep 24, 2018

nventuro commented Sep 24, 2018

ChrisCates commented Sep 24, 2018

frangio commented Sep 26, 2018

ChrisCates commented Sep 26, 2018

ChrisCates commented Sep 26, 2018

Aniket-Engg commented Oct 2, 2018

nventuro commented Oct 2, 2018 •

edited

Loading

ChrisCates commented Oct 3, 2018 •

edited

Loading

Aniket-Engg commented Oct 3, 2018 •

edited

Loading

ChrisCates commented Oct 3, 2018 •

edited

Loading

nventuro commented Oct 3, 2018

Aniket-Engg commented Oct 3, 2018

ChrisCates commented Oct 3, 2018

nventuro commented May 22, 2019

nventuro commented Sep 10, 2019

ChrisCates commented Sep 10, 2019

Remove all Solidity warnings #1346

Remove all Solidity warnings #1346

Comments

nventuro commented Sep 24, 2018

ChrisCates commented Sep 24, 2018

nventuro commented Sep 24, 2018

ChrisCates commented Sep 24, 2018

nventuro commented Sep 24, 2018

ChrisCates commented Sep 24, 2018

frangio commented Sep 26, 2018

ChrisCates commented Sep 26, 2018

ChrisCates commented Sep 26, 2018

Aniket-Engg commented Oct 2, 2018

nventuro commented Oct 2, 2018 • edited Loading

ChrisCates commented Oct 3, 2018 • edited Loading

Aniket-Engg commented Oct 3, 2018 • edited Loading

ChrisCates commented Oct 3, 2018 • edited Loading

nventuro commented Oct 3, 2018

Aniket-Engg commented Oct 3, 2018

ChrisCates commented Oct 3, 2018

nventuro commented May 22, 2019

nventuro commented Sep 10, 2019

ChrisCates commented Sep 10, 2019

nventuro commented Oct 2, 2018 •

edited

Loading

ChrisCates commented Oct 3, 2018 •

edited

Loading

Aniket-Engg commented Oct 3, 2018 •

edited

Loading

ChrisCates commented Oct 3, 2018 •

edited

Loading