ERC721Enumerable helper function #1196

immrsd · 2024-10-29T14:57:00Z

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

codecov · 2024-10-29T15:22:33Z

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.56%. Comparing base (8f4e8a1) to head (ef8ffd6).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...tensions/erc721_enumerable/erc721_enumerable.cairo	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1196      +/-   ##
==========================================
- Coverage   91.61%   91.56%   -0.05%     
==========================================
  Files          47       47              
  Lines        1228     1233       +5     
==========================================
+ Hits         1125     1129       +4     
- Misses        103      104       +1

Files with missing lines	Coverage Δ
...tensions/erc721_enumerable/erc721_enumerable.cairo	`95.91% <80.00%> (-1.81%)`	⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f4e8a1...ef8ffd6. Read the comment docs.

ericnordelo

Looking good @immrsd, letf some comments.

docs/modules/ROOT/pages/api/erc721.adoc

packages/test_common/src/mocks/erc721_enumerable.cairo

ericnordelo · 2024-10-29T15:56:05Z

packages/token/src/erc721/extensions/erc721_enumerable/erc721_enumerable.cairo

@@ -84,6 +83,36 @@ pub mod ERC721EnumerableComponent {
        }
    }

+    #[embeddable_as(ERC721EnumerableExtendedImpl)]
+    impl ERC721EnumerableExtended<


This implementation won't be used much without the not extended version functions. What do you think if we add the not extended functions to this as well, and so users would need to embed just the extended implementation instead of both? Similar to what we have in Ownable with the two-step implementation. If we do this we need to update the doc-site as well.

It may work. It would also lead to some concern though:

We'll have 2 almost similar interfaces and the same situation with their implementations

It won't be clear which one to use and why did we introduce 2 slightly different versions in the first place

Should we add an interface ID for IERC721ENUMERABLE_EXTENDED_ID?

If we do, what interface ID should be registered in the initializer of the component: the standard or the extended one?
To summarize, I believe it would complicate things for our users and there's no strong reason to do so.

At the same time, I agree with you and the current implementation doesn't look perfect to me: users have to know about the extended trait version and they have to add 2 enumerable impls to get the full functionality.

I've got an idea how to improve things:

We add the new all_tokens_of_owner function directly to the existing IERC721Enumerable interface and its impl

We resolve the new SRC5 interface ID and name it IERC721ENUMERABLE_V2_ID

We keep the current interface ID by naming it IERC721ENUMERABLE_V1_ID

ERC721EnumerableComponent will implement all_tokens_of_owner function and will register both interface IDs in its initializer

New contracts utilizing the component will provide the extended functionality and return true for the both interface IDs

Old contracts with ERC721EnumerableComponent will return true only for the original interface ID

As a result:

It will have no effect on the already deployed contracts

If a caller is interested in the general Enumerable functionality, he will call supports_interface with IERC721ENUMERABLE_V1_ID

If he's looking for the extended functionality, he will check for IERC721ENUMERABLE_V2_ID

What do you think? I know it may sound complex, but it's actually very concise approach that will lead to having a single interface, a single implementation and the extended functionality built-in by default.

Regarding the concerns:

We'll have 2 almost similar interfaces and the same situation with their implementations

We can reference functions of one in the other one so we don't need to duplicate the code.

It won't be clear which one to use and why did we introduce 2 slightly different versions in the first place

I think the Extended suffix leaves implicit that the implementation is extending the other one, and with correct comments and docs it should be clear why.

Should we add an interface ID for IERC721ENUMERABLE_EXTENDED_ID?

I don't think we have to, since we add interfaces for standards to make them easier to track, but in this case we are providing a helper on top of the standard, not a different one.

If we do, what interface ID should be registered in the initializer of the component: the standard or the extended one?

We can register the same interface we are registering now, since the Extended implementation will provide every method defined in the interface, so it is implemented. It doesn't matter that the contract has an extra external method for this matter.

Regarding the suggestion, the only part I don't like much is adding a new interface ID, since as mentioned before I think is enough with the current one even if we are adding a new external function. If we do this we would have an interface whose ID won't match the SRC5 standard. For that, I'm still leaning toward the first approach:

IERC721Enumerable and IERC721EnumerableExtended embeddable implementations with the second one being a superset of the first one (as suggested by the name).

Registering only the IERC721Enumerable interface ID even when the extended version is used, since that implies that the IERC721Enumerable is implemented as it is contained.

This will make things easier for users as they will only need to embed one implementation, and with appropriate comments and docs it should be easy to get the difference.

IERC721Enumerable and IERC721EnumerableExtended embeddable implementations with the second one being a superset of the first one (as suggested by the name).

Registering only the IERC721Enumerable interface ID even when the extended version is used, since that implies that the IERC721Enumerable is implemented as it is contained.

This will make things easier for users as they will only need to embed one implementation, and with appropriate comments and docs it should be easy to get the difference.

Between the proposed approaches, I lean toward what's quoted above. To @immrsd's points though:

We'll have 2 almost similar interfaces and the same situation with their implementations

It won't be clear which one to use and why did we introduce 2 slightly different versions in the first place

it seems a bit clunky. We're just adding a non-standardized helper view fn. What do you guys think about making this even simpler (in some respects) and just have all_tokens_of_owner as an internal fn that the using contract may manually expose? Here's the difference between implementions:

#[abi(embed_v0)] impl ERC721EnumerableExtendedImpl = ERC721EnumerableComponent::ERC721EnumerableExtendedImpl<ContractState>;

vs

