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

Initial proposal for ERC2135 and Ticket721 Draft. #1838

Closed
wants to merge 6 commits into from

Conversation

xinbenlv
Copy link
Contributor

@xinbenlv xinbenlv commented Jul 23, 2019

Hi Open Zeppelin Community, I'd like to add the ERC2135 and one of its reference implementation to Open Zeppelin repository from https://github.com/xinbenlv/eip-2135/tree/master/impl.

Presubmit CheckList

Note: I don't know how to update the changelog or whether it applies - @xinbenlv

@xinbenlv
Copy link
Contributor Author

xinbenlv commented Jul 23, 2019

Give the newly added contract /consumption coverage is 100% https://coveralls.io/jobs/51165007, I believe the coveralls failure is a false positive.

Update: I take this comment back, it's caused by: https://coveralls.io/jobs/51165007/source_files/1550379330#L16

@xinbenlv
Copy link
Contributor Author

Can anyone kindly help conduct the first review?

@nventuro
Copy link
Contributor

Hello @xinbenlv, thanks for contributing to OpenZeppelin! I'm sorry we took so long to review this.

When adding new features (specially new standard interfaces) we usually look for potential use cases that call for them, or demand from the community. Could you share a bit more about projects you're working on where this need came up?

I can see how burning tokens might be useful (which is why we provide _burn), but am unsure as to whether a full-fledged standard interface for doing so is the way to go.

@xinbenlv
Copy link
Contributor Author

xinbenlv commented Jul 31, 2019

Hi @nventuro yes, thank you for a review and suggestion, and being both very encouraging and frank for discussion.

Allow me to defend the necessity of the ERC and an standard implementation. I am open to be convinced too.

I think it makes sense to have a burn or consume like ERC to standardize this behavior. The fact that our OpenZapellin version implementation of ERC20 and ERC721 both have found the need of implementing a burn function speaks for a need. This address the "need/use-case" question of a such behavior.

I believe other kinds of Tokens and virtual asset standards are currently being actively proposed among the EIPs community as well as other future interfaces, they will find use-case of consume and the consume activity be recorded.

And the next question whether it needs a ( full fledge) ERC to help standardize such behavior.
I believe if consume behavior potentially have many usages, it's better to have a standard so different implementations or applications can inter-operate.

If the only concern is that is too trivial, I would compare it with ERC-173 Ownership which is also very simple. Such standard, despite having only a few seemingly trivial methods, help people form consensus to how an ownership is defined, what name of function and events should be and what kind of parameters they should have, in what order. These are the value of standardization.

One other thing: actually, you reminded me, that for backward compatibility purpose, whether we should rename the ERC-2135 main function consume to burn, I would like to have your advice.

  • burn make it immediately compatible to what is already in OpenZapellin implementation, which already have usage.
  • consume is probably more generic to represent its meaning.

Thank you!

@xinbenlv
Copy link
Contributor Author

xinbenlv commented Aug 8, 2019

Friendly ping for another round of review / response from reviewers

@xinbenlv
Copy link
Contributor Author

Friendly ping again~

@nventuro
Copy link
Contributor

Hello @xinbenlv! Sorry again for taking so long to look at this, the last couple weeks where a bit hectic with Berlin blockchain week.

I think it makes sense to have a burn or consume like ERC to standardize this behavior. The fact that our OpenZapellin version implementation of ERC20 and ERC721 both have found the need of implementing a burn function speaks for a need. This address the "need/use-case" question of a such behavior.

Your points make a lot of sense, I think this is just me being generally disillusioned with EIPs 😅 My belief is whatever big application comes forward will be the de-facto standard, but I do see the value in thought-out and written-down specs. That said, it'd be great to have the EIP finalized: we avoid merging stuff in draft state, to prevent having to change them in the future.

One other thing: actually, you reminded me, that for backward compatibility purpose, whether we should rename the ERC-2135 main function consume to burn, I would like to have your advice.
burn make it immediately compatible to what is already in OpenZapellin implementation, which already have usage.
consume is probably more generic to represent its meaning.

API stability is very important to us, and I wouldn't want to have both burn and consume methods. In this case it may be best to, like EIP173 did, simply write down a spec for the behavior currently out there. I'd suggest taking a look at how ERC777 does burning, and see if you can imitate that.

@frangio
Copy link
Contributor

frangio commented Aug 30, 2019

I think there is value in stuff like ERC173 and the ERC2135 proposed in this PR. However, IMO it would be best for the ERC to standardize the behavior and names that are already in use, instead of introducing new ones. So I would go for burn instead of consume, etcetera.

@frangio
Copy link
Contributor

frangio commented Aug 30, 2019

I don't really understand the role of Ticket721 in this. Would you mind explaining?

@xinbenlv
Copy link
Contributor Author

xinbenlv commented Aug 30, 2019

@nventuro

I'd suggest taking a look at how ERC777 does burning, and see if you can imitate that.

@frangio

So I would go for burn instead of consume, etcetera.

Agreed to both, compatibility is probably the most important when it comes to adoptions. I lean towards using "burn" as well now.

I don't really understand the role of Ticket721 in this. Would you mind explaining?

The role of Ticket721 is to demonstrate what will a typical "ticket" like in EventBrite (being both transferable and consumable) look like, under the ERC2135 contract standard. Does this explanation answers your question?

@frangio
Copy link
Contributor

frangio commented Sep 2, 2019

Yes, I think I understand. I was asking to understand whether it's something that should be included in the OpenZeppelin Contracts or if it's more like a special-purpose example application. I'm leaning towards the second.

@xinbenlv
Copy link
Contributor Author

xinbenlv commented Sep 2, 2019

@frangio thanks for clarifying. I humbly argue that it is the first case: it is more of a general case possible being referenced / used by many applications as part of their set of contracts.

@stale
Copy link

stale bot commented Sep 17, 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 Sep 17, 2019
@xinbenlv
Copy link
Contributor Author

Hah, thank you Stalebot. I am working on communicating the preferred interface and waiting for community members input to further revise

@stale stale bot removed the stale label Sep 17, 2019
@stale
Copy link

stale bot commented Oct 2, 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 Oct 2, 2019
@xinbenlv
Copy link
Contributor Author

xinbenlv commented Oct 2, 2019

It's still waiting for review.

@stale stale bot removed the stale label Oct 2, 2019
@xinbenlv
Copy link
Contributor Author

xinbenlv commented Oct 2, 2019

Actually, hold on, I will update the interface to reflect another conversation's suggestion

@frangio
Copy link
Contributor

frangio commented Oct 7, 2019

We've decided not to merge this because there doesn't seem to be a lot of interest in the ERC, and moreover it's still a Draft, which we usually don't include because the API may change and we need to keep a stable API.

Sorry for taking a while to come to a definitive answer. Thank you @xinbenlv!

@frangio frangio closed this Oct 7, 2019
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.

3 participants