ERC721 URIstorage extension implementation

[gerceboss]

Hey , I recently needed to implement dynamic nfts in Cairo but oppenzepelin doesn't provide it as ERC721's extension they do in solidity.
I have implemented dynamic nft myself in the project .

Would like to work on implementing
ERC721URIstorage extension.

[andrew-fleming]

Hey, @gerceboss! Thanks for opening this issue. I think this would be a great addition to the library! We'd be happy to review and support a PR :)

[gerceboss]

Hey @andrew-fleming , can you guide a bit on how the structure must be? How and in which files should I structure the component ?

[andrew-fleming]

can you guide a bit on how the structure must be?

Sure!

How and in which files should I structure the component ?

Regarding which files, I'd say the structure should be something like this:

token
  └── erc721
        ├── extensions.cairo
        └── extensions
                └── erc721_uri_storage.cairo (component in here)

Regarding how the component itself should be structured, you can take a look at how the erc20_votes extension is defined as a template. I imagine the component should define a URIStorage implementation of IERC721Metadata with the new token_uri behavior as well as an InternalImpl with set_token_uri

[gerceboss]

Okay,great !! Will do that

[gerceboss]

@andrew-fleming, can you please confirm the tests that will be needed for this extension? And also suggest other tests than these?

1. test_initialiser()
2. test_set_token_uri()
3. test_token_uri()

[andrew-fleming]

Hey, @gerceboss. Yes to those tests. Assuming the extension behaves like the OZ Solidity implementation, you can use the Solidity tests as a guide: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/test/token/ERC721/extensions/ERC721URIStorage.test.js

[gerceboss]

@andrew-fleming , while testing, there is a problem as the branch I pushed the changes on my forked repository is different, it does not resolves and throws this error:

error: Identifier not found.
use openzeppelin::tests::mocks::erc721_uri_storage_mocks::ERC721URIstorageMock;
                                ^**********************^

error: Identifier not found.
use openzeppelin::token::erc721::extensions::ERC721URIstorageComponent;
                                 ^********^

Can you help on how I run the tests?

[andrew-fleming]

It's hard to identify without seeing the code. Did you make them visible by adding:

• pub(crate) mod erc721_uri_storage_mocks; to tests/mocks.cairo?
• pub mod extensions; to token/erc721.cairo?

If this isn't it, kindly link your branch and I'll take a look

[gerceboss]

Hey @andrew-fleming,

    impl ERC721URIstorage<
        TContractState,
        +HasComponent<TContractState>,
        +SRC5Component::HasComponent<TContractState>,
        +ERC721Component::HasComponent<TContractState>,
        +Drop<TContractState>
    > of IERC721URIstorage<ComponentState<TContractState>> {

I currently implement the functionality like this and the IERC721URIstorage implements following:

    fn name(self: @TState) -> ByteArray;
    fn symbol(self: @TState) -> ByteArray;
    fn token_uri(self: @TState,token_id:u256)->ByteArray;(changed implementation)
    fn set_token_uri(ref self: TState,token_id:u256,_token_uri:ByteArray);

I am stuck on how I implement the name and the symbol functionalities.

[andrew-fleming]

Hey! You can use get_dep_component! to read from dependencies:

    impl ERC721URIStorage<
        ...
        impl ERC721: ERC721Component::HasComponent<TContractState>
    > of IERC721Metadata<ComponentState<TContractState>> {
        fn name(self: @ComponentState<TContractState>) -> ByteArray {
            let erc721 = get_dep_component!(self, ERC721);
            erc721.ERC721_name.read()
        }
        ...
    }

Also, I'm not sure we need a new interface for URI storage. We're just adding an implementation of IERC721Metadata

[andrew-fleming]

Should we? If a project wants this to be an external fn, they can create an external fn in the contract itself and implement whatever permissions with it. That's a very specific case and as a library, I don't think that's for us to do. The URI storage extension should just support storing and reading a unique URI per token. Generally, URIs are set when they're minted

[gerceboss]

@andrew-fleming ,I have implemented the tests and included the mock contract and the test file in the needed files. It compiles but when I run scarb test the tests are still not running, I am unable to find the reason for that. I am opening a PR , can you help me do that there?