Skip to content
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

docs: Refactor pausable for erc721 example #414

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

0xNeshi
Copy link
Collaborator

@0xNeshi 0xNeshi commented Nov 20, 2024

Resolves #212

Copy link

netlify bot commented Nov 20, 2024

Deploy Preview for contracts-stylus canceled.

Name Link
🔨 Latest commit 0fa0a34
🔍 Latest deploy log https://app.netlify.com/sites/contracts-stylus/deploys/673edafc9f654200080f7e0c

Copy link
Collaborator

@bidzyyys bidzyyys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am against those changes as they are NOT consistent with our examples.
If we want to merge this PR, also all the rust docs should be updated where needed.

@qalisander @ggonzalez94 wdyt?

@qalisander
Copy link
Member

qalisander commented Nov 25, 2024

@0xNeshi Pausable doesn't work this way alas. In this case safe_transfer_from_with_data function will call nonoverriden _update function beneath. You can check how example is implemented currently for Pausable extension.
So we should keep when_not_paused check for every public function that calls _update.

Also It's been attempt before to implement Pausable with different approach. But it involves more code duplication..

@qalisander
Copy link
Member

qalisander commented Nov 25, 2024

I see what I missed. Technically you're calling when_not_paused every time we need it. I think we should refactor our pausable implementation (like here) before updating docs

@0xNeshi
Copy link
Collaborator Author

0xNeshi commented Nov 25, 2024

@qalisander @bidzyyys ahh, life without multi-inheritance is such a pain 😔

The more I look at this code, the more I like the original design. If a dev wants to make a pausable ERC721, they should inherit both erc721 and Pausable, and reimplement all the functions they want to make pausable to just call when_not_paused before calling the relevant erc721 function after it. Although it's a bit boring to have to do that, I think it's much worse to have to actually reimplement all the underlying erc721 function because it all depends on fn _update. It results in much more code duplication.
(Technically speaking, it would have been more correct if I had reimplemented all the erc721 functions I wanted to make pausable and public instead of going around them and calling _update directly. It looks a bit hacky imho)

Maybe we just close this PR and leave things as-is until multi-inheritance is properly supported, what do you guys think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor pausable for erc721
3 participants