-
Notifications
You must be signed in to change notification settings - Fork 358
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
Add ERC1155 mixin #941
Add ERC1155 mixin #941
Conversation
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.
Small suggestion, besides LGTM!
Co-authored-by: Eric Nordelo <[email protected]>
CHANGELOG.md
Outdated
@@ -11,7 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
### Added | |||
|
|||
- ERC1155 component and preset (#896) | |||
- Mixin implementations in components (#863) |
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.
We need to move this now to the Unreleased section.
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 created a separate entry specifically for ERC115 and ERC1155Receiver in Unreleased
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.
We should add a Mixin to the ERC1155ReceiverComponent too to be consistent with ERC721.
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!
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.
looking very good!
@@ -61,7 +62,7 @@ mod ERC1155ReceiverComponent { | |||
+HasComponent<TContractState>, | |||
+SRC5Component::HasComponent<TContractState>, | |||
+Drop<TContractState> | |||
> of IERC1155ReceiverCamel<ComponentState<TContractState>> { | |||
> of interface::IERC1155ReceiverCamel<ComponentState<TContractState>> { |
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.
any reason to make this explicit here?
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.
The mixin requires another import from erc1155::interface
. I figured it's better to just use the single interface
mod rather than bringing three interfaces into scope. No strong preference though
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.
but didn't the previous version imported only the ones it used? I lean towards dropping interface:: from these impl definitions to improve readability
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.
Done!
Fixes #933.
PR Checklist