#[abi(embed_v0)] fn all_tokens_of_owner(self: @ContractState, owner: ContractAddress) -> Span<u256> { self.erc721_enumerable.all_tokens_of_owner(owner) }

If we don't like the suggestion, again, I lean toward the superset impl

I personally like what @andrew-fleming is suggesting the most, but I don't have a strong opinion against adding the extra implementation, as long as we don't start abusing the pattern.

I also like what @andrew-fleming has suggested, this way we don't have to introduce a new interface and interface ID. I refactored the code accordingly

andrew-fleming

Looking good so far! I think you guys raised a lot of good points in the big thread. I left my two cents :)

docs/modules/ROOT/pages/api/erc721.adoc

andrew-fleming · 2024-11-05T00:48:04Z

packages/token/src/erc721/extensions/erc721_enumerable/erc721_enumerable.cairo

@@ -84,6 +83,36 @@ pub mod ERC721EnumerableComponent {
        }
    }

+    #[embeddable_as(ERC721EnumerableExtendedImpl)]
+    impl ERC721EnumerableExtended<


IERC721Enumerable and IERC721EnumerableExtended embeddable implementations with the second one being a superset of the first one (as suggested by the name).

Registering only the IERC721Enumerable interface ID even when the extended version is used, since that implies that the IERC721Enumerable is implemented as it is contained.

This will make things easier for users as they will only need to embed one implementation, and with appropriate comments and docs it should be easy to get the difference.

Between the proposed approaches, I lean toward what's quoted above. To @immrsd's points though:

We'll have 2 almost similar interfaces and the same situation with their implementations

It won't be clear which one to use and why did we introduce 2 slightly different versions in the first place

it seems a bit clunky. We're just adding a non-standardized helper view fn. What do you guys think about making this even simpler (in some respects) and just have all_tokens_of_owner as an internal fn that the using contract may manually expose? Here's the difference between implementions:

#[abi(embed_v0)] impl ERC721EnumerableExtendedImpl = ERC721EnumerableComponent::ERC721EnumerableExtendedImpl<ContractState>;

vs

#[abi(embed_v0)] fn all_tokens_of_owner(self: @ContractState, owner: ContractAddress) -> Span<u256> { self.erc721_enumerable.all_tokens_of_owner(owner) }

andrew-fleming · 2024-11-05T01:01:52Z

packages/token/src/erc721/extensions/erc721_enumerable/erc721_enumerable.cairo

@@ -84,6 +83,36 @@ pub mod ERC721EnumerableComponent {
        }
    }

+    #[embeddable_as(ERC721EnumerableExtendedImpl)]
+    impl ERC721EnumerableExtended<


If we don't like the suggestion, again, I lean toward the superset impl

…er an internal function

ericnordelo

LGTM!

andrew-fleming

LGTM! 🤞 for coverage not to weirdly fail

immrsd added 4 commits September 29, 2024 16:25

Implement helper function for ERC721Enumerable

98b4ae2

Merge main

f427dc5

Merge main

b5b137b

Change function name

b5d49c4

immrsd requested review from ggonzalez94, ericnordelo and andrew-fleming October 29, 2024 14:57

immrsd added 2 commits October 29, 2024 16:11

Run linter

b71dd53

Add changelog entry

b4538ec

Add documentation

5e20c08

immrsd marked this pull request as ready for review October 29, 2024 15:34

ericnordelo reviewed Oct 29, 2024

View reviewed changes

immrsd self-assigned this Oct 30, 2024

immrsd added 3 commits October 30, 2024 09:32

Remove unnecessary Enumerable mocks

82f2bf0

Add ERC721EnumerableExtended impl to ERC721Enumerable mock

4f5ead8

Merge main

61de69c

andrew-fleming reviewed Nov 5, 2024

View reviewed changes

immrsd added 5 commits November 6, 2024 14:44

Remove IERC721EnumerableExtended interface and make all_tokens_of_own…

31c0f02

…er an internal function

Update doc

dc9248d

Update changelog

87d0a3e

Add more test cases

d93f8b0

Merge main

70c26bd

immrsd requested review from andrew-fleming and ericnordelo November 6, 2024 15:03

Try to fix coverage issue

97c7116

ericnordelo approved these changes Nov 6, 2024

View reviewed changes

Revert test coverage fix changes

ef8ffd6

andrew-fleming approved these changes Nov 6, 2024

View reviewed changes

immrsd merged commit 02c9ce6 into OpenZeppelin:main Nov 6, 2024
7 of 8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ERC721Enumerable helper function #1196

ERC721Enumerable helper function #1196

immrsd commented Oct 29, 2024 •

edited

Loading

codecov bot commented Oct 29, 2024 •

edited

Loading

ericnordelo left a comment

ericnordelo Oct 29, 2024

immrsd Oct 30, 2024

ericnordelo Oct 30, 2024

andrew-fleming Nov 5, 2024

andrew-fleming Nov 5, 2024

ericnordelo Nov 5, 2024

immrsd Nov 6, 2024

andrew-fleming left a comment

andrew-fleming Nov 5, 2024

andrew-fleming Nov 5, 2024

ericnordelo left a comment

andrew-fleming left a comment

ERC721Enumerable helper function #1196

ERC721Enumerable helper function #1196

Conversation

immrsd commented Oct 29, 2024 • edited Loading

codecov bot commented Oct 29, 2024 • edited Loading

Codecov Report

ericnordelo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-fleming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericnordelo left a comment

Choose a reason for hiding this comment

andrew-fleming left a comment

Choose a reason for hiding this comment

immrsd commented Oct 29, 2024 •

edited

Loading

codecov bot commented Oct 29, 2024 •

edited

Loading