the tests are not running on scarb test.

The new tests or all tests? If the latter, are you in the package root (cairo-contracts) when running the test command?

@andrew-fleming , they were running but it was not showing any errors on my local machine. When I opened the pull request three of them were failing and the tests were running. I have committed the changes and all the tests are passing right now . Let me know if more tests are needed.

@immrsd , @andrew-fleming , I wanted to contribute more to the repository and am waiting for any suggestions to add to this PR to get merged, to take up a new issue. Can you please review again?

Hey @gerceboss, sorry for being this late to the conversation. I reviewed the PR and the implementation, tests, and docs look great. With this said, I think that for this particular case we don't need an extra component, because we can add just an extra embeddable implementation (ERC721MetadataURIStorage) to the existing ERC721 component (besides the storage member, the event, and the internal set_token_uri function). This would make the usage of it less verbose, since users would only need to change an embedded implementation on the final contract, instead of adding the extra component verbosity on it. What do you think @andrew-fleming?

@ericnordelo I lean toward keeping the component light and not introduce non-core impls directly. The additions are small, but it forces all contracts to add extra storage for a feature that many contracts likely won't use.

On the contrary, if it seems worth it to include this in the component itself, why don't we make the URIStorage token_uri the default impl? token_uri is really for off-chain queries; therefore, gas shouldn't be a concern. If there's no storage URI for a given token id, it'll behave like the current impl with base_uri + id

I lean toward keeping the component light and not introduce non-core impls directly. The additions are small, but it forces all contracts to add extra storage for a feature that many contracts likely won't use.

Note that the extra storage is completely transparent to the final contract, and it doesn't require any overhead gas-wise. We also have the two-step ownable setup as an extra embeddable implementation to avoid adding an extra component, so this would be consistent, and even smaller and easier to use than that.

On the contrary, if it seems worth it to include this in the component itself, why don't we make the URIStorage token_uri the default impl? token_uri is really for off-chain queries; therefore, gas shouldn't be a concern. If there's no storage URI for a given token id, it'll behave like the current impl with base_uri + id

We can make it the default implementation, but I'm leaning toward keeping consistency with our Solidity counterpart on this, for the sake of consistency but also because even when it may be negligible gas-wise, developers often want the option to avoid an extra check on storage, that could be useful for a use case that we currently don't see.

@ericnordelo yeah, those are fair points. A separate impl in the core component sounds good to me. One last thing to note is that the Solidity implementation also supports erc4906. If we include support for that or a SN equivalent, I think we'll need to have a separate initializer for URI storage to declare support. We can address that later though

@ericnordelo @andrew-fleming ,so , how do we need to change the implementation as your suggestion is to not have it as an extension? And about erc4906, I am planning to make a PR but am waiting to complete this one first.

how do we need to change the implementation as your suggestion is to not have it as an extension?

We can move the embeddable URI storage impl from the extension into the erc721 component itself. Be aware, we're now using SN Foundry for testing so the tests will need to be refactored accordingly and we're also in the process of migrating to multiple packages (#1065)

Will check that tomorrow 👍

@andrew-fleming @ericnordelo , wanted to confirm the steps for the change:

1. Add an internal implementation in the packages/token/src/erc721/erc721.cairo for the _set_token_uri
2. Add a storage member for the URIs in the same file itself

the second option is to add a new interface like ERC721Metadata interface, can you confirm what to do?

1. Add an internal implementation in the packages/token/src/erc721/erc721.cairo for the _set_token_uri

Do we need another implementation just for _set_token_uri? Can't we just add the function to ERC721Component::InternalImpl?

2. Add a storage member for the URIs in the same file itself

In the same Storage struct, yes

the second option is to add a new interface like ERC721Metadata interface, can you confirm what to do?

Not sure what you mean by second option. It's the same interface so we don't need to define a new interface. The idea is to have an additional implementation of the same IERC721Metadata interface within the ERC721Component module like this:

#[starknet::component]
pub mod ERC721Component {
    ...

    #[embeddable_as(ERC721MetadataImpl)]
    impl ERC721Metadata<...> of interface::IERC721Metadata<...> {
        ...
    }

    #[embeddable_as(ERC721URIStorageImpl)]
    impl ERC721URIStorage<...> of interface::IERC721Metadata<...> {
        ...
    }

    ...
}

@andrew-fleming, can you also mention how do I change the testing ?

can you also mention how do I change the testing ?

I think the only thing to change in these tests are the event assertions. Instead of using utils::pop_log, we should be using snforge_std's spy_events in conjunction with our EventSpyExt

Here's an implementation for reference: https://github.com/OpenZeppelin/cairo-contracts/blob/main/packages/security/src/tests/test_pausable.cairo#L125-L154