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

Made private methods internal to allow for overriding #2027

Merged

Conversation

MickdeGraaf
Copy link
Contributor

@MickdeGraaf MickdeGraaf commented Dec 16, 2019

Makes private functions in ERC777 internal instead of private.

Making these methods internal allows for overriding and adding of functionality in tokens developers create.

Fixes #2047

Example usecase:

In DDAI interest needs to be claimed on every transfer.
Overriding every functoin like transfer, transferfrom, send and so on would add unnecessary complexity to the code. Thats why I override the _move function.

To prevent people from making unnecessary forks of this repo(like I did) I think its wise to make all functions internal instead of private.

@stale
Copy link

stale bot commented Dec 31, 2019

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

@stale stale bot added the stale label Dec 31, 2019
@frangio
Copy link
Contributor

frangio commented Jan 1, 2020

Hi @MickdeGraaf. Really sorry it took so long to reply to this.

Overriding every functoin like transfer, transferfrom, send and so on would add unnecessary complexity to the code. Thats why I override the _move function.

@nventuro and I have discussed this and we don't think it would be right for _move to be internal. We definitely want to support extensibility and your use case in particular, and although we agree that it would be nicer to override a single function, we feel more strongly about providing an internal API that respects the important properties of the contract. In the case of ERC777, the specification requires that all sends and transfers trigger function calls on the sender and the receiver, and exposing _move internally would allow to bypass this protocol, accidentally or not. If we were to expose something internally with the purpose of enhancing extensibility it would have to be something else that preserves this protocol.

@frangio
Copy link
Contributor

frangio commented Jan 15, 2020

Hi @MickdeGraaf, are you interested in continuing this PR or should we pick it up?

@nventuro
Copy link
Contributor

Thank you for pushing for this @MickdeGraaf and @asselstine!

In v3.0 of OpenZeppelin Contracts, which will feature support for Solidity v0.6.x, we're planning on adding hooks on events such as token transfers and approvals. That should provide an easier extension mechanism than the current overriding of the internal functions, which can be dangerous. You can take a look at current progress and discussions in #2063.

Thanks again!

@nventuro nventuro merged commit 73abd54 into OpenZeppelin:master Jan 23, 2020
@MickdeGraaf
Copy link
Contributor Author

Sorry missed the conversation. Thanks for merging!

The hooks sound very interesting!

KaiRo-at pushed a commit to KaiRo-at/openzeppelin-contracts that referenced this pull request Mar 16, 2020
)

* Made private methods internal to allow for overriding

* Revert package.lock changes.

* Make _move private again

* Expose the ERC1820 registry address

* Add changelog entry

Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve ERC777 extensibility: private should instead be internal
3 participants