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

ERC-721 token_uri() Hook #1184

Open
rsodre opened this issue Oct 17, 2024 · 3 comments
Open

ERC-721 token_uri() Hook #1184

rsodre opened this issue Oct 17, 2024 · 3 comments

Comments

@rsodre
Copy link

rsodre commented Oct 17, 2024

🧐 Motivation

We need more freedom to build token_uri.
The current implementation of token_uri() just concatenates the base_uri and token_id, which is limited to provide a URL.

Fully on-chain tokens need to provide the full json metadata, which needs to be rendered on the contract, from a hook.

📝 Details

Proposed solution:

  • Add token_uri(token_id: u256) hook to ERC721Component::ERC721HooksTrait
  • Use the custom uri provided by the hook implementation in ERC721Metadata::token_uri
  • I have this feature working here, can PR any time.
@ericnordelo
Copy link
Member

Hey @rsodre, thanks for taking the time for such a detailed request.

Hooks are designed to extend functionality by appending and/or pretending code to already existing logic. However, they are not intended as a mechanism for simulating the override of functions, which is more suitable for the use case of changing the token_uri logic.

Note that you can "override" the token_uri function to customize the logic, by implementing the IERC721Metadata interface directly in your contract, instead of embedding the component implementation. With this said, this is a thing we have in scope for improving, and in the future we may provide some custom traits associated to components that will allow customizing the logic without this much verbosity. This still won't be Hooks as a concept.

@rsodre
Copy link
Author

rsodre commented Oct 17, 2024

Not really... that would be the straight-forward solution, but can't do it currently because IERC721Metadata is embedded into IERC721Mixin.
I would have to clone the whole IERC721Mixin, which is not a great idea.
Or separate IERC721Metadata from IERC721Mixin on the OZ component.
Adding a hook was the most non-intrusive solution I found.

But anyway, I think this is an issue that needs to be addressed.
Dojo is deprecating the Origami erc components and asking developers to start using OZ components. Many will render on-chain metadata and require a way to customize token_uri(). I'm open to helping implement whatever solution the OZ team finds more suitable.

@ericnordelo
Copy link
Member

Dojo is deprecating the Origami erc components and asking developers to start using OZ components. Many will render on-chain metadata and require a way to customize token_uri(). I'm open to helping implement whatever solution the OZ team finds more suitable.

Noted, we will try to prioritize this accordingly, thanks for bringing it up.

I would have to clone the whole IERC721Mixin, which is not a great idea.

No need for cloning the while Mixin, you will need to embed the implementations separately, but that's about it, besides implementing just IERC721Metadata.

This:

    // ERC721
    #[abi(embed_v0)]
    impl ERC721Impl = ERC721Component::ERC721Impl<ContractState>;
    #[abi(embed_v0)]
    impl ERC721CamelOnly = ERC721Component::ERC721CamelOnlyImpl<ContractState>;
    impl ERC721InternalImpl = ERC721Component::InternalImpl<ContractState>;

    // SRC5
    #[abi(embed_v0)]
    impl SRC5Impl = SRC5Component::SRC5Impl<ContractState>;

instead of this:

    // ERC721Mixin
    #[abi(embed_v0)]
    impl ERC721MixinImpl = ERC721Component::ERC721Impl<ContractState>;
    impl ERC721InternalImpl = ERC721Component::InternalImpl<ContractState>;

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

No branches or pull requests

3 participants
@rsodre @ericnordelo and others