OpenZeppelin / cairo-contracts

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

Use `ByteArray` for strings #857

Closed andrew-fleming closed 7 months ago

andrew-fleming commented 9 months ago

Fixes #817.

This PR is stalled until ByteArrays can be added to Storage.

The SRC5 interface IDs for ISRC6, IERC20Metadata, and IERC721Metadata still need to be updated.

PR Checklist

rsodre commented 7 months ago

Too bad this missed the 0.9.0 release.

When is this planned to be released?

andrew-fleming commented 7 months ago

When is this planned to be released?

Hey, @rsodre! We expect this to be included in the release within the next two weeks. Our development cycle should be merged soon #914 which documents our whole process and what to expect. TL;DR we plan to release every three weeks and our current and forthcoming milestones are documented in the GitHub Project

andrew-fleming commented 7 months ago

This PR proposes to update the logic revolving how token URIs are handled.

Due to limitations with previous iterations of Cairo, token URIs returned a single felt and token URIs were stored individually in a mapping—meaning each token ID pointed to a unique URI.

This PR now proposes to store a single base URI and use token_uri to return the concatenation of the base URI and target token ID by default. This approach mirrors OZ’s default Solidity implementation.

The Good

Tokens can be minted without the additional cost of setting the URI for that specific token. The following gas estimates were gathered using a modified ERC721 preset contract:

URI strategy mint cost
(CURRENT) mapping token ID to URI 427660
(PROPOSED) base URI + token ID 282040

Gas savings: 145620 (34.05%)

The Bad

token_uri calls will be dynamically more expensive within transactions. The bigger the token ID, the more expensive. The following table benchmarks an example using different token URI strategies:

URI strategy token_uri cost (small u256) token_uri cost (max u256) increase %
(CURRENT) mapping token ID to URI 114790 - -
(PROPOSED) base URI + token ID 249460 2081060 734.226%
URI storage - base URI + token ID 373030 2204630 491.006%
URI storage - storage read, no base URI 314120 - -

To reiterate, this PR proposes for token_uri to concatenate the base URI with the token ID to mirror OZ's default Solidity implementation. I think it makes more sense to offer ERC721URIStorage as an extension/impl to keep contracts lighter.

martriay commented 7 months ago

token_uri calls will be dynamically more expensive within transactions.

It should be noted that token_uri is rarely called within transactions, instead it is mostly queried offchain for free.