Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ERC721Enumerable helper function #1196
ERC721Enumerable helper function #1196
Changes from 7 commits
98b4ae2
f427dc5
b5b137b
b5d49c4
b71dd53
b4538ec
5e20c08
82f2bf0
4f5ead8
61de69c
31c0f02
dc9248d
87d0a3e
d93f8b0
70c26bd
97c7116
ef8ffd6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may work. It would also lead to some concern though:
IERC721ENUMERABLE_EXTENDED_ID
?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:
all_tokens_of_owner
function directly to the existingIERC721Enumerable
interface and its implIERC721ENUMERABLE_V2_ID
IERC721ENUMERABLE_V1_ID
ERC721EnumerableComponent
will implementall_tokens_of_owner
function and will register both interface IDs in its initializertrue
for the both interface IDsERC721EnumerableComponent
will returntrue
only for the original interface IDAs a result:
supports_interface
withIERC721ENUMERABLE_V1_ID
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the concerns:
We can reference functions of one in the other one so we don't need to duplicate the code.
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.
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.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Between the proposed approaches, I lean toward what's quoted above. To @immrsd's points though:
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:vs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't like the suggestion, again, I lean toward the superset impl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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