Use Cairo 1+ and OpenZeppelin Contracts for Cairo v0.8.0

[ericglau]

Preview: https://deploy-preview-305--openzeppelin-contracts-wizard.netlify.app/cairo

Cairo-specific highlighting will be handled in a separate PR.

[socket-security]

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
@types/node	18.19.1	None	+0	3.82 MB	types

[ericnordelo]

Hey Eric, this may be intentional, but In the token name, for example, I can use spaces and symbols like ; or any unicode one, and this is not reflected on the class/module name, but it is reflected on the registered token name. Shouldn't we add more strict validation to inputs?

[ericglau]

@ericnordelo Thanks for the suggestion.

The intention is to sanitize the user inputted name into a separate valid identifier name and a valid string. These can be different as there is no requirement for them to be exactly the same, and I think it provides a better UX to sanitize rather than give an error if invalid characters are provided.

I've updated the sanitization to conform to Cairo's rules for identifiers and short strings as much as possible. The logic for these are defined here.

[ericglau]

@martriay @ericnordelo @andrew-fleming This PR is ready for review. Thanks!

[ericnordelo]

Looking good Eric! Some comments:

1. To reduce the number of imports. I think we should convert this:

    use openzeppelin::token::erc20::interface::IERC20;
    use openzeppelin::token::erc20::interface::IERC20CamelOnly;
    use openzeppelin::token::erc20::interface::ISafeAllowance;
    use openzeppelin::token::erc20::interface::ISafeAllowanceCamel;

Into this:

    use openzeppelin::token::erc20::interface;

    ...

    #[external(v0)]
    impl ERC20Impl of interface::IERC20<ContractState> {
        ...
    }

We could also have:

    use openzeppelin::token::erc20::interface::{
        IERC20, IERC20CamelOnly, ISafeAllowance, ISafeAllowanceCamel
    };

But I like the former the most.

This also applies for ERC721.

[ericnordelo]

2. Update External to ExternalImpl:

