solidity – How to Make a Payable transferFrom Function in an OpenZeppelin ERC721 Contract

ethereumjsolidity

I have just finished the crypto zombies tutorial https://cryptozombies.io/ and wanted to get it working locally but also using a solc 0.8.0. Compilation has thrown me lots of errors, many of which I just had to change external to public in my overrides and also add the override modifier. There is one issue, however, that I am unsure how to resolve. The contract inherits from the @openzeppelin/contracts version of ERC721. The two functions I am having issues with use the payable modifier.

  function transferFrom(address _from, address _to, uint256 _tokenId) public payable override {
    require (zombieToOwner[_tokenId] == msg.sender || zombieApprovals[_tokenId] == msg.sender);
    _transfer(_from, _to, _tokenId);
  }

  function approve(address _approved, uint256 _tokenId) public payable override onlyOwnerOf(_tokenId) {
    zombieApprovals[_tokenId] = _approved;
    emit Approval(msg.sender, _approved, _tokenId);
  }

I get an error during compilation..

TypeError: Overriding function changes state mutability from "nonpayable" to "payable".

So I find that in the ERC721 contract, neither of these functions is marked as payable.

So my question is do I remove the payable modifiers in my function overrides? I thought that this would mean the function/tx cannot carry an ether value. Or do I add the modifiers to the functions in the contracts I am inheriting from? Why do the functions lack these modifiers if (from what I have read) solidity expects them to be present in these two functions? Or am I missing something? I am unsure how to proceed so any help would be massively appreciated!

Best Answer

Only use payable when you're explicitly sending WETH. Sending tokens or NFTs does not inheritably cost WETH (only gas). If the transfer function calls another function it will inherit payable from the called function.

Related Solutions

Solidity Remix Warnings – No Visibility Specified, Violate Checks-Effects-Interaction Pattern

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.

Solidity – Handling Unimplemented Functions in Contracts

The problem is that totalSupply() implementation is missing. Seems like autogenerated getters do not count. So you need to explicitly create the function.

contract StandardToken is Token {
    uint256 _totalSupply;

    function totalSupply() constant returns (uint256 totalSupply) {
        totalSupply = _totalSupply;
    }
}

Best Answer

Related Solutions

Solidity Remix Warnings – No Visibility Specified, Violate Checks-Effects-Interaction Pattern

Solidity – Handling Unimplemented Functions in Contracts

Related Topic