[Ethereum] Solidity: Function state mutability warning

In general

You can let the code run, ignoring the compiler warnings, but it's always good not to do so. Most of the time, compiler warnings are warning you about something that may cause a bug in your code. Compared to compiler errors, the difference is your code is syntactically correct, hence can be compiled, but can lead to problems, especially when someone try to do something you don't expect others to do.

Compiler warnings indicate things that might cause problems or might have unintended effects that the programmer wasn't aware of.

A good read regarding this can be found here, and you may refer to this question as well. And most importantly, read the security considerations from Solidity docs. That may save your life!

Anyhow, the compiler is not as intelligent as you and it claims what it's lacking from in statements with Note: as follows:

Note: Modifiers are currently not considered by this static analysis.

Sometimes, these notes may tell the occasions that you can safely ignore these warnings.

Example

One error warning you have mentioned is:

browser/TestContract 8.sol:15:3 Warning: No visibility specified. Defaulting to "public":

Let's say you have function A(uint val) {}. You want it to be used only within the contract due to some prior validations you need to do (This is just an example ;)). And if the function is called without validation, that may cause some unwanted executions. And the only use of function A is to use it inside some other functions within the contract.

pragma solidity ^0.4.0;

contract Example {


    function A(uint val) {
       //do something with val 
    }

    function B(uint val){
        //validation of val
        A(val);
    }

}

Now if you keep the contract like above, since function A() is also by default set to public, anyone who use your contract can use function A with any uint value and that may cause trouble. But if you had the access modifier of function A declared as private, this trouble won't appear since function A can't be accessed publicly. And as your requirement is also to use function A only inside the other functions within the contract, your goal is also achieved.

As when no access modifier is declared, the compiler assumes a function to be public and it warns you that it's setting the function to be public and ask you to check whether it should really be public. In the above case, you will get warnings regarding both functions and setting function A to be private and function B to be public will remove the warnings because now the compiler knows you are intentionally making function B public and function A private.

pragma solidity ^0.4.0;

contract Example {


    function A(uint val) private {
       //do something with val 
    }

    function B(uint val)public {
        //validation of val
        A(val);
    }

}

The warning regarding function B being set to public is a type of warning that you can safely ignore as your intention is also the same, but the warning regarding function A is something you should really consider.

What I suggest,

• not to ignore the warning, but go through them and check whether they do really matter, with how you expect the code to behave and try to resolve them.

• try to use the latest compiler version (atm 0.4.17), as newer stable versions tend to have less bugs and more features (hence it generates new warnings).

Some insight regarding some other warnings you get

browser/TestContract 8.sol:100:3 Warning: Function state mutability can be restricted to pure

You may refer to this answer and this question regarding the introduction of view and pure type functions.

Potential Violation of Checks-Effects-Interaction pattern in TestContract.testFunction(address):

This is for not following the pattern of Checks-Effects-Interactions as described here in the docs. Refer to the function that gets this warning and follow the pattern defined in the docs to avoid the warning.

And as per the code found in the link you posted, I think the closeSale() warning should probably be due to the compiler assuming that there can be a chance of re-entrancy, as you are calling the member event LogSaleClosed(uint256 issuedSupply, uint256 restrictedTokens) after interactions with token contract.

What I can suggest is having that event inside the contract token. You can see here for re-entrancy vulnerability. It's just saying if the bad code there let another contract to withdraw money several times because they change the state variable shares after sending transaction which might take some time leaving the other contract to call the same function several times because it keeps msg.sender value non-zero until the transaction get executed. If you see the good code there, they first set shares[msg.sender] to zero first. Since you are the one who developed this, you know the flow of expected events and can be sure that re-entrancy won't occur, it's safe to ignore it.

Test.transferFrom(address,address,uint256): Potentially should be constant but is not. Note: Modifiers are currently not considered by this static analysis.

What constant means is that it would not be changing the state. That's no state variable is changed. So normally, those functions are called constant functions and are defined with the keyword of constant. You may read this.

So if your functions are not changing any state variable, you can declare them constant. Solidity is reminding you about this.

View can be used to with a function that does not modify the state but reads state variables.

Pure should be used with functions that neither modify state nor read ( access) state variables. They generally perform operations based on input params.

An example illustrating the same is here:

pragma solidity ^0.4.24;

contract ViewVsPure {
  uint public age = 18;

  function addToAge(uint _no) public view returns (uint){
    return age + _no;
  }

  function add(uint _a, uint _b) public pure returns (uint) {
    return _a + _b;
  }
}