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

ERC721 implementation shouldn't extend from Ownable #97

Closed
mg6maciej opened this issue Jun 22, 2018 · 4 comments
Closed

ERC721 implementation shouldn't extend from Ownable #97

mg6maciej opened this issue Jun 22, 2018 · 4 comments
Assignees

Comments

@mg6maciej
Copy link
Contributor

I have to say it IS convenient in my unit tests, that it does, but not all deployments will require it.

@MoMannn
Copy link
Collaborator

MoMannn commented Jun 22, 2018

You are correct. We will move the Ownable to the mocks.

@MoMannn
Copy link
Collaborator

MoMannn commented Jun 22, 2018

#98 @mg6maciej How does this look to you now? The implementation itself is without ownable but mocks have it.

@mg6maciej
Copy link
Contributor Author

@MoMannn LGTM. If possible I would go with a bit different inheritance to avoid duplicating mint and burn in 4 mock contracts. Something like NFTokenEnumerableMock is NFTokenEnumerable, NFTokenMock.

@MoMannn
Copy link
Collaborator

MoMannn commented Jun 22, 2018

@mg6maciej Ok, then I will close this issue. We will consider your suggestion about inheritance but that is a separate thing so I will open another issue for discussion about it.

@MoMannn MoMannn closed this as completed Jun 22, 2018
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

No branches or pull requests

2 participants