OpenZeppelin / cairo-contracts

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

Remove dualcase SRC5 #882

Closed TAdev0 closed 3 months ago

TAdev0 commented 5 months ago

Fixes #841

PR Checklist

TAdev0 commented 5 months ago

@martriay what would be the best way to test this removal feature on a public network?

martriay commented 5 months ago

maybe for this module we don't need that

TAdev0 commented 5 months ago

@martriay i don't know why, but after removing dualSRC5, i get a new class hash for ERC721 (which is expected), but i get the same class hash for Account. Is this the normal behaviour? Account also exposes SRC5Impl, the class hash shoud also be modified, no?

TAdev0 commented 5 months ago

mmh actually it seems neither ERC721 nor Account exposes SRC5Camel, this means the class hash shouldn't change for both?

TAdev0 commented 5 months ago

update: now i get it . I changed _check_on_erc721_received function in the ERC721 so that it doesn't use DualCaseSRC5anymore, this is why class hash changed for ERC721 and not for Account !

martriay commented 5 months ago

Then we should test those presets onchain.

TAdev0 commented 5 months ago

How should i test that? Deploying the ERC721 preset and check that _check_on_ERC721_received works as expected?

TAdev0 commented 4 months ago

@martriay ok now i found out the issue : removing dualcase SRC5 will break compatibility in some cases.

I cannot send ERC721 tokens to a Braavos Goerli wallet cairo0, because it only implements camelCase supportsInterface. I finally succceded with an Argent wallet cairo2 as a receiver, with a succesfull on_erc721_received callback.

Braavos account contract are still in cairo 0 on mainnet, with only camelCase impl for supportInterface function. Any ERC721 transfers to such an account will fail if the ERC721 contract doesnt implement dualcase SRC5.

Here is the goerli address of the ERC721 contract i deployed, minting 1 token to the deployer. 0x6c087b2e2bc55bb8168cabe4486761787bf3aac4d77a4c39a0a8de4385493c7

And the tx hash of the safeTransferFrom call, to transfer 1 token on a Argent wallet 0x73a77d6f740162aaff45e709cb93f7e280f9c4bee4fbb973af87f32707d8993

ericnordelo commented 4 months ago

Braavos account contract are still in cairo 0 on mainnet, with only camelCase impl for supportInterface function. Any ERC721 transfers to such an account will fail if the ERC721 contract doesnt implement dualcase SRC5.

With dualcase SRC5 they shouldn't work either, because the interface used in Cairo 0 was ERC165 like, not the one we are using with SRC5 nowadays.

TAdev0 commented 4 months ago

@ericnordelo oh ok thank you very much for these informations!

TAdev0 commented 4 months ago

@ericnordelo i think we should also remove all the test_dual_supportsInterface functions in tests?

TAdev0 commented 4 months ago

@ericnordelo pr updated.

There are still some mentions to camelCase supportsInterface in various files (mocks especially), but i think these ones shouldn't be removed?

TAdev0 commented 4 months ago

@andrew-fleming just updated the PR. Now supportsInterface is removed everywhere.

There is just one mention, still, in erc1155.doc. But there is no snake case supports_interface (its erc165 compliant) so i guess we should leave it like this ?

ericnordelo commented 3 months ago

Thanks again for taking the time @TAdev0. After solving the conflicts, we can merge this to main already.

TAdev0 commented 3 months ago

@ericnordelo @andrew-fleming i resolved conflicts and updated the code for ERC1155 which used again the DualCaseSRC5 interface.

For now, i'm having issues declaring the contracts on a testnet, i don"t know why. ERC721 and ERC1155 class hashes still need to be updated, can you take care of it? I can try again tomorrow but sncast seems not to be working on sepolia

andrew-fleming commented 3 months ago

For now, i'm having issues declaring the contracts on a testnet, i don"t know why...I can try again tomorrow but sncast seems not to be working on sepolia

The RPC probably hasn't updated to Sierra 1.5 yet. I switched to blast and declared/deployed the relevant presets in this PR on sepolia.

ERC721

ERC1155


ERC721 and ERC1155 class hashes still need to be updated, can you take care of it?

The class hashes are only going to be updated during releases from now on, so we don't have to worry about that anymore. And I think we're good to go :) thanks, @TAdev0!

TAdev0 commented 3 months ago

great! thank you :)