OpenZeppelin / cairo-contracts

OpenZeppelin Contracts written in Cairo for Starknet, a decentralized ZK Rollup
https://docs.openzeppelin.com/contracts-cairo
MIT License
829 stars 344 forks source link

ERC-721 token_uri() Hook #1184

Open rsodre opened 1 month ago

rsodre commented 1 month ago

🧐 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:

ericnordelo commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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>;