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

feat: add 1363 implementation #3487

Conversation

vittominacori
Copy link
Contributor

Proposal to add the ERC1363 implementation as clarified in the official EIP (pending) PR.

Discussion started here #2620 but it substitute the previous PR about ERC1363 as it uses a different approach.

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@Amxx
Copy link
Collaborator

Amxx commented Jun 17, 2022

Why open a PR when there is already one open for that #3017

Let not multiply the threads where the conversation is happening.

@Amxx Amxx closed this Jun 17, 2022
@vittominacori
Copy link
Contributor Author

@Amxx I've created a code structure following other OZ contracts.
Params naming was taken from OZ ERC20.
Why you are adding only a mock contract for all?
Why interfaces have not their folder under token folder?
I've also added complete test case with 100% coverage.

Anyway if you want to go ahead with your implementation feel free to keep your code.
I think Open Zeppelin is going to be less open than in the past.

@Amxx
Copy link
Collaborator

Amxx commented Jun 17, 2022

If you believe the current PR is not right, feel free to propose changes. I'm sorry but having competing PR just adds more work for maintainers to track, and duplicates are always closed. This is not the first or the last time it happens.

That doesn't make OZ less open.

I'd also like to point out, that for about 2.5 years, ERC1363 has been final and not part of OZ's repo. Apparently, nobody cared. I spent time and effort pushing for it over the past months, both internally in the OZ team, and publicly at EthMagicians gatherings and on Twitter. I tried to create awareness about what this can solve, while also discussing the possible limitations.

I honestly regret having initiated that discussion.

Over the past 72 hours, its been a stream of (sometimes harsh) messages telling me I'm not doing things the right way. I'm reaching a point where I would strongly consider stopping my support of any such ERC, and refusing to include that in OZ, because of a lack of consensus in the community, the absence of discussion of the ERC before it was finalized (the cat herders would not accept that today), and the plethora of other "standard-proposal" that are possibly better designed and have a stronger chance to reach consensus.

@vittominacori
Copy link
Contributor Author

I really thank you for discussing about it.
I use OZ from my first solidity days and I always try to write my code use the OZ guidelines also when I work on other projects.

This PR code structure follow in my opinion the OZ code structure and guidelines, while the previous seems to be in draft or in test on in ongoing mode.

As it is a first add of code, was simpler for me to create from scratch instead of edit your PR that has already many edits and a completely different test cases. I thought it was simpler to read.

@vittominacori vittominacori mentioned this pull request Jun 17, 2022
3 tasks
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.

2 participants