Improve encapsulation in the Heritable contract

[eternauta1337]

heir is public and has a setter setHeir

Since we are providing a setter, we should also provide a getter and make heir private. It ensures that inheriting contracts cannot modify the heir in an unintended way.

This is a general practice we are starting to adopt in all OpenZeppelin, i.e. more extensive use of private contract variables.

We may want to consider this for heartbeatTimeout and timeOfDeath as well.

@azavalla something that might interest you?

[trejas]

Should the getters be accessible only to the owner of the contract, or should they be publicly accessible?

[shrugs]

The getter should be public, imo.

[azavalla]

Reducing the public API of the contracts minimizes the number of guarantees we have to make to the code using our contract (thus reducing the places where we have to watch out for compatibility issues), giving us more freedom to change -private- implementation details.

Also, being a framework focused on security, it makes sense to try and avoid having developers extending a contract in an unintended way. But to be honest, I'm not sure making members private provides you that much safety in the face of a inheriting contracts making an unintended use. Explicitly overriding a method/variable is a conscious decision. I guess it makes sense if you add logic to the setter to check an invariant.

The cost you pay is extendability. We are restricting the library only to the intended use cases, i.e. the ones we can imagine right now.

IMHO, given OZ is an open source library, if there's doubts about the appropiate visibility, we should choose the less strict approach if we want to promote people using and extending OZ. This way we wont promote people copy-pasting a contract just because they had a slightly different use case we couldn't think of.

contract Heritable, likewise most of OZ contracts, is designed to be inheritable. I think the approach in the general case should be:
"Are you sure that a subclass will never want to override this method ? Make it private, else make it internal or public (and document it accordingly)."

@ajsantander what do you think?

[frangio]

Fixed in #702.

[azavalla]

@frangio any thoughts on this ?

[frangio]

We agree that there's a trade-off between extensibility and security.

I guess it makes sense if you add logic to the setter to check an invariant.

Yeah, in fact, this is aligned with the reasoning behind this issue. In this case it was not checking an invariant that we wanted to enforce, but emitting the event HeirChanged and the heartbeat whenever the state variable is changed. It's true that if a state variable didn't have any sort of additional logic necessary when modified, or invariants to be maintained, it doesn't need to be private.

Explicitly overriding a method/variable is a conscious decision.

I disagree with this as an argument, because we aim to provide APIs which are hard to use insecurely. This includes external APIs, as well as internal APIs for contracts that extend via inheritance. If one of our contracts has a public or internal state variable heir, the setter for that variable heir = ... is part of our internal API, i.e. we are telling the developer "feel free to set this variable to any value you want, do not expect any odd consequences". Not emitting the HeirChanged event is such an odd consequence, so it made sense to me to make it private.

There is still something a little bit weird: a contract could override the setHeir function. The language provides no way to prevent that, but we would probably use it if it did (ethereum/solidity#463). Here's where we see ourselves forced to draw a line and say: overriding this method would be a conscious decision and the developer should know that they may be causing trouble. But I feel that overriding a function is an even more conscious decision than setting a variable, which is why I'm more comfortable with it.

[frangio]

IMHO, given OZ is an open source library, if there's doubts about the appropiate visibility, we should choose the less strict approach if we want to promote people using and extending OZ. This way we wont promote people copy-pasting a contract just because they had a slightly different use case we couldn't think of.

I definitely see where you're going with this and I share a similar concern. I think that we want to promote people extending OZ in secure ways, and we have to set the library up for being securely extended. private state variables enable more secure extension. Even if they may preclude some types of extension, we have to design our APIs so that the extensions precluded are insecure ones. In the case of Heritable, setting the heir without emitting the HeirChanged event would be considered insecure.

I hope the reasoning was made clearer now. Let me know if you have any other thoughts about it. :-)