Openzeppelin-contracts: ERC777 change visibility of some attributes and methods from private to internal

[JonahGroendal]

🧐 Motivation
I'm trying to extend the base ERC777 contract to be pauseable, but ran into an issue. The best way I can think of to accomplish this is to override _move(), _mint() and _burn() to check if the contract is paused, but _move()'s visibility is private, so it can't be overridden. I'd also like to create setter methods for _name and _symbol but can't because they are private also.

📝 Details
I'm wondering what the rationale is for making methods/attributes private and if it's okay to change some of them to internal, namely move(), _name and _symbol. I'll submit a pull request if there are no objections.

[abcoathup]

Hi @JonahGroendal ,

State variables were made private from OpenZeppelin Contracts 2.0

https://github.com/OpenZeppelin/openzeppelin-contracts/releases/tag/v2.0.0
All state variables are now private, which means that derived contracts cannot access them directly, but have to use getters. This is to increase encapsulation, to be able to reason better about the code.

Hence _name and _symbol should stay private.

I am curious with why you want to be able to change the Name and Symbol of a token? I am not sure how well wallets would support this.

If you needed to do this, you could create private variables, e.g. something like _updateableName and _updateableSymbol and override the current getters, and implement setters (with appropriate access control).

As for making an ERC777 pausable:
ERC20Pausable overrides the interface functions with a modifier checking the pause state:
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20Pausable.sol

I assume a similar approach should be taken for ERC777, so rather than override the low level _move, the interface functions should be overriden, which then makes it clearer which functionality has been changed.

[JonahGroendal]

I'm creating a dApp that gives each user his own token, but the token contract can be created before the user signs up, so _symbol and _name must be settable after contract creation. This is probably a pretty non-standard use case, but I figured changing visibility to internal wouldn't affect anything. But it's no biggie, I can always just fork the repo. Creating 2 extra private variables would be a bit too messy I think.

As for the pauseable dilemma, I thought about doing it the way ERC20Pausable does it, as you suggest, and it would work fine, but it would take more method overrides than overriding the three helper functions. And this way if a new state-mutating method gets added to the ERC777 contract down the road (tho that may be unlikely) I wouldn't need to add an override for it.

My use-case is fairly unique so I can understand if you don't think accommodating my needs would be worth going against openzeppelin's internal standards. I think my best bet is to clone and modify the contract rather than extend it. Thanks for the reply and feel free to close the issue

[frangio]

Hi @JonahGroendal! Your use case sounds quite reasonable!

Unfortunately though, we do have a strict policy of keeping all variables private.

Rather than forking, I would highly encourage you to inherit the contract, create the 2 extra variables, and add overrides. I agree that it sounds messy, but it's more aligned with the recommended way to use the library. It's highly discouraged to fork and modify the code for yourself because you can accidentally introduce bugs, and even though this particular case is harmless, when it comes to security conventions it's better to strictly follow them always.

Similarly to the private variables, I think _move is too low-level a function for us to safely expose internally. Unfortunately you won't be able to modify send and transfer through overrides either because they're external and super.send will not compile. This is a shortcoming of our current implementation that we should fix. We closed things down as much as possible initially to keep our options open, to see what features users would need and what would be the best interface we can provide for them. Apologies that you ran into these issues!

@nventuro what do you think we should do here to enable the implementation of a Pausable ERC777?

[JonahGroendal]

send and transfer could be made public, but this would cause send to use more gas due to the data parameter.

[nventuro]

Ah, if only we had a way to limit internal access to other OZ contracts...

I have mixed feelings about implementing Pausable in terms of _move. On the one hand, it _is_ both convenient and correct, but it is not immediately obvious from looking at the function pseudo signature (including modifiers). Reverts due to e.g. a zero address are _also_ not obvious this way however, so this is not the strongest argument.

On the other, locking down the external interface via the modifier is much more explicit, though error prone (which I don't consider much of an issue given the context of this project). The biggest win here is that we won't run into a scenario where a function is pausable without us realizing it because it indirectly moves tokens around.

Regarding the name and symbol, couldn't we add a setter for these variables? I'm not sure we should do this as the base contract level however. @JonahGroendal could you expand a bit on why it is you need to deploy the token contract before you know name and symbol? That may help us come up with a better design.

[JonahGroendal]

@nventuro The long story: The novel feature of my dApp is the ability to authenticate web domain owners via X509 certificates on the blockchain. So a website owner can prove his/her identity to claim ownership of a special ENS address (e.g. example.com.dnsroot.eth). My token contracts aren't owned by the contract creator but rather by an ENS node specified at contract creation. The token contracts are created by a TokenFactory contract that can be called from any account willing to pay for the gas (presumably a user who wants to buy the token), and then the owner of the contract can authenticate and claim ownership of the token later. So any account can create a token that's owned by an ENS node and purchase tokens from it, then later the ENS node owner can withdraw the funds used to buy the tokens.

Like i said this is probably a pretty uncommon use-case. And because of y'all's rule against internal state variables, accommodating my needs would require more changes than I originally envisioned. So whatever you guys think is fine with me. The workaround that @frangio recommended would always work.

[frangio]

a scenario where a function is pausable without us realizing it because it indirectly moves tokens around.

Wouldn't this be desirable though? I would expect that if I pause the contract all moving is paused.

I still don't think exposing _move is a good idea anyway.

My proposal is to make the ERC777 interface functions public rather than external as they are now. This allows people to override them to add functionality and to create new external functions that make use of them.

[nventuro]

Wouldn't this be desirable though? I would expect that if I pause the contract all moving is paused.

It is desirable in the 'I want to pause all moving sense', but not in that it is not obvious which external functions will end up being affected by this.

[arindamghosh4995]

My proposal is to make the ERC777 interface functions public rather than external as they are now. This allows people to override them to add functionality and to create new external functions that make use of them.

@frangio Any update/decision on this, I have a use case which needs this. Happy to put up a pull request.
I have created an issue here #1994 .

[frangio]

Thank you @arindamghosh4995. Closing in favor of #1994.