From 0f9d3d7dd29fe4b7a676b7a9022a97371a36ba71 Mon Sep 17 00:00:00 2001 From: Andrew Date: Thu, 16 Nov 2023 21:03:33 -0500 Subject: [PATCH 01/26] start migration guide --- docs/modules/ROOT/nav.adoc | 1 + .../ROOT/pages/guides/src5-migration.adoc | 69 +++++++++++++++++++ docs/modules/ROOT/pages/introspection.adoc | 2 + 3 files changed, 72 insertions(+) create mode 100644 docs/modules/ROOT/pages/guides/src5-migration.adoc diff --git a/docs/modules/ROOT/nav.adoc b/docs/modules/ROOT/nav.adoc index e1b2dc12a..c3155749e 100644 --- a/docs/modules/ROOT/nav.adoc +++ b/docs/modules/ROOT/nav.adoc @@ -23,6 +23,7 @@ ** xref:/api/security.adoc[API Reference] * xref:introspection.adoc[Introspection] +** xref:/guides/src5-migration.adoc[SRC5 Migration] ** xref:/api/introspection.adoc[API Reference] // * xref:udc.adoc[Universal Deployer Contract] diff --git a/docs/modules/ROOT/pages/guides/src5-migration.adoc b/docs/modules/ROOT/pages/guides/src5-migration.adoc new file mode 100644 index 000000000..6d560118c --- /dev/null +++ b/docs/modules/ROOT/pages/guides/src5-migration.adoc @@ -0,0 +1,69 @@ += SRC5 Migration + +== Migrating to SRC5 + +WARNING: Thoroughly test the process with the target contracts before performing the migration. + +This guide is for already-deployed, upgradeable contracts that support ERC165. +Migrating to SRC5 requires integrating `SRC5Component` and `UpgradeableComponent`. +The migration also requires re-registering interface support. +The following template assumes that the upgradeable mechanism uses the `upgrade` selector: + +[,javascript] +---- +#[starknet::contract] + +#[starknet::contract] +mod MigratingContract { + use openzeppelin::introspection::src5::SRC5Component; + use openzeppelin::upgrades::UpgradeableComponent; + use starknet::ClassHash; + use starknet::ContractAddress; + + component!(path: SRC5Component, storage: src5, event: SRC5Event); + component!(path: UpgradeableComponent, storage: upgradeable, event: UpgradeableEvent); + + // SRC5 + #[abi(embed_v0)] + impl SRC5Impl = SRC5Component::SRC5Impl; + #[abi(embed_v0)] + impl SRC5CamelOnlyImpl = SRC5Component::SRC5CamelImpl; + impl SRC5InternalImpl = SRC5Component::InternalImpl; + + // Upgradeable + impl UpgradeableInternalImpl = UpgradeableComponent::InternalImpl; + + #[storage] + struct Storage { + #[substorage(v0)] + src5: SRC5Component::Storage, + #[substorage(v0)] + upgradeable: UpgradeableComponent::Storage, + } + + #[event] + #[derive(Drop, starknet::Event)] + enum Event { + #[flat] + SRC5Event: SRC5Component::Event, + #[flat] + UpgradeableEvent: UpgradeableComponent::Event + } + + #[external(v0)] + fn upgrade(ref self: ContractState, new_class_hash: ClassHash) { + // Add permissions mechanism + self.upgradeable._upgrade(new_class_hash); + } + + // The rest of the contract logic +} + +---- + +Please keep in mind that the SRC5 interface IDs are calculated differently than those in ERC165. +Contracts that leverage introspection like ERC721 will also need to be upgraded in order to query the correct interface ID. + +Deployed contracts with upgradeability should also be carefully tested before migrating. +Upgradeable contracts will likely include some form of a permissions mechanism for upgrading contracts. +Take extreme care with ensuring admins are not inadvertantly stripped of their permissions. diff --git a/docs/modules/ROOT/pages/introspection.adoc b/docs/modules/ROOT/pages/introspection.adoc index 6bba1ff71..f15717df3 100644 --- a/docs/modules/ROOT/pages/introspection.adoc +++ b/docs/modules/ROOT/pages/introspection.adoc @@ -99,3 +99,5 @@ mod MyContract { TIP: If you are unsure whether a contract implements SRC5 or not, you can follow the process described in https://github.com/starknet-io/SNIPs/blob/main/SNIPS/snip-5.md#how-to-detect-if-a-contract-implements-src-5[here]. + +xref:/guides/src5-migration.adoc[Counterfactual deployments] From f2ecaf3ee772e9bf456b14f38ad7bb5630242805 Mon Sep 17 00:00:00 2001 From: Andrew Date: Fri, 17 Nov 2023 20:57:03 -0500 Subject: [PATCH 02/26] add links, add register_interfaces example --- .../ROOT/pages/guides/src5-migration.adoc | 59 ++++++++++++++++--- 1 file changed, 52 insertions(+), 7 deletions(-) diff --git a/docs/modules/ROOT/pages/guides/src5-migration.adoc b/docs/modules/ROOT/pages/guides/src5-migration.adoc index 6d560118c..71f436189 100644 --- a/docs/modules/ROOT/pages/guides/src5-migration.adoc +++ b/docs/modules/ROOT/pages/guides/src5-migration.adoc @@ -1,18 +1,29 @@ = SRC5 Migration +:eip165: https://eips.ethereum.org/EIPS/eip-165[EIP-165] +:snip5: https://github.com/starknet-io/SNIPs/blob/main/SNIPS/snip-5.md[SNIP-5] +:dual-interface-discussion: https://github.com/OpenZeppelin/cairo-contracts/discussions/640[Dual Introspection Detection] +:shamans-proposal: https://community.starknet.io/t/starknet-standard-interface-detection/92664[Starknet Shamans proposal] + +In the smart contract ecosystem, having the ability to query if a contract supports a given interface is an extremely important feature. +The initial introspection design in Cairo v0 followed Ethereum's {eip165}. +Since the language evolved and acquired some stability, we're now migrating to the {snip5} standard for introspection. +SNIP-5 allows interface ID calculations to use Cairo types and the Starknet keccak (`sn_keccak`) function. +For more information on the decision, see the {shamans-proposal} or the {dual-interface-discussion} discussion. + == Migrating to SRC5 -WARNING: Thoroughly test the process with the target contracts before performing the migration. +:src5-component: xref:api/introspection#SRC5Component[SRC5Component] +:upgradeable-component: xref:api/upgrades#UpgradeableComponent[UpgradeableComponent] +:isrc6: xref:api/account.adoc#ISRC6[ISRC6] +:src5-rs: https://github.com/ericnordelo/src5-rs[src5-rs] This guide is for already-deployed, upgradeable contracts that support ERC165. -Migrating to SRC5 requires integrating `SRC5Component` and `UpgradeableComponent`. -The migration also requires re-registering interface support. +Migrating to SRC5 requires integrating {src5-component} and {upgradeable-component}. The following template assumes that the upgradeable mechanism uses the `upgrade` selector: [,javascript] ---- -#[starknet::contract] - #[starknet::contract] mod MigratingContract { use openzeppelin::introspection::src5::SRC5Component; @@ -58,11 +69,45 @@ mod MigratingContract { // The rest of the contract logic } +---- + +The contract will also need to register the interface ID values in order to continue declaring support for those interfaces. +We can create a migration initializer to handle this which needs to be called after the contract upgrade. + +TIP: OpenZeppelin's Contracts for Cairo already provides the interface IDs of common interfaces in the API section (like {isrc6}). +For interfaces not provided, tools such as {src5-rs} can help with interface ID calculation. + +[,javascript] +---- +#[starknet::contract] +mod MigratingContract { + + (...) + + #[external(v0)] + fn initialize_src5_migration(ref self: ContractState, interface_ids: Span) { + // Add permissions mechanism + self.register_interfaces(interface_ids); + } + + #[generate_trait] + impl InternalImpl of InternalTrait { + fn register_interfaces(ref self: ContractState, mut interface_ids: Span) { + loop { + if interface_ids.len() == 0 { + break; + } + let id = *interface_ids.pop_front().unwrap(); + self.src5.register_interface(id); + } + } + } +} ---- -Please keep in mind that the SRC5 interface IDs are calculated differently than those in ERC165. -Contracts that leverage introspection like ERC721 will also need to be upgraded in order to query the correct interface ID. +Note that SRC5 interface IDs are calculated differently than those in ERC165. +Because of this, contracts that leverage introspection (like ERC721) will also need to be upgraded in order to query the correct interface IDs. Deployed contracts with upgradeability should also be carefully tested before migrating. Upgradeable contracts will likely include some form of a permissions mechanism for upgrading contracts. From e812a8733adb93a82221384255f17b9e740c2f1b Mon Sep 17 00:00:00 2001 From: Andrew Date: Fri, 17 Nov 2023 23:03:03 -0500 Subject: [PATCH 03/26] add supports_interfaces --- src/introspection/src5.cairo | 12 ++++++++++++ src/tests/introspection/test_src5.cairo | 26 +++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/src/introspection/src5.cairo b/src/introspection/src5.cairo index 9b1a8cab4..36965b27c 100644 --- a/src/introspection/src5.cairo +++ b/src/introspection/src5.cairo @@ -50,6 +50,18 @@ mod SRC5Component { self.SRC5_supported_interfaces.write(interface_id, true); } + // Registers the given interfaces as supported by the contract. + fn register_interfaces(ref self: ComponentState, mut interface_ids: Span) { + loop { + if interface_ids.len() == 0 { + break; + } + + let id = *interface_ids.pop_front().unwrap(); + self.register_interface(id); + } + } + /// Deregisters the given interface as supported by the contract. fn deregister_interface(ref self: ComponentState, interface_id: felt252) { assert(interface_id != interface::ISRC5_ID, Errors::INVALID_ID); diff --git a/src/tests/introspection/test_src5.cairo b/src/tests/introspection/test_src5.cairo index db8b53ce3..e49191c60 100644 --- a/src/tests/introspection/test_src5.cairo +++ b/src/tests/introspection/test_src5.cairo @@ -3,6 +3,7 @@ use openzeppelin::introspection::src5::SRC5Component::InternalTrait; use openzeppelin::tests::mocks::src5_mocks::DualCaseSRC5Mock; const OTHER_ID: felt252 = 0x12345678; +const OTHER_ID_2: felt252 = 0x87654321; fn STATE() -> DualCaseSRC5Mock::ContractState { DualCaseSRC5Mock::contract_state_for_testing() @@ -33,6 +34,31 @@ fn test_register_interface() { assert(supports_new_interface, 'Should support new interface'); } +#[test] +#[available_gas(2000000)] +fn test_register_interfaces() { + let mut state = STATE(); + let ids = array![OTHER_ID, OTHER_ID_2]; + state.src5.register_interfaces(ids.span()); + + let supports_id = state.src5.supports_interface(OTHER_ID); + assert(supports_id, 'Should support new interface'); + + let supports_id_2 = state.src5.supports_interface(OTHER_ID_2); + assert(supports_id_2, 'Should support new interface'); +} + +#[test] +#[available_gas(2000000)] +fn test_register_interfaces_one_id() { + let mut state = STATE(); + let ids = array![OTHER_ID]; + state.src5.register_interfaces(ids.span()); + + let supports_id = state.src5.supports_interface(OTHER_ID); + assert(supports_id, 'Should support new interface'); +} + #[test] #[available_gas(2000000)] fn test_deregister_interface() { From 7ce26168ea334b23961106c38389d70772346275 Mon Sep 17 00:00:00 2001 From: Andrew Date: Fri, 17 Nov 2023 23:03:21 -0500 Subject: [PATCH 04/26] add supports_interfaces to docs --- .../modules/ROOT/pages/api/introspection.adoc | 7 ++++++ .../ROOT/pages/guides/src5-migration.adoc | 25 +++---------------- 2 files changed, 11 insertions(+), 21 deletions(-) diff --git a/docs/modules/ROOT/pages/api/introspection.adoc b/docs/modules/ROOT/pages/api/introspection.adoc index 91def5579..e8dba8149 100644 --- a/docs/modules/ROOT/pages/api/introspection.adoc +++ b/docs/modules/ROOT/pages/api/introspection.adoc @@ -66,6 +66,7 @@ SRC5 component extending xref:ISRC5[`ISRC5`]. .InternalImpl * xref:#SRC5Component-register_interface[`++register_interface(self, interface_id)++`] +* xref:#SRC5Component-register_interfaces[`++register_interfaces(self, interface_ids)++`] * xref:#SRC5Component-deregister_interface[`++deregister_interface(self, interface_id)++`] -- @@ -87,6 +88,12 @@ See xref:ISRC5-supports_interface[`ISRC5::supports_interface`]. Registers support for the given `interface_id`. +[.contract-item] +[[SRC5Component-register_interfaces]] +==== `[.contract-item-name]#++register_interfaces++#++(ref self: ComponentState, interface_ids: Span)++` [.item-kind]#internal# + +Registers support for the given `interface_ids`. + [.contract-item] [[SRC5Component-deregister_interface]] ==== `[.contract-item-name]#++deregister_interface++#++(ref self: ComponentState, interface_id: felt252)++` [.item-kind]#internal# diff --git a/docs/modules/ROOT/pages/guides/src5-migration.adoc b/docs/modules/ROOT/pages/guides/src5-migration.adoc index 71f436189..71781763a 100644 --- a/docs/modules/ROOT/pages/guides/src5-migration.adoc +++ b/docs/modules/ROOT/pages/guides/src5-migration.adoc @@ -71,7 +71,7 @@ mod MigratingContract { } ---- -The contract will also need to register the interface ID values in order to continue declaring support for those interfaces. +The contract will also need to register the new SRC5 interface IDs in order to continue declaring support for those interfaces. We can create a migration initializer to handle this which needs to be called after the contract upgrade. TIP: OpenZeppelin's Contracts for Cairo already provides the interface IDs of common interfaces in the API section (like {isrc6}). @@ -85,30 +85,13 @@ mod MigratingContract { (...) #[external(v0)] - fn initialize_src5_migration(ref self: ContractState, interface_ids: Span) { + fn migration_initializer(ref self: ContractState, interface_ids: Span) { // Add permissions mechanism - self.register_interfaces(interface_ids); - } - - #[generate_trait] - impl InternalImpl of InternalTrait { - fn register_interfaces(ref self: ContractState, mut interface_ids: Span) { - loop { - if interface_ids.len() == 0 { - break; - } - - let id = *interface_ids.pop_front().unwrap(); - self.src5.register_interface(id); - } - } + self.src5.register_interfaces(interface_ids); } } ---- -Note that SRC5 interface IDs are calculated differently than those in ERC165. -Because of this, contracts that leverage introspection (like ERC721) will also need to be upgraded in order to query the correct interface IDs. - -Deployed contracts with upgradeability should also be carefully tested before migrating. +Note that deployed contracts with upgradeability should also be carefully tested before migrating. Upgradeable contracts will likely include some form of a permissions mechanism for upgrading contracts. Take extreme care with ensuring admins are not inadvertantly stripped of their permissions. From d46b75edab0f5dcf5a441d7b83b25bff4212b025 Mon Sep 17 00:00:00 2001 From: Andrew Date: Sun, 19 Nov 2023 13:34:11 -0500 Subject: [PATCH 05/26] fix formatting --- src/introspection/src5.cairo | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/introspection/src5.cairo b/src/introspection/src5.cairo index 36965b27c..0b369f14e 100644 --- a/src/introspection/src5.cairo +++ b/src/introspection/src5.cairo @@ -51,7 +51,9 @@ mod SRC5Component { } // Registers the given interfaces as supported by the contract. - fn register_interfaces(ref self: ComponentState, mut interface_ids: Span) { + fn register_interfaces( + ref self: ComponentState, mut interface_ids: Span + ) { loop { if interface_ids.len() == 0 { break; From 716051133e525aa14f4b51f7bf3086262464bfd5 Mon Sep 17 00:00:00 2001 From: Andrew Date: Sun, 19 Nov 2023 13:40:18 -0500 Subject: [PATCH 06/26] revert change --- docs/modules/ROOT/pages/introspection.adoc | 2 -- 1 file changed, 2 deletions(-) diff --git a/docs/modules/ROOT/pages/introspection.adoc b/docs/modules/ROOT/pages/introspection.adoc index f15717df3..6bba1ff71 100644 --- a/docs/modules/ROOT/pages/introspection.adoc +++ b/docs/modules/ROOT/pages/introspection.adoc @@ -99,5 +99,3 @@ mod MyContract { TIP: If you are unsure whether a contract implements SRC5 or not, you can follow the process described in https://github.com/starknet-io/SNIPs/blob/main/SNIPS/snip-5.md#how-to-detect-if-a-contract-implements-src-5[here]. - -xref:/guides/src5-migration.adoc[Counterfactual deployments] From b0d9190add8a3cdc0cde36b92c24b291cacc3b72 Mon Sep 17 00:00:00 2001 From: Andrew Fleming Date: Tue, 28 Nov 2023 16:30:55 -0500 Subject: [PATCH 07/26] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Martín Triay --- docs/modules/ROOT/nav.adoc | 2 +- docs/modules/ROOT/pages/guides/src5-migration.adoc | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/docs/modules/ROOT/nav.adoc b/docs/modules/ROOT/nav.adoc index c3155749e..4e186784a 100644 --- a/docs/modules/ROOT/nav.adoc +++ b/docs/modules/ROOT/nav.adoc @@ -23,7 +23,7 @@ ** xref:/api/security.adoc[API Reference] * xref:introspection.adoc[Introspection] -** xref:/guides/src5-migration.adoc[SRC5 Migration] +** xref:/guides/src5-migration.adoc[Migrate to SRC5] ** xref:/api/introspection.adoc[API Reference] // * xref:udc.adoc[Universal Deployer Contract] diff --git a/docs/modules/ROOT/pages/guides/src5-migration.adoc b/docs/modules/ROOT/pages/guides/src5-migration.adoc index 71781763a..ac8013ed8 100644 --- a/docs/modules/ROOT/pages/guides/src5-migration.adoc +++ b/docs/modules/ROOT/pages/guides/src5-migration.adoc @@ -1,4 +1,4 @@ -= SRC5 Migration += Migrating ERC165 to SRC5 :eip165: https://eips.ethereum.org/EIPS/eip-165[EIP-165] :snip5: https://github.com/starknet-io/SNIPs/blob/main/SNIPS/snip-5.md[SNIP-5] @@ -6,8 +6,8 @@ :shamans-proposal: https://community.starknet.io/t/starknet-standard-interface-detection/92664[Starknet Shamans proposal] In the smart contract ecosystem, having the ability to query if a contract supports a given interface is an extremely important feature. -The initial introspection design in Cairo v0 followed Ethereum's {eip165}. -Since the language evolved and acquired some stability, we're now migrating to the {snip5} standard for introspection. +The initial introspection design for Contracts for Cairo before version v0.7.0 followed Ethereum's {eip165}. +Since the Cairo language evolved introducing native types, we needed an introspection solution tailored to the Cairo ecosystem: the {snip5} standard. SNIP-5 allows interface ID calculations to use Cairo types and the Starknet keccak (`sn_keccak`) function. For more information on the decision, see the {shamans-proposal} or the {dual-interface-discussion} discussion. @@ -19,8 +19,7 @@ For more information on the decision, see the {shamans-proposal} or the {dual-in :src5-rs: https://github.com/ericnordelo/src5-rs[src5-rs] This guide is for already-deployed, upgradeable contracts that support ERC165. -Migrating to SRC5 requires integrating {src5-component} and {upgradeable-component}. -The following template assumes that the upgradeable mechanism uses the `upgrade` selector: +Migrating to SRC5 requires integrating the {src5-component}. [,javascript] ---- @@ -93,5 +92,5 @@ mod MigratingContract { ---- Note that deployed contracts with upgradeability should also be carefully tested before migrating. -Upgradeable contracts will likely include some form of a permissions mechanism for upgrading contracts. +Upgradeable contracts will likely include some form of xref:access.adoc[access mechanism] for the upgrade function. Take extreme care with ensuring admins are not inadvertantly stripped of their permissions. From 1929fb73fe11cf9bb8e753fdc257235a42304839 Mon Sep 17 00:00:00 2001 From: Andrew Date: Wed, 29 Nov 2023 17:06:31 -0500 Subject: [PATCH 08/26] remove upgrade fn from code block --- docs/modules/ROOT/pages/guides/src5-migration.adoc | 6 ------ 1 file changed, 6 deletions(-) diff --git a/docs/modules/ROOT/pages/guides/src5-migration.adoc b/docs/modules/ROOT/pages/guides/src5-migration.adoc index ac8013ed8..cd2cc0ee3 100644 --- a/docs/modules/ROOT/pages/guides/src5-migration.adoc +++ b/docs/modules/ROOT/pages/guides/src5-migration.adoc @@ -60,12 +60,6 @@ mod MigratingContract { UpgradeableEvent: UpgradeableComponent::Event } - #[external(v0)] - fn upgrade(ref self: ContractState, new_class_hash: ClassHash) { - // Add permissions mechanism - self.upgradeable._upgrade(new_class_hash); - } - // The rest of the contract logic } ---- From 3e89aab7fc9ebea80f5ce46f4cd8a299765d834a Mon Sep 17 00:00:00 2001 From: Andrew Date: Sun, 3 Dec 2023 13:22:27 -0500 Subject: [PATCH 09/26] remove supports_interfaces --- src/introspection/src5.cairo | 14 -------------- src/tests/introspection/test_src5.cairo | 25 ------------------------- 2 files changed, 39 deletions(-) diff --git a/src/introspection/src5.cairo b/src/introspection/src5.cairo index f37d533be..fa47feb1b 100644 --- a/src/introspection/src5.cairo +++ b/src/introspection/src5.cairo @@ -50,20 +50,6 @@ mod SRC5Component { self.SRC5_supported_interfaces.write(interface_id, true); } - // Registers the given interfaces as supported by the contract. - fn register_interfaces( - ref self: ComponentState, mut interface_ids: Span - ) { - loop { - if interface_ids.len() == 0 { - break; - } - - let id = *interface_ids.pop_front().unwrap(); - self.register_interface(id); - } - } - /// Deregisters the given interface as supported by the contract. fn deregister_interface(ref self: ComponentState, interface_id: felt252) { assert(interface_id != interface::ISRC5_ID, Errors::INVALID_ID); diff --git a/src/tests/introspection/test_src5.cairo b/src/tests/introspection/test_src5.cairo index e49191c60..437f33dba 100644 --- a/src/tests/introspection/test_src5.cairo +++ b/src/tests/introspection/test_src5.cairo @@ -34,31 +34,6 @@ fn test_register_interface() { assert(supports_new_interface, 'Should support new interface'); } -#[test] -#[available_gas(2000000)] -fn test_register_interfaces() { - let mut state = STATE(); - let ids = array![OTHER_ID, OTHER_ID_2]; - state.src5.register_interfaces(ids.span()); - - let supports_id = state.src5.supports_interface(OTHER_ID); - assert(supports_id, 'Should support new interface'); - - let supports_id_2 = state.src5.supports_interface(OTHER_ID_2); - assert(supports_id_2, 'Should support new interface'); -} - -#[test] -#[available_gas(2000000)] -fn test_register_interfaces_one_id() { - let mut state = STATE(); - let ids = array![OTHER_ID]; - state.src5.register_interfaces(ids.span()); - - let supports_id = state.src5.supports_interface(OTHER_ID); - assert(supports_id, 'Should support new interface'); -} - #[test] #[available_gas(2000000)] fn test_deregister_interface() { From 170a3e3dcf9bf0128d35612a166cf006caf8cb46 Mon Sep 17 00:00:00 2001 From: Andrew Date: Wed, 6 Dec 2023 01:02:16 -0500 Subject: [PATCH 10/26] add deregister_erc165_interface --- .../test_erc165_deregister.cairo | 42 +++++++++++++++++++ src/tests/mocks.cairo | 1 + src/tests/mocks/erc165_deregister_mock.cairo | 29 +++++++++++++ src/utils.cairo | 16 +++++++ 4 files changed, 88 insertions(+) create mode 100644 src/tests/introspection/test_erc165_deregister.cairo create mode 100644 src/tests/mocks/erc165_deregister_mock.cairo diff --git a/src/tests/introspection/test_erc165_deregister.cairo b/src/tests/introspection/test_erc165_deregister.cairo new file mode 100644 index 000000000..1d69564ec --- /dev/null +++ b/src/tests/introspection/test_erc165_deregister.cairo @@ -0,0 +1,42 @@ +use openzeppelin::tests::mocks::erc165_deregister_mock::ERC165DeregisterMock::ERC165DeregisterMockImpl; +use openzeppelin::tests::mocks::erc165_deregister_mock::ERC165DeregisterMock; + +const ID_1: felt252 = 0x12345678; +const ID_2: felt252 = 0x87654321; + +fn STATE() -> ERC165DeregisterMock::ContractState { + ERC165DeregisterMock::contract_state_for_testing() +} + +#[test] +#[available_gas(2000000)] +fn test_deregister_interface() { + let mut state = STATE(); + + state.register_interface(ID_1); + state.register_interface(ID_2); + + assert(state.supports_interface(ID_1), 'Should support ID_1'); + assert(state.supports_interface(ID_2), 'Should support ID_2'); + + state.deregister_erc165_interface(ID_1); + state.deregister_erc165_interface(ID_2); + + assert(!state.supports_interface(ID_1), 'Should not support ID_1'); + assert(!state.supports_interface(ID_2), 'Should not support ID_1'); +} + +#[test] +#[available_gas(2000000)] +fn test_deregister_interface_not_registered() { + let mut state = STATE(); + + assert(!state.supports_interface(ID_1), 'Should not support ID_1'); + assert(!state.supports_interface(ID_2), 'Should not support ID_1'); + + state.deregister_erc165_interface(ID_1); + state.deregister_erc165_interface(ID_2); + + assert(!state.supports_interface(ID_1), 'Should not support ID_1'); + assert(!state.supports_interface(ID_2), 'Should not support ID_1'); +} diff --git a/src/tests/mocks.cairo b/src/tests/mocks.cairo index 4bc03de22..f69128bd2 100644 --- a/src/tests/mocks.cairo +++ b/src/tests/mocks.cairo @@ -1,6 +1,7 @@ mod accesscontrol_mocks; mod account_mocks; mod erc20_mocks; +mod erc165_deregister_mock; mod erc721_mocks; mod erc721_receiver_mocks; mod initializable_mock; diff --git a/src/tests/mocks/erc165_deregister_mock.cairo b/src/tests/mocks/erc165_deregister_mock.cairo new file mode 100644 index 000000000..737693912 --- /dev/null +++ b/src/tests/mocks/erc165_deregister_mock.cairo @@ -0,0 +1,29 @@ +#[starknet::contract] +mod ERC165DeregisterMock { + use openzeppelin::utils; + + #[storage] + struct Storage { + ERC165_supported_interfaces: LegacyMap, + } + + #[event] + #[derive(Drop, starknet::Event)] + enum Event {} + + #[generate_trait] + #[external(v0)] + impl ERC165DeregisterMockImpl of ExternalTrait { + fn supports_interface(self: @ContractState, interface_id: felt252) -> bool { + self.ERC165_supported_interfaces.read(interface_id) + } + + fn register_interface(ref self: ContractState, interface_id: felt252) { + self.ERC165_supported_interfaces.write(interface_id, true); + } + + fn deregister_erc165_interface(ref self: ContractState, interface_id: felt252) { + utils::deregister_erc165_interface(interface_id); + } + } +} diff --git a/src/utils.cairo b/src/utils.cairo index fdf795415..0928ad008 100644 --- a/src/utils.cairo +++ b/src/utils.cairo @@ -6,10 +6,12 @@ mod selectors; mod serde; mod unwrap_and_cast; +use core::pedersen::pedersen; use starknet::ContractAddress; use starknet::SyscallResult; use starknet::SyscallResultTrait; use starknet::call_contract_syscall; +use starknet::storage_address_try_from_felt252; use unwrap_and_cast::UnwrapAndCast; fn try_selector_with_fallback( @@ -27,6 +29,20 @@ fn try_selector_with_fallback( } } +/// Manually deletes `erc165_id` value from the storage variable +/// `ERC165_supported_interfaces` in the calling contract's storage. +/// This function should only be used by a migration initializer during the +/// ERC165-to-SRC5 migration process. +fn deregister_erc165_interface(erc165_id: felt252) { + let address_domain = 0_u32; + let base_address = selector!("ERC165_supported_interfaces"); + let storage_address = storage_address_try_from_felt252( + pedersen(base_address, erc165_id) + ).unwrap(); + + starknet::storage_write_syscall(address_domain, storage_address, 0); +} + impl BoolIntoFelt252 of Into { fn into(self: bool) -> felt252 { if self { From 59f17629b5c4d5f26e2ac4806c1e31b1068655a9 Mon Sep 17 00:00:00 2001 From: Andrew Date: Wed, 6 Dec 2023 01:02:33 -0500 Subject: [PATCH 11/26] change src5 migration title --- docs/modules/ROOT/nav.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/modules/ROOT/nav.adoc b/docs/modules/ROOT/nav.adoc index c1e821f1d..3c3e3ac91 100644 --- a/docs/modules/ROOT/nav.adoc +++ b/docs/modules/ROOT/nav.adoc @@ -25,7 +25,7 @@ ** xref:/api/security.adoc[API Reference] * xref:introspection.adoc[Introspection] -** xref:/guides/src5-migration.adoc[Migrate to SRC5] +** xref:/guides/src5-migration.adoc[Migrating ERC165 to SRC5] ** xref:/api/introspection.adoc[API Reference] // * xref:udc.adoc[Universal Deployer Contract] From 44997ed08f11c58a7fc27c53896670d90fe8e349 Mon Sep 17 00:00:00 2001 From: Andrew Date: Wed, 6 Dec 2023 01:03:17 -0500 Subject: [PATCH 12/26] fix src5 migration doc --- .../ROOT/pages/guides/src5-migration.adoc | 106 ++++++++++++++---- 1 file changed, 87 insertions(+), 19 deletions(-) diff --git a/docs/modules/ROOT/pages/guides/src5-migration.adoc b/docs/modules/ROOT/pages/guides/src5-migration.adoc index cd2cc0ee3..5ea723a8b 100644 --- a/docs/modules/ROOT/pages/guides/src5-migration.adoc +++ b/docs/modules/ROOT/pages/guides/src5-migration.adoc @@ -11,26 +11,34 @@ Since the Cairo language evolved introducing native types, we needed an introspe SNIP-5 allows interface ID calculations to use Cairo types and the Starknet keccak (`sn_keccak`) function. For more information on the decision, see the {shamans-proposal} or the {dual-interface-discussion} discussion. -== Migrating to SRC5 +== How to migrate -:src5-component: xref:api/introspection#SRC5Component[SRC5Component] -:upgradeable-component: xref:api/upgrades#UpgradeableComponent[UpgradeableComponent] -:isrc6: xref:api/account.adoc#ISRC6[ISRC6] -:src5-rs: https://github.com/ericnordelo/src5-rs[src5-rs] +The migration process involves integrating SRC5, deregistering ERC165 interface IDs, reregistering SRC5 interface IDs, and exposing a migration initializer to call after the contract upgrade. +The following guide will go through each step with examples. -This guide is for already-deployed, upgradeable contracts that support ERC165. -Migrating to SRC5 requires integrating the {src5-component}. +=== Component integration + +:src5-component: xref:/api/introspection.adoc#SRC5Component[SRC5Component] +:upgradeable-component: xref:/api/upgrades.adoc#UpgradeableComponent[UpgradeableComponent] +:initializable-component: xref:/api/security.adoc#InitializableComponent[InitializableComponent] + +The first step is to integrate the necessary components into the new contract. +The contract should include the new introspection mechanism, {src5-component}. +It should also include the {upgradeable-component} and {initializable-component} which will be used in the migration initializer. +Here's the setup: [,javascript] ---- #[starknet::contract] mod MigratingContract { use openzeppelin::introspection::src5::SRC5Component; + use openzeppelin::security::initializable::InitializableComponent; use openzeppelin::upgrades::UpgradeableComponent; use starknet::ClassHash; use starknet::ContractAddress; component!(path: SRC5Component, storage: src5, event: SRC5Event); + component!(path: InitializableComponent, storage: initializable, event: InitializableEvent); component!(path: UpgradeableComponent, storage: upgradeable, event: UpgradeableEvent); // SRC5 @@ -40,6 +48,9 @@ mod MigratingContract { impl SRC5CamelOnlyImpl = SRC5Component::SRC5CamelImpl; impl SRC5InternalImpl = SRC5Component::InternalImpl; + // Initializable + impl InitializableInternalImpl = InitializableComponent::InternalImpl; + // Upgradeable impl UpgradeableInternalImpl = UpgradeableComponent::InternalImpl; @@ -48,6 +59,8 @@ mod MigratingContract { #[substorage(v0)] src5: SRC5Component::Storage, #[substorage(v0)] + initializable: InitializableComponent::Storage, + #[substorage(v0)] upgradeable: UpgradeableComponent::Storage, } @@ -57,34 +70,89 @@ mod MigratingContract { #[flat] SRC5Event: SRC5Component::Event, #[flat] + InitializableEvent: InitializableComponent::Event, + #[flat] UpgradeableEvent: UpgradeableComponent::Event } - - // The rest of the contract logic } ---- -The contract will also need to register the new SRC5 interface IDs in order to continue declaring support for those interfaces. -We can create a migration initializer to handle this which needs to be called after the contract upgrade. +=== Interface registration + +:ierc721: xref:/api/erc721.adoc#IERC721[IERC721] +:ierc721-metadata: xref:/api/erc721.adoc#IERC721Metadata[IERC721Metadata] +:storage-write-syscall: https://docs.starknet.io/documentation/architecture_and_concepts/Smart_Contracts/system-calls-cairo1/#storage_write[storage_write_syscall] -TIP: OpenZeppelin's Contracts for Cairo already provides the interface IDs of common interfaces in the API section (like {isrc6}). -For interfaces not provided, tools such as {src5-rs} can help with interface ID calculation. +To successfully migrate ERC165 to SRC5, the contract needs to both deregister support for ERC165 interface IDs and reregister support for those interface IDs with SRC5. + +For this example, let's say that this contract supports the {ierc721} and {ierc721-metadata} interfaces. +The contract should implement an `InternalImpl` and add functions to deregister and reregister support for those interfaces like this: [,javascript] ---- #[starknet::contract] mod MigratingContract { + use openzeppelin::utils; + use openzeppelin::token::erc721::interface::{IERC721_ID, IERC721Metadata_ID}; + + (...) + + #[generate_trait] + impl InternalImpl of InternalTrait { + // Delete ERC165 interface support from storage + fn deregister_erc165_interfaces(ref self: ContractState) { + let ERC165_IERC721_ID = 0x80ac58cd; + let ERC165_IERC721_METADATA_ID = 0x5b5e139f; + + utils::deregister_erc165_interface(ERC165_IERC721_ID); + utils::deregister_erc165_interface(ERC165_IERC721_METADATA_ID); + } + + // Reregister SRC5 interfaces + fn register_src5_interfaces(ref self: ContractState) { + self.src5.register_interface(IERC721_ID); + self.src5.register_interface(IERC721Metadata_ID); + } + } +} +---- + +A contract normally includes the storage variable in the `Storage` struct in order to access it. +The new contract, however, will no longer be reading from ERC165's storage address because it's reading from SRC5's storage address. +To avoid adding ERC165's storage variable to the new contract's storage, the contract can utilize `utils::deregister_erc165_interface`. +This utility function uses the {storage-write-syscall} to manually delete the passed `erc165_id` from the ERC165 storage address. + +=== Migration initializer + +:access-control: xref:/access.adoc[Access Control] +Next, the contract should define and expose a migration initializer that will invoke the deregister and reregister functions. +Since the initializer will be publicly callable, it should include some sort of {access-control} so that only permitted addresses can call the function. +Finally, the initializer should include a reinitialization check to ensure that it cannot be called more than once. + +[,javascript] +---- +#[starknet::contract] +mod MigratingContract { (...) #[external(v0)] - fn migration_initializer(ref self: ContractState, interface_ids: Span) { - // Add permissions mechanism - self.src5.register_interfaces(interface_ids); + fn migration_initializer(ref self: ContractState) { + // Add appropriate Access Control mechanism + + // Invoke `initialize` so this function can only be called once + self.initializable.initialize(); + + // Deregister ERC165 IDs + self.deregister_erc165_interfaces(); + + // Reregister SRC5 IDs + self.register_src5_interfaces(); } } ---- -Note that deployed contracts with upgradeability should also be carefully tested before migrating. -Upgradeable contracts will likely include some form of xref:access.adoc[access mechanism] for the upgrade function. -Take extreme care with ensuring admins are not inadvertantly stripped of their permissions. +=== Execute migration + +Once the new contract is prepared for migration and *rigorously tested*, all that's left is to migrate! +Simply upgrade the contract and then call the `migration_initializer`. From d86570f127b257be99046c5e7f5a05a04cd0f95a Mon Sep 17 00:00:00 2001 From: Andrew Date: Wed, 6 Dec 2023 02:12:23 -0500 Subject: [PATCH 13/26] fix how-to section --- docs/modules/ROOT/pages/guides/src5-migration.adoc | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/docs/modules/ROOT/pages/guides/src5-migration.adoc b/docs/modules/ROOT/pages/guides/src5-migration.adoc index 5ea723a8b..bb3b9d629 100644 --- a/docs/modules/ROOT/pages/guides/src5-migration.adoc +++ b/docs/modules/ROOT/pages/guides/src5-migration.adoc @@ -13,8 +13,14 @@ For more information on the decision, see the {shamans-proposal} or the {dual-in == How to migrate -The migration process involves integrating SRC5, deregistering ERC165 interface IDs, reregistering SRC5 interface IDs, and exposing a migration initializer to call after the contract upgrade. -The following guide will go through each step with examples. +Migrating from ERC165 to SRC5 involves four major steps: + +1. Integrate SRC5 into the contract. +2. Create functions to deregister ERC165 IDs and reregister SRC5 IDs. +3. Create a `migration_initializer` to call the registration functions. +4. Upgrade the contract and call `migration_initializer`. + +The following guide will go through the steps with examples. === Component integration @@ -83,7 +89,7 @@ mod MigratingContract { :ierc721-metadata: xref:/api/erc721.adoc#IERC721Metadata[IERC721Metadata] :storage-write-syscall: https://docs.starknet.io/documentation/architecture_and_concepts/Smart_Contracts/system-calls-cairo1/#storage_write[storage_write_syscall] -To successfully migrate ERC165 to SRC5, the contract needs to both deregister support for ERC165 interface IDs and reregister support for those interface IDs with SRC5. +To successfully migrate ERC165 to SRC5, the contract needs to both deregister support for ERC165 interface IDs and reregister support for those interfaces with SRC5. For this example, let's say that this contract supports the {ierc721} and {ierc721-metadata} interfaces. The contract should implement an `InternalImpl` and add functions to deregister and reregister support for those interfaces like this: From 447d495f9d4073be41deb2e3a52894e2c43fea40 Mon Sep 17 00:00:00 2001 From: Andrew Date: Wed, 6 Dec 2023 08:51:16 -0500 Subject: [PATCH 14/26] fix formatting --- src/tests/mocks.cairo | 2 +- src/utils.cairo | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/tests/mocks.cairo b/src/tests/mocks.cairo index f69128bd2..c01981f63 100644 --- a/src/tests/mocks.cairo +++ b/src/tests/mocks.cairo @@ -1,7 +1,7 @@ mod accesscontrol_mocks; mod account_mocks; -mod erc20_mocks; mod erc165_deregister_mock; +mod erc20_mocks; mod erc721_mocks; mod erc721_receiver_mocks; mod initializable_mock; diff --git a/src/utils.cairo b/src/utils.cairo index 05affe71a..cee056a64 100644 --- a/src/utils.cairo +++ b/src/utils.cairo @@ -35,9 +35,8 @@ fn try_selector_with_fallback( fn deregister_erc165_interface(erc165_id: felt252) { let address_domain = 0_u32; let base_address = selector!("ERC165_supported_interfaces"); - let storage_address = storage_address_try_from_felt252( - pedersen(base_address, erc165_id) - ).unwrap(); + let storage_address = storage_address_try_from_felt252(pedersen(base_address, erc165_id)) + .unwrap(); starknet::storage_write_syscall(address_domain, storage_address, 0); } From c85088dc70410c4687bb6c2c9cc378972eb9dd0d Mon Sep 17 00:00:00 2001 From: Andrew Date: Wed, 6 Dec 2023 10:37:22 -0500 Subject: [PATCH 15/26] add deregister_erc165_interface --- docs/modules/ROOT/pages/utilities.adoc | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/docs/modules/ROOT/pages/utilities.adoc b/docs/modules/ROOT/pages/utilities.adoc index ad29ed2a1..466fc777f 100644 --- a/docs/modules/ROOT/pages/utilities.adoc +++ b/docs/modules/ROOT/pages/utilities.adoc @@ -22,6 +22,7 @@ Module containing core utilities of the library. -- .Functions * xref:#utils-try_selector_with_fallback[`++try_selector_with_fallback(target, selector, fallback, args)++`] +* xref:#utils-deregister_erc165_interface[`++deregister_erc165_interface(erc165_id)++`] .Traits * xref:#utils-UnwrapAndCast[`++UnwrapAndCast++`] @@ -58,6 +59,14 @@ error is returned. WARNING: The fallback mechanism won't work on live chains (mainnet or testnets) until they implement panic handling in their runtime. +[.contract-item] +[[utils-deregister_erc165_interface]] +==== `[.contract-item-name]#++deregister_erc165_interface++#++(erc165_id: felt252)++` [.item-kind]#function# + +Manually deletes `erc165_id` value from the storage variable `ERC165_supported_interfaces` in the calling contract's storage. + +This function should only be used by a migration initializer in the ERC165-to-SRC5 migration process. + [#utils-Traits] ==== Traits From 3ac12f2ff88533a0603dcc778980b9ad995b209e Mon Sep 17 00:00:00 2001 From: Andrew Date: Wed, 6 Dec 2023 10:37:55 -0500 Subject: [PATCH 16/26] update interface registration section --- docs/modules/ROOT/pages/guides/src5-migration.adoc | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/docs/modules/ROOT/pages/guides/src5-migration.adoc b/docs/modules/ROOT/pages/guides/src5-migration.adoc index bb3b9d629..cc090b732 100644 --- a/docs/modules/ROOT/pages/guides/src5-migration.adoc +++ b/docs/modules/ROOT/pages/guides/src5-migration.adoc @@ -87,6 +87,7 @@ mod MigratingContract { :ierc721: xref:/api/erc721.adoc#IERC721[IERC721] :ierc721-metadata: xref:/api/erc721.adoc#IERC721Metadata[IERC721Metadata] +:deregister-function: xref:/utilities.adoc#utils-deregister_erc165_interface[deregister_erc165_interface] :storage-write-syscall: https://docs.starknet.io/documentation/architecture_and_concepts/Smart_Contracts/system-calls-cairo1/#storage_write[storage_write_syscall] To successfully migrate ERC165 to SRC5, the contract needs to both deregister support for ERC165 interface IDs and reregister support for those interfaces with SRC5. @@ -123,11 +124,17 @@ mod MigratingContract { } ---- +==== Deregistering IDs + A contract normally includes the storage variable in the `Storage` struct in order to access it. The new contract, however, will no longer be reading from ERC165's storage address because it's reading from SRC5's storage address. -To avoid adding ERC165's storage variable to the new contract's storage, the contract can utilize `utils::deregister_erc165_interface`. +To avoid adding ERC165's storage variable to the new contract's storage, the contract can utilize {deregister-function}. This utility function uses the {storage-write-syscall} to manually delete the passed `erc165_id` from the ERC165 storage address. +==== Reregistering IDs + +Since the new contract integrates `SRC5Component`, it can leverage SRC5's `register_interface` function to reregister supported interfaces. + === Migration initializer :access-control: xref:/access.adoc[Access Control] From 558da38c0de152254fb268af9b1bf86c43e63adc Mon Sep 17 00:00:00 2001 From: Andrew Date: Wed, 6 Dec 2023 10:40:40 -0500 Subject: [PATCH 17/26] add link to register_interface --- docs/modules/ROOT/pages/guides/src5-migration.adoc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/modules/ROOT/pages/guides/src5-migration.adoc b/docs/modules/ROOT/pages/guides/src5-migration.adoc index cc090b732..0f1c02d6a 100644 --- a/docs/modules/ROOT/pages/guides/src5-migration.adoc +++ b/docs/modules/ROOT/pages/guides/src5-migration.adoc @@ -133,7 +133,9 @@ This utility function uses the {storage-write-syscall} to manually delete the pa ==== Reregistering IDs -Since the new contract integrates `SRC5Component`, it can leverage SRC5's `register_interface` function to reregister supported interfaces. +:register_interface: xref:/api/introspection.adoc#SRC5Component-register_interface[register_interface] + +Since the new contract integrates `SRC5Component`, it can leverage SRC5's {register_interface} function to reregister supported interfaces. === Migration initializer From 3cab384c6475e571318d4ad223f30fb05f5c970f Mon Sep 17 00:00:00 2001 From: Andrew Date: Wed, 6 Dec 2023 15:40:16 -0500 Subject: [PATCH 18/26] update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8789aba12..4b75d5c22 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,3 +10,4 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Utilities documentation (#825) +- Documentation for SRC5 migration (#821) From 3929fc2bef757a2ca8c83312b360ed1c276ff9ed Mon Sep 17 00:00:00 2001 From: Andrew Fleming Date: Wed, 6 Dec 2023 20:24:54 -0500 Subject: [PATCH 19/26] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Martín Triay --- .../ROOT/pages/guides/src5-migration.adoc | 16 ++++++++-------- docs/modules/ROOT/pages/utilities.adoc | 4 ++-- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/docs/modules/ROOT/pages/guides/src5-migration.adoc b/docs/modules/ROOT/pages/guides/src5-migration.adoc index 0f1c02d6a..250da1886 100644 --- a/docs/modules/ROOT/pages/guides/src5-migration.adoc +++ b/docs/modules/ROOT/pages/guides/src5-migration.adoc @@ -16,9 +16,9 @@ For more information on the decision, see the {shamans-proposal} or the {dual-in Migrating from ERC165 to SRC5 involves four major steps: 1. Integrate SRC5 into the contract. -2. Create functions to deregister ERC165 IDs and reregister SRC5 IDs. -3. Create a `migration_initializer` to call the registration functions. -4. Upgrade the contract and call `migration_initializer`. +2. Create functions to deregister ERC165 IDs and register SRC5 IDs. +3. Add a `migrate` function to apply introspection changes. +4. Upgrade the contract and call `migrate`. The following guide will go through the steps with examples. @@ -93,7 +93,7 @@ mod MigratingContract { To successfully migrate ERC165 to SRC5, the contract needs to both deregister support for ERC165 interface IDs and reregister support for those interfaces with SRC5. For this example, let's say that this contract supports the {ierc721} and {ierc721-metadata} interfaces. -The contract should implement an `InternalImpl` and add functions to deregister and reregister support for those interfaces like this: +The contract should implement an `InternalImpl` and add functions to deregister and reregister those interfaces like this: [,javascript] ---- @@ -129,9 +129,9 @@ mod MigratingContract { A contract normally includes the storage variable in the `Storage` struct in order to access it. The new contract, however, will no longer be reading from ERC165's storage address because it's reading from SRC5's storage address. To avoid adding ERC165's storage variable to the new contract's storage, the contract can utilize {deregister-function}. -This utility function uses the {storage-write-syscall} to manually delete the passed `erc165_id` from the ERC165 storage address. +This utility function uses the {storage-write-syscall} to delete the registered interface from the ERC165 storage slot. -==== Reregistering IDs +==== Registering IDs :register_interface: xref:/api/introspection.adoc#SRC5Component-register_interface[register_interface] @@ -141,7 +141,7 @@ Since the new contract integrates `SRC5Component`, it can leverage SRC5's {regis :access-control: xref:/access.adoc[Access Control] -Next, the contract should define and expose a migration initializer that will invoke the deregister and reregister functions. +Next, the contract should define and expose a migration function that will invoke the deregister and reregister functions. Since the initializer will be publicly callable, it should include some sort of {access-control} so that only permitted addresses can call the function. Finally, the initializer should include a reinitialization check to ensure that it cannot be called more than once. @@ -153,7 +153,7 @@ mod MigratingContract { #[external(v0)] fn migration_initializer(ref self: ContractState) { - // Add appropriate Access Control mechanism + // WARNING: Missing Access Control mechanism. Make sure to add one. // Invoke `initialize` so this function can only be called once self.initializable.initialize(); diff --git a/docs/modules/ROOT/pages/utilities.adoc b/docs/modules/ROOT/pages/utilities.adoc index 466fc777f..e980eea3b 100644 --- a/docs/modules/ROOT/pages/utilities.adoc +++ b/docs/modules/ROOT/pages/utilities.adoc @@ -63,9 +63,9 @@ they implement panic handling in their runtime. [[utils-deregister_erc165_interface]] ==== `[.contract-item-name]#++deregister_erc165_interface++#++(erc165_id: felt252)++` [.item-kind]#function# -Manually deletes `erc165_id` value from the storage variable `ERC165_supported_interfaces` in the calling contract's storage. +Deletes `erc165_id` value from the storage variable `ERC165_supported_interfaces` in the calling contract's storage. -This function should only be used by a migration initializer in the ERC165-to-SRC5 migration process. +This function is designed to be used in a ERC165-to-SRC5 migration process. [#utils-Traits] ==== Traits From 7a562a2d888fb5ad2fb1b83d0b5b7b8ac2d0b057 Mon Sep 17 00:00:00 2001 From: Andrew Date: Wed, 6 Dec 2023 20:28:23 -0500 Subject: [PATCH 20/26] remove register_erc165_interface --- .../test_erc165_deregister.cairo | 42 ------------------- src/tests/mocks.cairo | 1 - src/tests/mocks/erc165_deregister_mock.cairo | 29 ------------- src/utils.cairo | 37 ---------------- 4 files changed, 109 deletions(-) delete mode 100644 src/tests/introspection/test_erc165_deregister.cairo delete mode 100644 src/tests/mocks/erc165_deregister_mock.cairo diff --git a/src/tests/introspection/test_erc165_deregister.cairo b/src/tests/introspection/test_erc165_deregister.cairo deleted file mode 100644 index 1d69564ec..000000000 --- a/src/tests/introspection/test_erc165_deregister.cairo +++ /dev/null @@ -1,42 +0,0 @@ -use openzeppelin::tests::mocks::erc165_deregister_mock::ERC165DeregisterMock::ERC165DeregisterMockImpl; -use openzeppelin::tests::mocks::erc165_deregister_mock::ERC165DeregisterMock; - -const ID_1: felt252 = 0x12345678; -const ID_2: felt252 = 0x87654321; - -fn STATE() -> ERC165DeregisterMock::ContractState { - ERC165DeregisterMock::contract_state_for_testing() -} - -#[test] -#[available_gas(2000000)] -fn test_deregister_interface() { - let mut state = STATE(); - - state.register_interface(ID_1); - state.register_interface(ID_2); - - assert(state.supports_interface(ID_1), 'Should support ID_1'); - assert(state.supports_interface(ID_2), 'Should support ID_2'); - - state.deregister_erc165_interface(ID_1); - state.deregister_erc165_interface(ID_2); - - assert(!state.supports_interface(ID_1), 'Should not support ID_1'); - assert(!state.supports_interface(ID_2), 'Should not support ID_1'); -} - -#[test] -#[available_gas(2000000)] -fn test_deregister_interface_not_registered() { - let mut state = STATE(); - - assert(!state.supports_interface(ID_1), 'Should not support ID_1'); - assert(!state.supports_interface(ID_2), 'Should not support ID_1'); - - state.deregister_erc165_interface(ID_1); - state.deregister_erc165_interface(ID_2); - - assert(!state.supports_interface(ID_1), 'Should not support ID_1'); - assert(!state.supports_interface(ID_2), 'Should not support ID_1'); -} diff --git a/src/tests/mocks.cairo b/src/tests/mocks.cairo index c01981f63..4bc03de22 100644 --- a/src/tests/mocks.cairo +++ b/src/tests/mocks.cairo @@ -1,6 +1,5 @@ mod accesscontrol_mocks; mod account_mocks; -mod erc165_deregister_mock; mod erc20_mocks; mod erc721_mocks; mod erc721_receiver_mocks; diff --git a/src/tests/mocks/erc165_deregister_mock.cairo b/src/tests/mocks/erc165_deregister_mock.cairo deleted file mode 100644 index 737693912..000000000 --- a/src/tests/mocks/erc165_deregister_mock.cairo +++ /dev/null @@ -1,29 +0,0 @@ -#[starknet::contract] -mod ERC165DeregisterMock { - use openzeppelin::utils; - - #[storage] - struct Storage { - ERC165_supported_interfaces: LegacyMap, - } - - #[event] - #[derive(Drop, starknet::Event)] - enum Event {} - - #[generate_trait] - #[external(v0)] - impl ERC165DeregisterMockImpl of ExternalTrait { - fn supports_interface(self: @ContractState, interface_id: felt252) -> bool { - self.ERC165_supported_interfaces.read(interface_id) - } - - fn register_interface(ref self: ContractState, interface_id: felt252) { - self.ERC165_supported_interfaces.write(interface_id, true); - } - - fn deregister_erc165_interface(ref self: ContractState, interface_id: felt252) { - utils::deregister_erc165_interface(interface_id); - } - } -} diff --git a/src/utils.cairo b/src/utils.cairo index cee056a64..6dda24868 100644 --- a/src/utils.cairo +++ b/src/utils.cairo @@ -5,12 +5,10 @@ mod selectors; mod serde; mod unwrap_and_cast; -use core::pedersen::pedersen; use starknet::ContractAddress; use starknet::SyscallResult; use starknet::SyscallResultTrait; use starknet::call_contract_syscall; -use starknet::storage_address_try_from_felt252; use unwrap_and_cast::UnwrapAndCast; fn try_selector_with_fallback( @@ -27,38 +25,3 @@ fn try_selector_with_fallback( } } } - -/// Manually deletes `erc165_id` value from the storage variable -/// `ERC165_supported_interfaces` in the calling contract's storage. -/// This function should only be used by a migration initializer during the -/// ERC165-to-SRC5 migration process. -fn deregister_erc165_interface(erc165_id: felt252) { - let address_domain = 0_u32; - let base_address = selector!("ERC165_supported_interfaces"); - let storage_address = storage_address_try_from_felt252(pedersen(base_address, erc165_id)) - .unwrap(); - - starknet::storage_write_syscall(address_domain, storage_address, 0); -} - -impl BoolIntoFelt252 of Into { - fn into(self: bool) -> felt252 { - if self { - return 1; - } else { - return 0; - } - } -} - -impl Felt252TryIntoBool of TryInto { - fn try_into(self: felt252) -> Option { - if self == 0 { - Option::Some(false) - } else if self == 1 { - Option::Some(true) - } else { - Option::None(()) - } - } -} From 878996132e02bd029dbb0caafe3006086850ae0f Mon Sep 17 00:00:00 2001 From: Andrew Date: Wed, 6 Dec 2023 20:29:42 -0500 Subject: [PATCH 21/26] remove deregister_erc165_interface from utilities --- docs/modules/ROOT/pages/utilities.adoc | 9 --------- 1 file changed, 9 deletions(-) diff --git a/docs/modules/ROOT/pages/utilities.adoc b/docs/modules/ROOT/pages/utilities.adoc index e980eea3b..ad29ed2a1 100644 --- a/docs/modules/ROOT/pages/utilities.adoc +++ b/docs/modules/ROOT/pages/utilities.adoc @@ -22,7 +22,6 @@ Module containing core utilities of the library. -- .Functions * xref:#utils-try_selector_with_fallback[`++try_selector_with_fallback(target, selector, fallback, args)++`] -* xref:#utils-deregister_erc165_interface[`++deregister_erc165_interface(erc165_id)++`] .Traits * xref:#utils-UnwrapAndCast[`++UnwrapAndCast++`] @@ -59,14 +58,6 @@ error is returned. WARNING: The fallback mechanism won't work on live chains (mainnet or testnets) until they implement panic handling in their runtime. -[.contract-item] -[[utils-deregister_erc165_interface]] -==== `[.contract-item-name]#++deregister_erc165_interface++#++(erc165_id: felt252)++` [.item-kind]#function# - -Deletes `erc165_id` value from the storage variable `ERC165_supported_interfaces` in the calling contract's storage. - -This function is designed to be used in a ERC165-to-SRC5 migration process. - [#utils-Traits] ==== Traits From bfc08694fb5c113f2a510804a225a9a5dcc78e69 Mon Sep 17 00:00:00 2001 From: Andrew Date: Wed, 6 Dec 2023 23:18:09 -0500 Subject: [PATCH 22/26] remove deregister from guide --- .../ROOT/pages/guides/src5-migration.adoc | 68 +++++-------------- 1 file changed, 16 insertions(+), 52 deletions(-) diff --git a/docs/modules/ROOT/pages/guides/src5-migration.adoc b/docs/modules/ROOT/pages/guides/src5-migration.adoc index 250da1886..38700e14e 100644 --- a/docs/modules/ROOT/pages/guides/src5-migration.adoc +++ b/docs/modules/ROOT/pages/guides/src5-migration.adoc @@ -16,7 +16,7 @@ For more information on the decision, see the {shamans-proposal} or the {dual-in Migrating from ERC165 to SRC5 involves four major steps: 1. Integrate SRC5 into the contract. -2. Create functions to deregister ERC165 IDs and register SRC5 IDs. +2. Register SRC5 IDs. 3. Add a `migrate` function to apply introspection changes. 4. Upgrade the contract and call `migrate`. @@ -25,12 +25,11 @@ The following guide will go through the steps with examples. === Component integration :src5-component: xref:/api/introspection.adoc#SRC5Component[SRC5Component] -:upgradeable-component: xref:/api/upgrades.adoc#UpgradeableComponent[UpgradeableComponent] :initializable-component: xref:/api/security.adoc#InitializableComponent[InitializableComponent] The first step is to integrate the necessary components into the new contract. The contract should include the new introspection mechanism, {src5-component}. -It should also include the {upgradeable-component} and {initializable-component} which will be used in the migration initializer. +It should also include the {initializable-component} which will be used in the `migrate` function. Here's the setup: [,javascript] @@ -39,13 +38,9 @@ Here's the setup: mod MigratingContract { use openzeppelin::introspection::src5::SRC5Component; use openzeppelin::security::initializable::InitializableComponent; - use openzeppelin::upgrades::UpgradeableComponent; - use starknet::ClassHash; - use starknet::ContractAddress; component!(path: SRC5Component, storage: src5, event: SRC5Event); component!(path: InitializableComponent, storage: initializable, event: InitializableEvent); - component!(path: UpgradeableComponent, storage: upgradeable, event: UpgradeableEvent); // SRC5 #[abi(embed_v0)] @@ -57,17 +52,12 @@ mod MigratingContract { // Initializable impl InitializableInternalImpl = InitializableComponent::InternalImpl; - // Upgradeable - impl UpgradeableInternalImpl = UpgradeableComponent::InternalImpl; - #[storage] struct Storage { #[substorage(v0)] src5: SRC5Component::Storage, #[substorage(v0)] - initializable: InitializableComponent::Storage, - #[substorage(v0)] - upgradeable: UpgradeableComponent::Storage, + initializable: InitializableComponent::Storage } #[event] @@ -76,9 +66,7 @@ mod MigratingContract { #[flat] SRC5Event: SRC5Component::Event, #[flat] - InitializableEvent: InitializableComponent::Event, - #[flat] - UpgradeableEvent: UpgradeableComponent::Event + InitializableEvent: InitializableComponent::Event } } ---- @@ -87,13 +75,12 @@ mod MigratingContract { :ierc721: xref:/api/erc721.adoc#IERC721[IERC721] :ierc721-metadata: xref:/api/erc721.adoc#IERC721Metadata[IERC721Metadata] -:deregister-function: xref:/utilities.adoc#utils-deregister_erc165_interface[deregister_erc165_interface] -:storage-write-syscall: https://docs.starknet.io/documentation/architecture_and_concepts/Smart_Contracts/system-calls-cairo1/#storage_write[storage_write_syscall] +:register_interface: xref:/api/introspection.adoc#SRC5Component-register_interface[register_interface] -To successfully migrate ERC165 to SRC5, the contract needs to both deregister support for ERC165 interface IDs and reregister support for those interfaces with SRC5. +To successfully migrate ERC165 to SRC5, the contract needs to register the interface IDs that the contract supports with SRC5. For this example, let's say that this contract supports the {ierc721} and {ierc721-metadata} interfaces. -The contract should implement an `InternalImpl` and add functions to deregister and reregister those interfaces like this: +The contract should implement an `InternalImpl` and add a function to register those interfaces like this: [,javascript] ---- @@ -106,16 +93,7 @@ mod MigratingContract { #[generate_trait] impl InternalImpl of InternalTrait { - // Delete ERC165 interface support from storage - fn deregister_erc165_interfaces(ref self: ContractState) { - let ERC165_IERC721_ID = 0x80ac58cd; - let ERC165_IERC721_METADATA_ID = 0x5b5e139f; - - utils::deregister_erc165_interface(ERC165_IERC721_ID); - utils::deregister_erc165_interface(ERC165_IERC721_METADATA_ID); - } - - // Reregister SRC5 interfaces + // Register SRC5 interfaces fn register_src5_interfaces(ref self: ContractState) { self.src5.register_interface(IERC721_ID); self.src5.register_interface(IERC721Metadata_ID); @@ -124,26 +102,15 @@ mod MigratingContract { } ---- -==== Deregistering IDs - -A contract normally includes the storage variable in the `Storage` struct in order to access it. -The new contract, however, will no longer be reading from ERC165's storage address because it's reading from SRC5's storage address. -To avoid adding ERC165's storage variable to the new contract's storage, the contract can utilize {deregister-function}. -This utility function uses the {storage-write-syscall} to delete the registered interface from the ERC165 storage slot. - -==== Registering IDs - -:register_interface: xref:/api/introspection.adoc#SRC5Component-register_interface[register_interface] - -Since the new contract integrates `SRC5Component`, it can leverage SRC5's {register_interface} function to reregister supported interfaces. +Since the new contract integrates `SRC5Component`, it can leverage SRC5's {register_interface} function to register the supported interfaces. === Migration initializer :access-control: xref:/access.adoc[Access Control] -Next, the contract should define and expose a migration function that will invoke the deregister and reregister functions. -Since the initializer will be publicly callable, it should include some sort of {access-control} so that only permitted addresses can call the function. -Finally, the initializer should include a reinitialization check to ensure that it cannot be called more than once. +Next, the contract should define and expose a migration function that will invoke the `register_src5_interfaces` function. +Since the `migrate` function will be publicly callable, it should include some sort of {access-control} so that only permitted addresses can execute the migration. +Finally, `migrate` should include a reinitialization check to ensure that it cannot be called more than once. [,javascript] ---- @@ -152,16 +119,13 @@ mod MigratingContract { (...) #[external(v0)] - fn migration_initializer(ref self: ContractState) { - // WARNING: Missing Access Control mechanism. Make sure to add one. + fn migrate(ref self: ContractState) { + // WARNING: Missing Access Control mechanism. Make sure to add one // Invoke `initialize` so this function can only be called once self.initializable.initialize(); - // Deregister ERC165 IDs - self.deregister_erc165_interfaces(); - - // Reregister SRC5 IDs + // Register SRC5 IDs self.register_src5_interfaces(); } } @@ -170,4 +134,4 @@ mod MigratingContract { === Execute migration Once the new contract is prepared for migration and *rigorously tested*, all that's left is to migrate! -Simply upgrade the contract and then call the `migration_initializer`. +Simply upgrade the contract and then call `migrate`. From b586d0061f1406239263c973093cb01ecbd9f72c Mon Sep 17 00:00:00 2001 From: Andrew Date: Wed, 6 Dec 2023 23:23:57 -0500 Subject: [PATCH 23/26] fix import --- docs/modules/ROOT/pages/guides/src5-migration.adoc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/docs/modules/ROOT/pages/guides/src5-migration.adoc b/docs/modules/ROOT/pages/guides/src5-migration.adoc index 38700e14e..33d9d8b7a 100644 --- a/docs/modules/ROOT/pages/guides/src5-migration.adoc +++ b/docs/modules/ROOT/pages/guides/src5-migration.adoc @@ -86,8 +86,7 @@ The contract should implement an `InternalImpl` and add a function to register t ---- #[starknet::contract] mod MigratingContract { - use openzeppelin::utils; - use openzeppelin::token::erc721::interface::{IERC721_ID, IERC721Metadata_ID}; + use openzeppelin::token::erc721::interface::{IERC721_ID, IERC721_METADATA_ID}; (...) @@ -96,7 +95,7 @@ mod MigratingContract { // Register SRC5 interfaces fn register_src5_interfaces(ref self: ContractState) { self.src5.register_interface(IERC721_ID); - self.src5.register_interface(IERC721Metadata_ID); + self.src5.register_interface(IERC721_METADATA_ID); } } } @@ -125,7 +124,7 @@ mod MigratingContract { // Invoke `initialize` so this function can only be called once self.initializable.initialize(); - // Register SRC5 IDs + // Register SRC5 interfaces self.register_src5_interfaces(); } } From 500d53299d81082fbe4fd6c7c01300bbef2e45b6 Mon Sep 17 00:00:00 2001 From: Andrew Date: Wed, 6 Dec 2023 23:27:44 -0500 Subject: [PATCH 24/26] remove register_interfaces fn from API --- docs/modules/ROOT/pages/api/introspection.adoc | 7 ------- 1 file changed, 7 deletions(-) diff --git a/docs/modules/ROOT/pages/api/introspection.adoc b/docs/modules/ROOT/pages/api/introspection.adoc index 28cf8e514..d5af87f67 100644 --- a/docs/modules/ROOT/pages/api/introspection.adoc +++ b/docs/modules/ROOT/pages/api/introspection.adoc @@ -66,7 +66,6 @@ SRC5 component extending xref:ISRC5[`ISRC5`]. .InternalImpl * xref:#SRC5Component-register_interface[`++register_interface(self, interface_id)++`] -* xref:#SRC5Component-register_interfaces[`++register_interfaces(self, interface_ids)++`] * xref:#SRC5Component-deregister_interface[`++deregister_interface(self, interface_id)++`] -- @@ -88,12 +87,6 @@ See xref:ISRC5-supports_interface[`ISRC5::supports_interface`]. Registers support for the given `interface_id`. -[.contract-item] -[[SRC5Component-register_interfaces]] -==== `[.contract-item-name]#++register_interfaces++#++(ref self: ComponentState, interface_ids: Span)++` [.item-kind]#internal# - -Registers support for the given `interface_ids`. - [.contract-item] [[SRC5Component-deregister_interface]] ==== `[.contract-item-name]#++deregister_interface++#++(ref self: ComponentState, interface_id: felt252)++` [.item-kind]#internal# From 4f6f8498996bd10807dfc460c6f9dd96a02bc16a Mon Sep 17 00:00:00 2001 From: Andrew Date: Wed, 6 Dec 2023 23:29:50 -0500 Subject: [PATCH 25/26] remove unused var --- src/tests/introspection/test_src5.cairo | 1 - 1 file changed, 1 deletion(-) diff --git a/src/tests/introspection/test_src5.cairo b/src/tests/introspection/test_src5.cairo index 437f33dba..db8b53ce3 100644 --- a/src/tests/introspection/test_src5.cairo +++ b/src/tests/introspection/test_src5.cairo @@ -3,7 +3,6 @@ use openzeppelin::introspection::src5::SRC5Component::InternalTrait; use openzeppelin::tests::mocks::src5_mocks::DualCaseSRC5Mock; const OTHER_ID: felt252 = 0x12345678; -const OTHER_ID_2: felt252 = 0x87654321; fn STATE() -> DualCaseSRC5Mock::ContractState { DualCaseSRC5Mock::contract_state_for_testing() From 60983134656b28eee5ebd55d203adc100a5735e3 Mon Sep 17 00:00:00 2001 From: Andrew Date: Fri, 8 Dec 2023 12:20:46 -0500 Subject: [PATCH 26/26] add initializable warning --- docs/modules/ROOT/pages/guides/src5-migration.adoc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/modules/ROOT/pages/guides/src5-migration.adoc b/docs/modules/ROOT/pages/guides/src5-migration.adoc index 33d9d8b7a..e62f197f9 100644 --- a/docs/modules/ROOT/pages/guides/src5-migration.adoc +++ b/docs/modules/ROOT/pages/guides/src5-migration.adoc @@ -111,6 +111,9 @@ Next, the contract should define and expose a migration function that will invok Since the `migrate` function will be publicly callable, it should include some sort of {access-control} so that only permitted addresses can execute the migration. Finally, `migrate` should include a reinitialization check to ensure that it cannot be called more than once. +WARNING: If the original contract implemented `Initializable` at any point and called the `initialize` method, the `InitializableComponent` will not be usable at this time. +Instead, the contract can take inspiration from `InitializableComponent` and create its own initialization mechanism. + [,javascript] ---- #[starknet::contract] @@ -121,7 +124,8 @@ mod MigratingContract { fn migrate(ref self: ContractState) { // WARNING: Missing Access Control mechanism. Make sure to add one - // Invoke `initialize` so this function can only be called once + // WARNING: If the contract ever implemented `Initializable` in the past, + // this will not work. Make sure to create a new initialization mechanism self.initializable.initialize(); // Register SRC5 interfaces