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

Inheritance: allow stricter mutability #3412

Closed
fulldecent opened this issue Jan 20, 2018 · 12 comments · Fixed by #9344
Closed

Inheritance: allow stricter mutability #3412

fulldecent opened this issue Jan 20, 2018 · 12 comments · Fixed by #9344
Assignees
Labels
language design :rage4: Any changes to the language, e.g. new features

Comments

@fulldecent
Copy link
Contributor

Code example 1:

pragma solidity ^0.4.19;

interface NamedThing {
    function name() public view returns (string _name);
}

contract Bob is NamedThing {
    string public constant name = "Bob";
}

Code example 2:

pragma solidity ^0.4.19;

interface NamedThing {
    function name() public view returns (string _name);
}

contract Bob is NamedThing {
    function name() public pure returns (string) {
        return "Bob";
    }
}

Discussion

These two cases illustrate where an implementing contract meets the requirements of an interface. However, both cases are rejected by the compiler with the error:

Overriding function changes state mutability from "view" to XXX

Mutability is expressed in these forms:

  1. Payable
  2. Nonpayable
  3. View
  4. Pure

These are strictly ordered. All pures are views, all views are nonpayables. All nonpayables are payables.

Recommendation

The compiler should allow class inheritance / interface implementation if the downstream function has a stronger mutability guarantee.

@fulldecent
Copy link
Contributor Author

pragma solidity ^0.4.19;

contract Bob {
    function name() public pure returns (string) {
        return "Bob";
    }
    
    modifier makePureToView {
        require(address(this) != address(0));
        _;
    }
    
    modifier makeViewNonpayable {
        address[] storage a;
        _;
    }
    
    modifier makeNonpayablePayable {
        require(msg.value == 0);
        _;
    }
}

@fulldecent fulldecent changed the title Implement interface: allow stricter visibility Implement interface: allow stricter mutability Jan 20, 2018
@axic axic added the feature label Jan 20, 2018
@axic
Copy link
Member

axic commented Jan 20, 2018

I think this is a good idea to allow reducing the state mutability levels during inheritance, but we should really examine all the effects before implementing the change.

Question is: should the external interface be based on the final implementation or the interface?

@fulldecent
Copy link
Contributor Author

For the "ABI" (which is not actually a binary interface), the stricter guarantees should be presented.

And for the actual external interface, i.e. EVM, there is no difference since this is not part of the function signature.

@fulldecent
Copy link
Contributor Author

fulldecent commented Feb 4, 2018

@axic Could you please help identify stakeholders for this issue and who would need to sign off? And we can query their issues/questions/concerns.

I am hoping to have this issue "accepted". This means we can be confident it will be implemented at some point (do we have a GitHub issue label for that?)

I care about this because standards work depends on this. Right now that standard (draft) is written in a way that assumes #3412 will be implemented at some point. It would be very helpful if I could justify such an assertion.

References:

@fulldecent
Copy link
Contributor Author

Also can we please evaluate if this test case should pass or fail:

Test case 1

pragma solidity ^0.4.20;

interface Math {
    function calculateMostCommonNumberInSolidityDocumentation() external payable returns (int);
}

contract MathImplementation is Math {
    function calculateMostCommonNumberInSolidityDocumentation() external returns (int) {
        return 69;
    }
}

Test case 2

pragma solidity ^0.4.20;

interface Math {
    function calculateMostCommonNumberInSolidityDocumentation() external returns (int);
}

contract MathImplementation is Math {
    function calculateMostCommonNumberInSolidityDocumentation() external payable returns (int) {
        return 69;
    }
}

Discussion

In the discussion of the deed/NFT/DAR standard ethereum/EIPs#841 we have specified which functions are payable and which are not. The 0x team argues that it is not in scope for a standard (an interface) to specify whether functions are payable.

I think this discussion has wider applicability in the Solidity ecosystem so I have brought it here.

@fulldecent
Copy link
Contributor Author

@axic Sorry, posted to the wrong place. Moving here.

Referencing your comment #3458 (comment)

I think the current specific rule in the code is that a contract implementing an interface may not modify the state mutability level of a function.

State mutability is defined as (the only summary - there is also another suggestion to move selfdestruct into another state mutability level):

stateMutability: a string with one of the following values:

  • pure (specified to not read blockchain state),
  • view (specified to not modify the blockchain state),
  • nonpayable (cannot accept value transfers) and
  • payable (can accept value transfers).

Above the state mutability level is increasing from pure (nothing) to payable (everything).

What are the reasons proposed for disregarding the payable keyword for interfaces?

@chriseth
Copy link
Contributor

Agreement from call: We should have this.

Unless anyone can come up with an example why this might be a bad example, we should check it on the inheritance graph instead of the linearized hierarchy.

@chriseth chriseth changed the title Implement interface: allow stricter mutability Inheritance: allow stricter mutability Mar 30, 2020
@elenadimitrova elenadimitrova added this to New issues in Solidity via automation May 14, 2020
@elenadimitrova elenadimitrova moved this from New issues to Implementation Backlog in Solidity May 14, 2020
@chriseth
Copy link
Contributor

chriseth commented Jul 1, 2020

Discussion: maybe take payable / nonpayable out because it is more complicated.

