Add ERC721EnumerableComponent extension #983
Conversation
…eat/add-hooks-#965
There was a problem hiding this comment.
The implementation looks good, I left a couple of broad suggestions, I will give a full review after the PR is finished. I vote we use StorageArray for releasing, but we can finish the PR either way since the changes should be small, and we can commit to one impl at the end.
CHANGELOG.md
Outdated
| @@ -8,6 +8,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| ### Added | |||
|
|
|||
| - before_update and after_update hooks to ERC721Component (#978) | |||
There was a problem hiding this comment.
Lets not forget to add the entry to the changelog.
| #[starknet::component] | ||
| mod ERC721EnumerableComponent { | ||
| use openzeppelin::introspection::src5::SRC5Component::InternalTrait as SRC5InternalTrait; | ||
| use openzeppelin::introspection::src5::SRC5Component::SRC5; |
| } | ||
|
|
||
| /// | ||
| fn _update(ref self: ComponentState<TContractState>, to: ContractAddress, token_id: u256,) { |
There was a problem hiding this comment.
Should we provide this as a hooks implementation (after_update)? Something like a function maybe is enough, and then users can call each extension after_update in the preset Hooks implementation. We need to document it, but I think it will be easy enough to use.
There was a problem hiding this comment.
That's basically the idea behind the design. I went back and forth on update vs before_update. I do like the idea of having a pattern of extension functions (required in hooks) named before/after_update. Will update
There was a problem hiding this comment.
Hey Andrew, the PR looks great but I have two "big" suggestions to consider before getting it merged:
-
I feel it is worth waiting until at least the next milestone to release it since we are going to have two good additions to the language: Map and StorageArray, that can help both making the implementation cleaner and avoid breaking right after we release a first version. I don't think we should target being backward compatible for this module, which brings us into suggestion two.
-
Break backward compatibility to use Map and StorageArray, and also remove the camelCase interface. I think at this point we are not targeting contracts that may upgrade from cairo0 to this new version, since they either upgraded already or are not gonna do it.
Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com>
Mmmmm yeah, I think that's fair. I'll change this to a draft and we can revisit in either the
Agreed. Will update! |
Fixes #722.
Deps:
PR Checklist