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

ERC1155 docs #920

Merged
merged 64 commits into from Mar 5, 2024
Merged

ERC1155 docs #920

merged 64 commits into from Mar 5, 2024

Conversation

ericnordelo
Copy link
Member

This PR adds documentation for ERC1155, and fixes some inconsistencies in ERC721 docs.

cloudvenger and others added 29 commits December 11, 2023 03:17
Update error testing and remove tests failing from erc1155
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.

Improvements look good! I left a few minor suggestions and we just have to fix conflicts. Otherwise, this looks ready to me!

docs/modules/ROOT/pages/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/utils/_class_hashes.adoc Outdated Show resolved Hide resolved
Copy link
Contributor

@martriay martriay left a comment

Choose a reason for hiding this comment

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

very nice and complete docs! left some comments

docs/modules/ROOT/pages/presets.adoc Show resolved Hide resolved
* xref:#ERC721Component-owner_of[`++owner_of(self, token_id)++`]
* xref:#ERC721Component-safe_transfer_from[`++safe_transfer_from(self, from, to, token_id, data)++`]
* xref:#ERC721Component-transfer_from[`++transfer_from(self, from, to, token_id)++`]
* xref:#ERC721Component-approve[`++approve(self, to, token_id)++`]
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this adding a level of indirection?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's an indirection level but no added but this IMO. We had a direct link to the interface section, even when the interface doesn't have the full description like requirements, etc... We should add the full explanation of the method instead of a link to the interface as we are doing below. I think that's the scope of another PR, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Continuing this discussion on this thread

Copy link
Contributor

Choose a reason for hiding this comment

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

continued the convo in the other thread but isn't this going against what you said? this is adding a level of indirection and in the other thread you vouch for removing indirections

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO this should be pointing to ERC721Component, not directly to the interface, but the description shouldn't be pointing to the interface either, as it is currently doing. So the level of indirection added should be fixed by fixing the description, not by maintaining the previous link.

Copy link
Contributor

Choose a reason for hiding this comment

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

understood. let's advance with your proposal but let's make sure we give it a proper thought and document the decision later on

Copy link
Contributor

Choose a reason for hiding this comment

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

docs/modules/ROOT/pages/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/utils/_common.adoc Outdated Show resolved Hide resolved
src/token/erc1155/erc1155.cairo Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc1155.adoc Outdated Show resolved Hide resolved
Copy link
Contributor

@martriay martriay left a comment

Choose a reason for hiding this comment

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

great improvements!

docs/modules/ROOT/pages/api/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc1155.adoc Outdated Show resolved Hide resolved
ericnordelo and others added 6 commits March 1, 2024 16:05
Co-authored-by: Martín Triay <martriay@gmail.com>
Co-authored-by: Martín Triay <martriay@gmail.com>
Co-authored-by: Martín Triay <martriay@gmail.com>
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.

Left a tiny comment and a few suggestions with the recent changes. We're almost there!

docs/modules/ROOT/pages/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc1155.adoc Outdated Show resolved Hide resolved
src/token/erc1155/erc1155.cairo Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc1155.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc1155.adoc Outdated Show resolved Hide resolved
ericnordelo and others added 4 commits March 5, 2024 00:18
Co-authored-by: Martín Triay <martriay@gmail.com>
Co-authored-by: Martín Triay <martriay@gmail.com>
Co-authored-by: Martín Triay <martriay@gmail.com>
Co-authored-by: Martín Triay <martriay@gmail.com>
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.

LGTM!

@ericnordelo ericnordelo merged commit bf780e6 into OpenZeppelin:main Mar 5, 2024
5 checks passed
@ericnordelo ericnordelo deleted the docs/erc1155 branch March 5, 2024 16:02
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.

None yet

4 participants