OpenZeppelin Smart Contract Design Rationale

Hi there!

Hope everyone is doing well in these crazy times!

I was wondering if openzeppelin keeps any documentation regarding the rationale behind some of the design decisions you make.

For example, one of the coding style rules states that all state variables must be private - I can see this being useful to avoid unnecesary automatically-generated getters but perhaps there are other reasons like preventing the overriding on inheritance, etc…

Others, like functions being public instead of external are harder to reason. One could say you could make a function public because you want to declare parameters as memory and avoid calldata, or perhaps because you are going to use the function both externally and internall - which is not the case in Openzeppelin contracts.

All in all, I personally think it would be very valuable to know what the reasoning behind these decisions is, and learn from others experiences.

Much appreciated!

1 Like

Hello @Madness, thanks for asking this!

I’ve given this some thought a couple times, though I’m unsure what shape it could take. They wouldn’t fit as ‘smart contract development recommendations’, since many relate to the fact that OpenZeppelin Contracts is a library and not the final, complete source code for a project. I can answer those two here regardless.

State variables are private to both prevent the automatic getters that come with public, and the setters that come with internal. We find that providing direct access to state variables is error prone, and prefer to instead expose better abstractions with internal functions. For example, instead of exposing balances in ERC20, and leaving it up to the user to remember to also update the total supply and emit events when required, we provide the _transfer, _mint and _burn methods.

The reason why all external functions are public is actually not related to calldata, but to inheritance and overrides. You can override an external function, but due to restrictions in Solidity it is not possible to call the parent’s implementation from the override, which prevents lots of valid use cases. You can read more about this on this PR: https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2162

Do you find this information helpful? Where do you think it belongs, and are there any other topics we could offer clarification on? Thanks!

1 Like

Hey @nventuro, thanks for the answer.

State variables are private to both prevent the automatic getters that come with public , and the setters that come with internal . We find that providing direct access to state variables is error prone, and prefer to instead expose better abstractions with internal functions. For example, instead of exposing balances in ERC20, and leaving it up to the user to remember to also update the total supply and emit events when required, we provide the...

I was not aware of the setters on internal - good to know, I will give it some testing.

The reason why all external functions are public is actually not related to calldata , but to inheritance and overrides. You can override an external function, but due to restrictions in Solidity it is not possible to call the parent’s implementation from the override, which prevents lots of valid use cases. You can read more about this on this PR...

I see, so it's an extensibility problem. Also, kinda unrelated but, then Solidity is asking you to basically decide before-hand if you intend your code to be extensible, right ?

This specific PR refers to solidity 0.6.x virtual keyword right ?
If so, why versions prior to 3.0 of openzeppelin/contracts like the ERC20 also contain public functions ? perhaps because that's how it's defined in the standard ?

Do you find this information helpful?

Very helpful indeed.

any other topics we could offer clarification on?

Actually, I have been thinking about something for a while now, I was wondering which way would be "best".

Let's say that I have a contract, that is an extension of another, perhaps it's a token an it extends the ERC20 and adds a couple more functionalities .

For the sake of the discussion, let's also say we are using 0.5.x because this problem is, in my view, solved by interface inheritance in 0.6.x.

How would you proceed to define this interface ? Would you define it as just a IToken that contains the extra functions ? or would you go ahead and also add all the IERC20 functionality in there ?
The reason I ask this, is because of the following:

If I have another contract that uses my IToken I could say:

import "./Interfaces/IToken.sol";

Contract Foo {
  ...
}

vs

import ".Interfaces/IERC20.sol";
import ".Interfaces/IToken.sol";

Contract Foo {
  ...
}

Technically speaking, the are both the same, because the interface is only used to generate the expected ABI and functon signature.

But for me, this could also be interpreted as the first case allowing to communicate with IToken contracts, and the second allowing both ERC20 and IToken - atleast at first glance.

Perhaps this is an irrelevant question and I'm just overthinking it hahaha.

Where do you think it belongs

As to where it should live...That's a very good question hahaha. Not gonna lie, my best guess it that some of this information should be provided by the Solidity documentation, but I think that might be too much to ask from a reference source.

What would I like? Well, I think that you guys already do a very good job at providing good practices and very helpful utilities and libraries as well as valuable code to learn from.
That said, if this document is to be made I think that it should strictly focus on the thought process. i.e instead of saying "the private keyword for state variables is used because blablabla..." it should focus on the thought process that needs to happen when developing a smart contract, which is then tied to specific decissions you can make, for example regarding the visibility of your storage.

This is because, in my opinion, the rabbit hole of trying to justify every alternative is just too much, because it depends entirely on the context of the work - I think it would just be better to outline the thought process, give context and then tie it to the different design decisions that one can make.

Perhaps it can be best thought as a walk through for the developing context and how that affects the design decisions ?

Sorry for the long answer.

Hope I'm making some sense hahaha - Thx !

1 Like

@nventuro friendly poke :smiley:

1 Like

Versions 2.x targeted Solidity v0.5, which doesn't have the virtual keyword. On the v3.0 we ported to Solidity v0.6, and included that change.

It depends on what you want to do. On Solidity v0.6 interfaces can inherit other interfaces, allowing you to say things like 'my interface is that of ERC20 plus these methods'.

Funny you mentioned this, we're actually working on a number of development best practices and things you should keep in mind when working with smart contracts :slight_smile: Is that the sort of thing you had in mind?

1 Like

Yeah, I was explicitly referring to v0.5 - but no worries :smiley:

That can work, not gonna lie I don't think I will know if it's what I'm looking for until I see it hahahaha.
Anyway, I would be happy to contribute if that's something you guys are looking for :smiley:

1 Like