ERC721 URI Storage #1031

gerceboss · 2024-07-01T20:27:20Z

Resolves #1019

Added extension's component implementation, mock contract, tests and updated documentation
Purpose: Adding ERC721URIstorage extension to OZ.

PR Checklist

Tests
Documentation
Added entry to CHANGELOG.md
Tried the feature on a public network

Transaction hash on Starknet sepolia: 0x044e53a70cb6cd021d2e2344a0b1921b833874cc25c676f997f8aa819302e664
Test contract deployed: 0x00f199172801f6685601944d749557d8efa121bb6f439026a9e37799c6b1fbae

andrew-fleming · 2024-07-01T21:22:44Z

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?

gerceboss · 2024-07-01T21:26:23Z

@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 · 2024-07-02T09:23:18Z

src/token/erc721/extensions/erc721_uri_storage.cairo

+// OpenZeppelin Contracts for Cairo v0.14.0 (token/erc721/extensions/erc721_uri_storage.cairo)
+
+#[starknet::component]
+pub mod ERC721URIstorageComponent {


Suggested change

pub mod ERC721URIstorageComponent {

pub mod ERC721URIStorageComponent {

Do I need to change that everywhere? ERC721URIstorage -> ERC721URIStorage.
Because I have used the earlier one in naming of files,Impl and in tests as well?

immrsd · 2024-07-02T09:24:57Z

src/token/erc721/extensions/erc721_uri_storage.cairo

+
+    #[storage]
+    struct Storage {
+        token_uris: LegacyMap<u256, ByteArray>,


Suggested change

token_uris: LegacyMap<u256, ByteArray>,

ERC721URIStorage_token_uris: LegacyMap<u256, ByteArray>,

Storage variables should be prefixed with component name to prevent collisions with ones of other contracts or components. Here's an example from ERC721 component

immrsd · 2024-07-02T09:25:39Z

src/token/erc721/extensions/erc721_uri_storage.cairo

+    #[event]
+    #[derive(Drop, PartialEq, starknet::Event)]
+    pub enum Event {
+        MetadataUpdate: MetadataUpdate,


Suggested change

MetadataUpdate: MetadataUpdate,

MetadataUpdated: MetadataUpdated,

For consistency among the codebase we strive to have all (almost) events named in the same way. Following this approach the name of this event should be MetadataUpdated, although it is indeed name MetadataUpdate in the Solidity counterpart

immrsd · 2024-07-02T09:26:55Z

src/token/erc721/extensions/erc721_uri_storage.cairo

+        fn symbol(self: @ComponentState<TContractState>) -> ByteArray {
+            self._symbol()
+        }
+        /// Returns the Uniform Resource Identifier (URI) for the `token_id` token.


Suggested change

/// Returns the Uniform Resource Identifier (URI) for the `token_id` token.

/// Returns the Uniform Resource Identifier (URI) for the `token_id` token.

immrsd · 2024-07-02T09:27:39Z

src/token/erc721/extensions/erc721_uri_storage.cairo

+        fn _name(self: @ComponentState<TContractState>) -> ByteArray {
+            let erc721_component = get_dep_component!(self, ERC721);
+            let name = erc721_component.ERC721_name.read();
+            return name;


Suggested change

return name;

name

immrsd · 2024-07-02T09:27:50Z

src/token/erc721/extensions/erc721_uri_storage.cairo

+        fn _symbol(self: @ComponentState<TContractState>) -> ByteArray {
+            let erc721_component = get_dep_component!(self, ERC721);
+            let symbol = erc721_component.ERC721_symbol.read();
+            return symbol;


Suggested change

return symbol;

symbol

immrsd · 2024-07-02T09:30:07Z

src/token/erc721/extensions/erc721_uri_storage.cairo

+                return "";
+            } else {
+                return format!("{}{}", base_uri, token_id);
+            }


Double if-else statement

Consider avoiding early returns if possible

return format!("{}{}", base_uri, token_id);

@immrsd, should I replace the second if-else with this?

gerceboss · 2024-07-09T20:22:23Z

@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?

andrew-fleming

Hey, thank you for opening this PR!

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?

We appreciate your eagerness to contribute! We review contributions as soon as we can, but please understand that reviews take time. That said, I finished my initial review. I left a lot of feedback (sorry!). In addition to addressing the review, could you kindly finish the PR checklist:

add this entry to CHANGELOG
deploy/interact with this feature on sepolia and provide an address and/or tx hash to document that the feature behaves as expected on a public network?

src/token/erc721/extensions/erc721_uri_storage.cairo

docs/modules/ROOT/pages/api/erc721.adoc

andrew-fleming

Looking much better! I left some feedback. Would you mind also updating your branch and fixing conflicts?

CHANGELOG.md

src/tests/token/erc721/test_erc721_uri_storage.cairo

src/token/erc721/extensions/erc721_uri_storage.cairo

docs/modules/ROOT/pages/api/erc721.adoc

src/tests/token/erc721/test_erc721_uri_storage.cairo

src/token/erc721/extensions/erc721_uri_storage.cairo

docs/modules/ROOT/pages/api/erc721.adoc

andrew-fleming

Looking good! Left a few comments mostly from the cairo bump

CHANGELOG.md

src/token/erc721/extensions/erc721_uri_storage.cairo

docs/modules/ROOT/pages/api/erc721.adoc

src/token/erc721/extensions/erc721_uri_storage.cairo

andrew-fleming

Great work on the changes! LGTM! Let's wait on a second pair of eyes before merging

ericnordelo · 2024-07-31T09:41:33Z

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?

andrew-fleming · 2024-07-31T18:59:38Z

@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

ericnordelo · 2024-08-01T12:37:27Z

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.

andrew-fleming · 2024-08-01T16:12:13Z

@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

gerceboss · 2024-08-01T16:17:03Z

@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.

andrew-fleming · 2024-08-01T20:38:52Z

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)

gerceboss · 2024-08-01T20:57:15Z

Will check that tomorrow 👍

immrsd · 2024-08-04T08:48:36Z

src/token/erc721/extensions/erc721_uri_storage.cairo

+        ///
+        /// - `token_id` exists.
+        fn token_uri(self: @ComponentState<TContractState>, token_id: u256) -> ByteArray {
+            let mut erc721_component = get_dep_component!(self, ERC721);


Does it have to be mutable?

immrsd · 2024-08-04T08:52:01Z

src/token/erc721/extensions/erc721_uri_storage.cairo

+            // If there is no base_uri, return the token_uri
+            if base_uri.len() == 0 {
+                return token_uri;
+            }
+
+            // If both are set, concatenate the base_uri and token_uri
+            if token_uri.len() > 0 {
+                return format!("{}{}", base_uri, token_uri);
+            }
+
+            // Implementation from ERC721Metadata::token_uri
+            return format!("{}{}", base_uri, token_id);


Suggested change

// If there is no base_uri, return the token_uri

if base_uri.len() == 0 {

return token_uri;

}

// If both are set, concatenate the base_uri and token_uri

if token_uri.len() > 0 {

return format!("{}{}", base_uri, token_uri);

}

// Implementation from ERC721Metadata::token_uri

return format!("{}{}", base_uri, token_id);

if base_uri.len() == 0 {

// If there is no base_uri, return the token_uri

token_uri

} else if token_uri.len() == 0 {

// Implementation from ERC721Metadata::token_uri

format!("{}{}", base_uri, token_id)

} else {

// If both are set, concatenate the base_uri and token_uri

format!("{}{}", base_uri, token_uri)

}

immrsd · 2024-08-04T08:52:39Z

src/token/erc721/extensions/erc721_uri_storage.cairo

+            ref self: ComponentState<TContractState>, token_id: u256, token_uri: ByteArray
+        ) {
+            self.ERC721URIStorage_token_uris.write(token_id, token_uri);
+            self.emit(MetadataUpdated { token_id: token_id });


Suggested change

self.emit(MetadataUpdated { token_id: token_id });

self.emit(MetadataUpdated { token_id });

gerceboss · 2024-08-05T11:24:26Z

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

Add an internal implementation in the packages/token/src/erc721/erc721.cairo for the _set_token_uri
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?

andrew-fleming · 2024-08-05T22:07:02Z

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?

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<...> {
        ...
    }

    ...
}

gerceboss · 2024-08-11T12:38:29Z

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

andrew-fleming · 2024-08-15T01:13:47Z

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

immrsd requested review from andrew-fleming, ericnordelo and immrsd and removed request for andrew-fleming July 2, 2024 09:06

immrsd reviewed Jul 2, 2024

View reviewed changes

immrsd changed the title ~~Resolves #1019~~ ERC721 URI Storage Jul 2, 2024

andrew-fleming reviewed Jul 9, 2024

View reviewed changes

gerceboss requested review from andrew-fleming and immrsd July 10, 2024 18:28

andrew-fleming reviewed Jul 12, 2024

View reviewed changes

gerceboss requested a review from andrew-fleming July 12, 2024 23:14

gerceboss force-pushed the feature/ERC721URIstorage-component-#1019 branch from e605ffe to eacbd42 Compare July 15, 2024 11:40

andrew-fleming reviewed Jul 15, 2024

View reviewed changes

gerceboss force-pushed the feature/ERC721URIstorage-component-#1019 branch from 507eadb to e4e7368 Compare July 15, 2024 20:17

gerceboss requested a review from andrew-fleming July 15, 2024 20:27

andrew-fleming approved these changes Jul 17, 2024

View reviewed changes

gerceboss requested a review from andrew-fleming July 23, 2024 17:51

immrsd requested changes Aug 4, 2024

View reviewed changes

updated with recent implementation

4789518

gerceboss added 21 commits August 5, 2024 16:37

added mock and tests ERC721URIstorage

deaceb7

added SAMPLE_URI and tests

f33d1c4

included paths removed bugs

14df649

fixed implementation of ERC721URIstorage extension

1421c64

updated documentation

7e13d49

updated documentation

fc03422

removed unused imports

ff4afb9

fixed typos lints

ec6ab2e

updated test implementation

2739e77

updated test_set_token_uri_nonexistent

0a47702

added suggestions-Changed variable names

87e8870

applied changes removed unnecessary internalImpl

9b86c3b

fixed linting

6728f49

added CHANGELOG resolved suggestions

5fcb2dd

added test_token_uri_persists_when_burnt_and_minted

2d43901

tested on test network

a0b2a17

applied suggestions fixed documentation

a12f5e5

updated cairo version

c52b60d

fixed linting

9804882

removed LegacyMap added Map

ca1eddc

fixed linting

5f2a7de

gerceboss force-pushed the feature/ERC721URIstorage-component-#1019 branch from 0fc0b3b to 5f2a7de Compare August 5, 2024 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ERC721 URI Storage #1031

ERC721 URI Storage #1031

gerceboss commented Jul 1, 2024 •

edited

Loading

andrew-fleming commented Jul 1, 2024

gerceboss commented Jul 1, 2024 •

edited

Loading

immrsd Jul 2, 2024

gerceboss Jul 2, 2024 •

edited

Loading

immrsd Jul 2, 2024

immrsd Jul 2, 2024

immrsd Jul 2, 2024

immrsd Jul 2, 2024

immrsd Jul 2, 2024

immrsd Jul 2, 2024

immrsd Jul 2, 2024

gerceboss Jul 2, 2024 •

edited

Loading

gerceboss commented Jul 9, 2024

andrew-fleming left a comment

andrew-fleming left a comment

andrew-fleming left a comment

andrew-fleming left a comment

ericnordelo commented Jul 31, 2024

andrew-fleming commented Jul 31, 2024 •

edited

Loading

ericnordelo commented Aug 1, 2024 •

edited

Loading

andrew-fleming commented Aug 1, 2024

gerceboss commented Aug 1, 2024 •

edited

Loading

andrew-fleming commented Aug 1, 2024

gerceboss commented Aug 1, 2024

immrsd Aug 4, 2024

immrsd Aug 4, 2024

immrsd Aug 4, 2024

gerceboss commented Aug 5, 2024

andrew-fleming commented Aug 5, 2024

gerceboss commented Aug 11, 2024

andrew-fleming commented Aug 15, 2024

	pub mod ERC721URIstorageComponent {
	pub mod ERC721URIStorageComponent {

	token_uris: LegacyMap<u256, ByteArray>,
	ERC721URIStorage_token_uris: LegacyMap<u256, ByteArray>,

	MetadataUpdate: MetadataUpdate,
	MetadataUpdated: MetadataUpdated,

	/// Returns the Uniform Resource Identifier (URI) for the `token_id` token.

	/// Returns the Uniform Resource Identifier (URI) for the `token_id` token.

	self.emit(MetadataUpdated { token_id: token_id });
	self.emit(MetadataUpdated { token_id });

ERC721 URI Storage #1031

Are you sure you want to change the base?

ERC721 URI Storage #1031

Conversation

gerceboss commented Jul 1, 2024 • edited Loading

PR Checklist

andrew-fleming commented Jul 1, 2024

gerceboss commented Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

gerceboss Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerceboss Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

gerceboss commented Jul 9, 2024

andrew-fleming left a comment

Choose a reason for hiding this comment

andrew-fleming left a comment

Choose a reason for hiding this comment

andrew-fleming left a comment

Choose a reason for hiding this comment

andrew-fleming left a comment

Choose a reason for hiding this comment

ericnordelo commented Jul 31, 2024

andrew-fleming commented Jul 31, 2024 • edited Loading

ericnordelo commented Aug 1, 2024 • edited Loading

andrew-fleming commented Aug 1, 2024

gerceboss commented Aug 1, 2024 • edited Loading

andrew-fleming commented Aug 1, 2024

gerceboss commented Aug 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerceboss commented Aug 5, 2024

andrew-fleming commented Aug 5, 2024

gerceboss commented Aug 11, 2024

andrew-fleming commented Aug 15, 2024

gerceboss commented Jul 1, 2024 •

edited

Loading

gerceboss commented Jul 1, 2024 •

edited

Loading

gerceboss Jul 2, 2024 •

edited

Loading

gerceboss Jul 2, 2024 •

edited

Loading

andrew-fleming commented Jul 31, 2024 •

edited

Loading

ericnordelo commented Aug 1, 2024 •

edited

Loading

gerceboss commented Aug 1, 2024 •

edited

Loading