@ekpyron
Copy link
Member

ekpyron commented Jul 1, 2020

I'm fine with taking payable/nonpayable out in the first step. Once we want to go for it, I'd say this:

Overriding something payable with something non-payable is fine (after all you could also implement a payable function with require(msg.value == 0); in the beginning) - but overriding something non-payable with something payable is not fine (since calling in the base contract should always be able to assume that there is a callvalue check).

Actually if we don't allow overriding payable with non-payable, then we should also not allow overriding payable with view or pure, because it has the same problem. But I think the following is fine:

The as far as I'm concerned, the order is payable < non-payable < view < pure, increasing is fine, decreasing isn't.

@Marenz Marenz self-assigned this Jul 6, 2020
@chriseth chriseth moved this from Implementation Backlog to In progress in Solidity Jul 7, 2020
@axic
Copy link
Member

axic commented Jul 7, 2020

Overriding something payable with something non-payable is fine (after all you could also implement a payable function with require(msg.value == 0); in the beginning)

I think that is correct, but at the same time it just opens the door for unwanted mistakes. We should also decide how much we try to ensure people avoid mistakes vs. "somehow forcing them" to be more depending on tests and not the compiler.

@chriseth
Copy link
Contributor

Implemented in #9344

Consolidate inheritance rules automation moved this from To do to Done Jul 21, 2020
Solidity automation moved this from In progress to Done Jul 21, 2020
kophyo1234 added a commit to kophyo1234/openzeppelin-contracts that referenced this issue Jan 29, 2022
Standards

ERC-20 Token Standard.
ERC-165 Standard Interface Detection.
ERC-173 Owned Standard.
ERC-223 Token Standard.
ERC-677 transferAndCall Token Standard.
ERC-827 Token Standard.
Ethereum Name Service (ENS). https://ens.domains
Instagram – What’s the Image Resolution? https://help.instagram.com/1631821640426723
JSON Schema. https://json-schema.org/
Multiaddr. https://github.com/multiformats/multiaddr
RFC 2119 Key words for use in RFCs to Indicate Requirement Levels. https://www.ietf.org/rfc/rfc2119.txt
Issues

The Original ERC-721 Issue. ethereum/EIPs#721
Solidity Issue OpenZeppelin#2330 – Interface Functions are External. ethereum/solidity#2330
Solidity Issue OpenZeppelin#3412 – Implement Interface: Allow Stricter Mutability. ethereum/solidity#3412
Solidity Issue OpenZeppelin#3419 – Interfaces Can’t Inherit. ethereum/solidity#3419
Solidity Issue OpenZeppelin#3494 – Compiler Incorrectly Reasons About the selector Function. ethereum/solidity#3494
Solidity Issue OpenZeppelin#3544 – Cannot Calculate Selector of Function Named transfer. ethereum/solidity#3544
CryptoKitties Bounty Issue OpenZeppelin#4 – Listing all Kitties Owned by a User is O(n^2). dapperlabs/cryptokitties-bounty#4
OpenZeppelin Issue OpenZeppelin#438 – Implementation of approve method violates ERC20 standard. OpenZeppelin#438
Solidity DelegateCallReturnValue Bug. https://solidity.readthedocs.io/en/develop/bugs.html#DelegateCallReturnValue
Discussions

Reddit (announcement of first live discussion). https://www.reddit.com/r/ethereum/comments/7r2ena/friday_119_live_discussion_on_erc_nonfungible/
Gitter #EIPs (announcement of first live discussion). https://gitter.im/ethereum/EIPs?at=5a5f823fb48e8c3566f0a5e7
ERC-721 (announcement of first live discussion). ethereum/EIPs#721 (comment)
ETHDenver 2018. https://ethdenver.com
NFT Implementations and Other Projects

CryptoKitties. https://www.cryptokitties.co
0xcert ERC-721 Token. https://github.com/0xcert/ethereum-erc721
Su Squares. https://tenthousandsu.com
Decentraland. https://decentraland.org
CryptoPunks. https://www.larvalabs.com/cryptopunks
DMarket. https://www.dmarket.io
Enjin Coin. https://enjincoin.io
Ubitquity. https://www.ubitquity.io
Propy. https://tokensale.propy.com
CryptoKitties Deployed Contract. https://etherscan.io/address/0x06012c8cf97bead5deae237070f9587f8e7a266d#code
Su Squares Bug Bounty Program. https://github.com/fulldecent/su-squares-bounty
XXXXERC721. https://github.com/fulldecent/erc721-example
ERC721ExampleDeed. https://github.com/nastassiasachs/ERC721ExampleDeed
Curio Cards. https://mycuriocards.com
Rare Pepe. https://rarepepewallet.com
Auctionhouse Asset Interface. https://github.com/dob/auctionhouse/blob/master/contracts/Asset.sol
OpenZeppelin SafeERC20.sol Implementation. https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/contracts/token/ERC20/SafeERC20.sol
@fulldecent
Copy link
Contributor Author

Is there any appetite here to allow a payable to be overridden by a stricter type (non-payable, view, pure), the way that other override-with-stricter is possible?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
language design :rage4: Any changes to the language, e.g. new features
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

5 participants