-
Notifications
You must be signed in to change notification settings - Fork 359
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 ERC721 preset #811
Add ERC721 preset #811
Conversation
Co-authored-by: Eric Nordelo <[email protected]>
src/presets/erc721.cairo
Outdated
component!(path: SRC5Component, storage: src5, event: SRC5Event); | ||
component!(path: ERC721Component, storage: erc721, event: ERC721Event); |
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 think we should swap the components to make ERC721 appear first (this applies to impls below too). That is consistent with the Account Preset 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.
I agree in the sense that it's the "main" component, but I switched it because Access
appears first in the imports and IMO it appears more organized that way. What do you think?
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.
mmm, got your point, but I still think it makes more sense to have the main component first, instead of letting the imports decide. We still need to define the rule for this, but I don't like the one coming from import orders that much 😢
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.
That's fair, and we also already have the account preset using the suggested approach. I'll update and we can maybe revisit in the future
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 actually like SRC5
first, as it's a dependency of ERC721
. also, not sure if there's always going to be a "main component", and it's not clear to me that's a concept we want to suggest or push.
+1 to not inheriting order from imports
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.
also, not sure if there's always going to be a "main component", and it's not clear to me that's a concept we want to suggest or push.
+1 to not inheriting order from imports
Let's say we create an ownable, pausable ERC20 preset. There's no component dependency like with SRC5. If we're not using the main component approach and we're not inheriting order from imports, how else can we organize the component declarations?
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.
maybe we don't need a rule for that
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.
Besides perhaps the XYZ discussion for changes pending to be released, I think this is good to go!
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.
Nice work! Looks good to me.
Opened an issue to track adding it to the docsite.
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. Thanks!
Fixes (partial) #804 and #693.
PR Checklist