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

Use the latest ERC-721 draft interface, fixes #745 #777

Closed
wants to merge 3 commits into from

Conversation

fulldecent
Copy link
Contributor

@fulldecent fulldecent commented Feb 27, 2018

Fixes #745

🚀 Description

The ERC-721 is near finalization. I have included the latest interfaces into here.

We are now working on interop testing and would love to work with the OpenZeppelin project on this.

  • 📘 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. HELL YES
  • 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

@fulldecent
Copy link
Contributor Author

Quoting @spalladino from https://gitter.im/ETH-NFT/Lobby to move the discussion here.

For minor discussion on #777, would you rather keep the discussion here, or on the PR itself? For instance, I'd consider changing visibility of external methods to public (as in ERC20), or remove seemingly unnecessary payable modifiers.

@fulldecent
Copy link
Contributor Author

I have made these requested changes.

Just wanted to note here that it is okay that OpenZeppelin's interface for ERC-721 is different than the canonical ERC-721 (external -> public, no payable). And it does not mean that the standard is deficient. The standard recognizes and makes allowance for this already.

Specifically, the standard is correct for the long term, future versions of Solidity. And this file in OZ is correct today.

Citing https://github.com/fulldecent/EIPs/blob/patch-2/EIPS/eip-721.md#caveats:

I'm pointing this out to assert that the standard does regulate on the payability and non-payability of functions. And I continue to fight to make the Solidity interface grammar more expressive!

@spalladino
Copy link
Contributor

Thanks @fulldecent! I'll compare this interface with the candidate implementation from @facuspagnuolo at #682, rework the implementation as needed, and revert back here if there is any strong mismatch.

@spalladino
Copy link
Contributor

Closing in favour of #803. Note that this interface was included in that PR. Again, thanks for your work @fulldecent!

@spalladino spalladino closed this Mar 8, 2018
@fulldecent fulldecent deleted the patch-1 branch March 9, 2018 00:35
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.

Use #841 reference implementation in OpenZeppelin
2 participants