#[generate_trait]
    #[external(v0)]
    impl External of ExternalTrait {

to

#[generate_trait]
    #[external(v0)]
    impl ExternalImpl of ExternalTrait {

I know we have it without it in the docs, that's my fault, but it should be ExternalImpl to be consistent with the unwritten patterns we follow. I will update the docs accordingly.

[ericnordelo]

3. When clicking the Download button, the message should be updated, from requiring the python package, to require scarb with openzeppelin as a dependency.

[ericnordelo]

Besides the comments I left, it looks ready to go to me!

[martriay]

This looks really great! Thanks.

I see upgrades and access control are not affected by pausable, is it intentional?

Do you think it makes sense to add some warning/visual feedback when the values are not right? For example

• empty module name (token metadata is fine to be empty)
• empty license ("" results in MIT, but " " does not)
• the provided metadata values exceed what can be represented by short strings. right now this is "sanitized" but the user has no feedback about it, potentially resulting in a different behavior than expected.

[ericglau]

@martriay

I see upgrades and access control are not affected by pausable, is it intentional?

This was intentional. I see a reason to have them unaffected: if there is a vulnerability identified in a contract, the pauser may want to pause it, but not unpause it until after they have upgraded the contract. Is there a reason to do otherwise?

Do you think it makes sense to add some warning/visual feedback when the values are not right? For example
empty module name (token metadata is fine to be empty)

Changed this to an error if identifier is empty.

empty license ("" results in MIT, but " " does not)

This is currently the same as Solidity, although SPDX-License-Identifier is strongly encouraged by the Solidity compiler. For Cairo, perhaps it could omit the SPDX-License-Identifier if the user input is "" (but not " "). What do you think?

the provided metadata values exceed what can be represented by short strings. right now this is "sanitized" but the user has no feedback about it, potentially resulting in a different behavior than expected.

Changed this to an error if short string length > 31

[martriay]

This was intentional. I see a reason to have them unaffected: if there is a vulnerability identified in a contract, the pauser may want to pause it, but not unpause it until after they have upgraded the contract. Is there a reason to do otherwise?

For upgrades I guess you can unpause + upgrade in the same tx as we have built-in multicall in Starknet thanks to the standard account design, but I think your pattern works too. For access control, I guess you could argue that if you don't pause it you cannot prevent a compromised role admin from making access changes. Not a very strong case though.

Changed this to an error if short string length > 31

maybe it's intentional/not worth fixing, but this only gets displayed if it's the first error in the form. if the name is empty, this error won't be shown. Should we do the same for decimals?

[ericglau]

@martriay

For upgrades I guess you can unpause + upgrade in the same tx as we have built-in multicall in Starknet thanks to the standard account design, but I think your pattern works too. For access control, I guess you could argue that if you don't pause it you cannot prevent a compromised role admin from making access changes. Not a very strong case though.

We haven't established a pattern to make these pausable, and this is the same for Solidity (UUPS or access control). If there is a strong case for changing these, we should consider it for both Solidity and Cairo along with any security implications.

maybe it's intentional/not worth fixing, but this only gets displayed if it's the first error in the form. if the name is empty, this error won't be shown. Should we do the same for decimals?

This is related to how we are handling the error. Once there is an error, we raise an exception to the UI and nothing else takes effect until that error is fixed. Opened #312 to consider improving this.

[andrew-fleming]

The wizard looks excellent, Eric! A couple notes:

1. When adding PausableComponent, ERC20 can mint and ERC721 can safe_mint while the contract is paused. Is this intentional?
2. When adding AccessControlComponent, both token contracts can only be burned by the owner. IMO the expected behavior would be having a BURNER_ROLE handle the burning. WDYT?

[ericglau]

@andrew-fleming

1. When adding PausableComponent, ERC20 can mint and ERC721 can safe_mint while the contract is paused. Is this intentional?

Good catch, looks like this was missed. I think we should add this!

2. When adding AccessControlComponent, both token contracts can only be burned by the owner. IMO the expected behavior would be having a BURNER_ROLE handle the burning. WDYT?

I don't think that would be appropriate.

ERC20:
For Solidity, the ERC20Burnable extension states it is for "destruction of own tokens" and "allows token holders to destroy both their own tokens and those that they have an allowance for".
For Cairo, we support the former. I'm not sure how to implement the latter (or whether it is important). But regardless, I don't think the intent should be to allow a BURNER_ROLE to burn anyone's tokens, since that would be quite different than what people are used to. Let me know what you think.

ERC721:
For Solidity, the ERC721Burnable extension states it is for "A way for token holders to burn their own tokens." The API docs for _burn state "The caller must own tokenId or be an approved operator."
For Cairo, I think we already support both of these with assert(self.erc721._is_approved_or_owner(caller, token_id), ERC721Component::Errors::UNAUTHORIZED);. Again, I don't think a BURNER_ROLE should be allowed to burn anyone's tokens.

[andrew-fleming]

@ericglau

ERC20:
For Solidity, the ERC20Burnable extension states it is for "destruction of own tokens" and "allows token holders to destroy both their own tokens and those that they have an allowance for".
For Cairo, we support the former. I'm not sure how to implement the latter (or whether it is important). But regardless, I don't think the intent should be to allow a BURNER_ROLE to burn anyone's tokens, since that would be quite different than what people are used to. Let me know what you think.

ERC721:
For Solidity, the ERC721Burnable extension states it is for "A way for token holders to burn their own tokens." The API docs for _burn state "The caller must own tokenId or be an approved operator."
For Cairo, I think we already support both of these with assert(self.erc721._is_approved_or_owner(caller, token_id), ERC721Component::Errors::UNAUTHORIZED);. Again, I don't think a BURNER_ROLE should be allowed to burn anyone's tokens.

You're correct, good call! I was thinking of the examples we have in the AccessControl docs which includes the BURNER_ROLE role, but that doesn't reflect the behavior of the burnable extensions

[ericglau]

Changed ERC20 mint and ERC721 safe_mint to be affected by pausable.

[martriay]

We haven't established a pattern to make these pausable, and this is the same for Solidity (UUPS or access control). If there is a strong case for changing these, we should consider it for both Solidity and Cairo along with any security implications.

Agree. I expect these patterns to be written down, analyzed, and applied cross-projects as long as it make sense

I was thinking of the examples we have in the AccessControl docs which includes the BURNER_ROLE role, but that doesn't reflect the behavior of the burnable extensions

Looking at it now, I wonder if it's a good example to have.

[martriay]

Btw I noticed toggling on/off Mintable in this context changes the order between Ownable and Pausable in many parts (component definition, import, etc). Super low priority, but it might make sense for them to be always ordered with the same criteria.

[martriay]

Changed ERC20 mint and ERC721 safe_mint to be affected by pausable.

Don't we need a safeMint one too?

[ericglau]

@martriay Added safeMint, and reordered some functions so the output ordering is much more stable now.

[andrew-fleming]

I was thinking of the examples we have in the AccessControl docs which includes the BURNER_ROLE role, but that doesn't reflect the behavior of the burnable extensions

Looking at it now, I wonder if it's a good example to have.

Perhaps with Cairo. It'd be a better example if we had an embeddable <token>BurnableImpl to abstract out the standard burn functionality

[andrew-fleming]

Improvements look good! I left a final suggestion

[andrew-fleming]

LGTM!