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

Document ERC721ReceiverComponent #945

Merged

Conversation

ericnordelo
Copy link
Member

Fixes #913

This PR adds the missing documentation for the ERC721ReceiverComponent, and also
fixes the name of the mixin implementation.

PR Checklist

  • Tests
  • Documentation
  • Added entry to CHANGELOG.md
  • Tried the feature on a public network

Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, sir! I left a few suggestions—the biggest one being that we combine the snake and camel embeddable blocks into a single embeddable block. I think we can update Creating a token receiver contract to use ERC721ReceiverMixinImpl as well to clean up the snippet. WDYT?

docs/modules/ROOT/pages/api/erc721.adoc Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc721.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc721.adoc Outdated Show resolved Hide resolved
src/token/erc721/erc721_receiver.cairo Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc721.adoc Outdated Show resolved Hide resolved
@ericnordelo
Copy link
Member Author

ericnordelo commented Mar 19, 2024

I think we can update Creating a token receiver contract to use ERC721ReceiverMixinImpl as well to clean up the snippet. WDYT?

The only issue I see with the approach is inconsistency. For example, the ERC721 in the same page is not using mixins, and I think we should update that as well. Let's let it for a different issue if you agree.

#948

Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a couple more small suggestions and they need to be applied in #941 as well. This looks good to me otherwise

docs/modules/ROOT/pages/api/erc721.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc721.adoc Outdated Show resolved Hide resolved
@ericnordelo ericnordelo merged commit 9f14829 into OpenZeppelin:main Mar 22, 2024
5 checks passed
@ericnordelo ericnordelo deleted the feat/document-erc721receiver-#913 branch March 22, 2024 11:38
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.

Add docs for ERC721Receiver component
3 participants