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

Implement interface: allow compatible visibility #3458

Closed
fulldecent opened this issue Feb 4, 2018 · 3 comments
Closed

Implement interface: allow compatible visibility #3458

fulldecent opened this issue Feb 4, 2018 · 3 comments

Comments

@fulldecent
Copy link
Contributor

Discussion

Currently, interface is meant to track ABI (again, I hate this word choice). Here is a contract that demonstrates public and external functions do have identical ABI:

contract MathABITest {
    function a() external pure {
    }
    function b() public pure {
    }
}

And the ABI calculated by Solidity is:

[
	{
		"constant": true,
		"inputs": [],
		"name": "b",
		"outputs": [],
		"payable": false,
		"stateMutability": "pure",
		"type": "function"
	},
	{
		"constant": true,
		"inputs": [],
		"name": "a",
		"outputs": [],
		"payable": false,
		"stateMutability": "pure",
		"type": "function"
	}
]

Therefore, a contract that uses an public or external function should be compatible with an interface that requires an external function.

Test case

pragma solidity ^0.4.19;

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

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

This should produce no error. But currently the error is:

TypeError: Overriding function visibility differs.

References

Related issues:

@fulldecent
Copy link
Contributor Author

fulldecent commented Feb 21, 2018

Sorry I posted here, but it should have been #3412.

OLD COMMENT 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.

@axic
Copy link
Member

axic commented Feb 21, 2018

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

chriseth commented Mar 7, 2018

Coming back to the initial proposal again: I think it should not be an error if a public function overrides an external function. The requirement for interface functions to be external makes their use extremely annoying. Because of that, I think this should be part of 0.5.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants