Made ERC777 registry and private functions internal.

[asselstine]

Motivation explained here:

#2047

🧐 Motivation

I'm subclassing ERC777 to build custom tokens. There is some functionality that would beneficial to use, such as _burn and _callTokensToSend, but isn't made available to subclasses because their visibility is private.

📝 Details

My request is that functions that are private instead become internal to allow for easier extensibility.

I don't want to cut-and-paste anymore!

[nventuro]

This is sort of a duplicate of #2027. There are two differences however, and I'm curious about those.

1. Why do you need access to the 1820 registry?
2. What are your thoughts on overriding _move and the relevant discussion on Made private methods internal to allow for overriding #2027?

[asselstine]

Good questions @nventuro.

Why do you need access to the 1820 registry?

My subclass needs to interact with the registry. Changing it to internal is useful and doesn't break encapsulation. It's just a constant.

What are your thoughts on overriding _move and the relevant discussion on #2027?

This is an interesting question. I do think having internal members will encourage more code re-use, as forced encapsulation will just cause developers to cut-and-paste. Determining the correctness of their overrides will be left to auditors.

For me specifically the reason I want to override functions is because I'm using a different data structure to store user balances. However, since this PR was created I've found a cleaner way to provide the extensibility I need by separating the ERC777 behaviour from the ERC777 state.

Have a look at a comparison of my fork here. You'll notice that any updates to _balances or _totalSupply are done through internal getter and setter functions. Now I can override those functions and use my own data structure to track balances.

I'd love to see this kind of extensibility in the OZ token code so that I don't have to fork it.

Thoughts?

Also a quick note: it's unclear to me whether to create a PR for openzeppelin-contracts or openzeppelin-contracts-ethereum-package. I use the latter project but it seems that changes flow from the original contracts version?

[nventuro]

For me specifically the reason I want to override functions is because I'm using a different data structure to store user balances. However, since this PR was created I've found a cleaner way to provide the extensibility I need by separating the ERC777 behaviour from the ERC777 state.

Interesting! I'm not sure it's feasible however for us to provide overrideable getters and setters to all internal state: not only does that sound like a maintenance hell, but in practice it'd also be rather difficult to override all required bits correctly. Could you expand a bit on your need for a custom data structure?

Also a quick note: it's unclear to me whether to create a PR for openzeppelin-contracts or openzeppelin-contracts-ethereum-package. I use the latter project but it seems that changes flow from the original contracts version?

Do make the PRs here, we port them over as new releases come out. And we may drop the need for that separate repo altogether soon 😉

[asselstine]

The custom data structure I'm referring to is Kleros' SortitionSumTreeFactory. It allows one to select an account with probability weighted by their balance.

The getters and setters may only be applicable to my situation, so perhaps they belong in a fork. However, making private members internal would really help me in terms of code re-use.

[frangio]

Note also that we've recently announced that getters will not be virtual, so overriding them wouldn't be possible.

Unfortunately I think this is the kind of modification that warrants a fork. Overriding getters and setters of internal state through inheritance is such a drastic change that I'd argue there would be no benefit compared to just forking. It's probably better to fork and make the changes inline to keep the code simpler.

Your use case seems analogous to our ERC20Snapshot, which needs to keep track of balances and update a separate data structure. In that case we achieved it through overriding the internal functions such as _transfer. In the case of ERC777 the number of operations that need to be tracked is much greater though, which has come up before in the context of #2027. We may have to think of a mechanism to more easily track the operations internally. Would that kind of thing work for you?

[asselstine]

The ERC20Snapshot is similar to my use case; the only difference being that since the sortition sum tree stores the balances I can eliminate the updates to the _balances to reduce gas consumption.

I'd be happy if ERC777 was brought more in-line with ERC20 in terms what it exposes: ERC20Snapshot is possible because more members are internal instead of private, allowing for overrides. This change alone would be very beneficial to code re-use.

[frangio]

Got it! We'll expedite #2027!