Skip to content

erc721 mintable and bouncer updates#1020

Closed
shrugs wants to merge 10 commits into
OpenZeppelin:masterfrom
shrugs:feat/erc721-mintable-and-bouncer-updates
Closed

erc721 mintable and bouncer updates#1020
shrugs wants to merge 10 commits into
OpenZeppelin:masterfrom
shrugs:feat/erc721-mintable-and-bouncer-updates

Conversation

@shrugs

@shrugs shrugs commented Jun 16, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Supercedes #957 and #950

fixes #962

🚀 Description

cc @vittominacori for review

  • DefaultTokenURI.sol — implement tokenURI(uint256 _tokenId) and returns a default tokenURI if a specific token URI is not set. saves the developer from storing the same tokenURI for every token

  • RBACMintable.sol — a super simple mintable role with onlyMinter modifier. Does not suggest role authorization, which is left up to the implementer. Pairs well with RBACOwnable

  • RBACOwnable — an Ownable implementation that allows multiple owners that can add/remove themselves

  • MintableERC721Token.sol — an ERC721Token that can be minted by anyone with the minter role.

  • ERC721Burnable — an interface to support burnFrom() mechanism

  • BurnableERC721Token — an implementation of that interface

  • 📘 I've reviewed the OpenZeppelin Contributor Guidelines

  • ✅ I've added tests where applicable to test my new functionality.

  • 📖 I've made sure that my contracts are well-documented.

  • 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

@shrugs shrugs requested a review from frangio June 16, 2018 23:18
@vittominacori vittominacori mentioned this pull request Jun 17, 2018
Closed
@vittominacori

vittominacori commented Jun 17, 2018

Copy link
Copy Markdown
Contributor

@shrugs pushed in your repo

shrugs#1

It should solve test failing.

@shrugs shrugs force-pushed the feat/erc721-mintable-and-bouncer-updates branch from 672a7a2 to e8a8333 Compare June 17, 2018 21:08
@frangio

frangio commented Jun 17, 2018

Copy link
Copy Markdown
Contributor

I see a few problems with this PR.

It's very complex, and doesn't feel very cohesive. There's additions to our ERC721 stack, SignatureBouncer and RBAC, as well as new stuff like NonceTracker and AutoIncrementing. It's very hard to review such disparate components. The huge new context in which they are presented results in a lot of overhead. We should aim for more cohesive and as small as possible PRs. I believe this will lead to better reviews by making better use of our time and focus. (In the possible case that a huge feature has to be developed, it should probably be done in an ongoing in progress PR with incremental reviews from the same reviewer.)

Additionally, I'm not quite convinced that ERC721Minter should be a part of OpenZeppelin. Although SignatureBouncer is very interesting, we shouldn't start writing bouncers for every feature in the library. ERC721Minter is a fine example of how a developer might build a system by composing a MintableERC721Token with a SignatureBouncer. What we should aim for is designing, writing, and documenting building blocks that can be safely composed by developers with little code. ERC721Minter is a success in this sense: it is very little code doing something quite powerful built out of OpenZeppelin components, but it is not something that I would include in the library.

What do you think about this?

My concrete proposal is to continue work on #957, and then merge ERC721Minter only as an example (i.e. in the examples/ directory).

@shrugs

shrugs commented Jun 17, 2018

Copy link
Copy Markdown
Contributor Author

@frangio Keeping ERC721Minter inside examples is good 👍

This PR is super huge, and it totally is all over the place, yeah. With the removal of ERC721Minter we can split noncetracker, autoincrementing, and signature bouncer changes into separate PRs. Will do that unless you have extra thoughts.

@shrugs

shrugs commented Jun 17, 2018

Copy link
Copy Markdown
Contributor Author

@frangio thanks for the feedback, I feel a lot better about the PR now. split them into

#1022
#1023
#1024

@shrugs shrugs force-pushed the feat/erc721-mintable-and-bouncer-updates branch from 26339ad to 4e2242c Compare June 26, 2018 22:59
@nventuro

nventuro commented Jul 20, 2018

Copy link
Copy Markdown
Contributor

@shrugs should we close this one then, since work continued on those other 3 PRs?

@shrugs

shrugs commented Jul 20, 2018

Copy link
Copy Markdown
Contributor Author

@nventuro this PR will have to eventually be rebased on master once those commits are merged

@nventuro nventuro added in progress on hold Put on hold for some reason that must be specified in a comment. labels Jul 20, 2018
@frangio frangio mentioned this pull request Aug 6, 2018
Closed
@shrugs shrugs mentioned this pull request Sep 4, 2018
Closed
@shrugs

shrugs commented Sep 4, 2018

Copy link
Copy Markdown
Contributor Author

closing in favor of tracking via #1272

@shrugs shrugs closed this Sep 4, 2018
@shrugs shrugs deleted the feat/erc721-mintable-and-bouncer-updates branch September 4, 2018 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@frangio frangio Awaiting requested review from frangio

Assignees

No one assigned

Labels

on hold Put on hold for some reason that must be specified in a comment.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

RBAC with owner

4 participants

@shrugs @vittominacori @frangio @nventuro