-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat: erc721 consecutive extension #202
Conversation
# Conflicts: # contracts/src/lib.rs # contracts/src/utils/math/alloy.rs # contracts/src/utils/math/mod.rs # contracts/src/utils/mod.rs
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.
Looks good in a second pass. I agree with comments left by @alexfertel that are still to be resolved.
Co-authored-by: alexfertel <[email protected]>
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.
LGTM! Just one comment regarding test function names.
/// Erc721 contract storage. | ||
Erc721 erc721; | ||
/// Checkpoint library contract for sequential ownership. | ||
Trace160 _sequential_ownership; |
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.
IMHO there is some inconsistency in naming, erc721
and than prefixed with _
, like _sequential_ownership
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.
Great job @qalisander!
I left some minor comments and nits.
This extension matches the solidity implementation except one thing:
_owner_of_inner
and_update
some functions are duplicated from erc721 token standardResolves #68
PR Checklist