Remove dualcase SRC5

[TAdev0]

Fixes #841

PR Checklist

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

[TAdev0]

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

[martriay]

maybe for this module we don't need that

[TAdev0]

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

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

[TAdev0]

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]

Then we should test those presets onchain.

[TAdev0]

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

[martriay]

The PR is looking good! Yes, we need to test the callback works properly, and we wanted to go the extra mile we could also test that it does not work for camelCase ERC721Receivers, although that's very complex to set up, not really needed I guess.

[TAdev0]

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

Looking good! Left a couple of comments and we still need to remove the supportsInterface method from ABIs like the Account and the EDRC721 ones in the interface files.

[ericnordelo]

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]

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

[TAdev0]

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

[TAdev0]

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

[ericnordelo]

LGTM!

[andrew-fleming]

Thanks for picking this up, @TAdev0! I left a couple comments, but we're looking good so far.

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

I think they can be removed. The camelCase supportsInterface was only there to support the dual casings and I don't see why we should keep them. This also includes removing the supportsInterface fallback from the dual case modules (example here). The try_selector_with_fallback should be replaced with the standard call_contract_syscall. Correct me if I'm wrong @martriay @ericnordelo

[TAdev0]

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

[andrew-fleming]

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 ?

Yeah, the erc1155 doc is from cairo v0, so no need to worry about that here. LGTM! Thanks again for picking this up 🙏

[ericnordelo]

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

[TAdev0]

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

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

• class hash: 0x04cd23b9cf1f3206bef278fe4a4cf6c34e88a7ae20d85884e3d6a1bf8df06693
• deployment: 0x02f388c3838e61631427820dcb7d26c68ab5bc5310720537608793f9c6251e7b

ERC1155

• class hash: 0x07a46b8cf4360d03c4fd78e2abe3105908708d1a6ff652bad84fa4f3e8010c4d
• deployment: 0x05085b6b99893a9cae9225682d1df0f6ee9b083a111ae7f6b0935b3308e6844e

---

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]

great! thank you :)