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 Standard #151

Open
dekz opened this Issue Mar 2, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@dekz

dekz commented Mar 2, 2018

Hey @enlight

These lessons are absolutely awesome!

I'd just like to talk about lesson number 5 or the one on NFT (aka ethereum/EIPs#721). The implementation in cryptozombies is influenced by CryptoKitties for obvious reasons, but it is not reflective of the 721 current draft going through the EIP process. I am more than happy to work with you or someone on your team to get lesson 5 to be compliant with the proposed 721 draft. IChanges include things like no takeOwnership and the addition of transferFrom and approvals to be more ERC20 like.

I see your Note that this is a proposal and you're using what is in OpenZeppelin currently. OpenZeppelin are also about to change that interface to be the new proposal.

@dekz dekz changed the title ERC721 Standard ERC721 Standard (proposal) Mar 2, 2018

@jamesmartinduffy

This comment has been minimized.

Contributor

jamesmartinduffy commented Mar 3, 2018

Hi @dekz, thanks for the kind words!

I looked at the EIP when writing the lesson, but it seemed like things were still being changed on a daily basis and it wasn't close enough to being a standard to base the lesson on. (E.g. A week ago the draft had tokens being referred to as "deeds", which I felt was a confusing word choice and would likely change by the final release anyway). Thus I chose to base the implementation on OpenZeppelin's contract.

I'm totally in favor of updating the lesson to be compliant with the standard, but would prefer to wait until it's an accepted standard rather than in draft form. That way we don't need to update the lesson content multiple times if it changes again before being finalized, if that makes sense.

Please feel free to keep us updated on the progress, and when it's far enough along we can update the lesson. Awesome work you guys are doing!

@dekz

This comment has been minimized.

dekz commented Mar 3, 2018

Sounds good to me, you're definitely correct, it was moving quite quickly! I'll ping you back when its moved forward with an OpenZeppelin example!

@dekz dekz changed the title ERC721 Standard (proposal) ERC721 Standard Mar 14, 2018

@dekz

This comment has been minimized.

dekz commented May 9, 2018

hey @jamesmartinduffy Just checking in on this again, any updates on changing the lesson to match the interface of the ERC721? I think having it right now as the very old draft is quite misleading to new developers.

OpenZeppelin have also merged their PR as can be seen here

@andreipope

This comment has been minimized.

Contributor

andreipope commented Oct 26, 2018

hey @dekz We are updating the lessons to match the ERC721 interface. We'll keep you updated on the progress. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment