diff --git a/apps/blockchain/starknet/Scarb.lock b/apps/blockchain/starknet/Scarb.lock index 12829734..6f84f180 100644 --- a/apps/blockchain/starknet/Scarb.lock +++ b/apps/blockchain/starknet/Scarb.lock @@ -3,8 +3,8 @@ version = 1 [[package]] name = "openzeppelin" -version = "0.10.0" -source = "git+https://github.com/OpenZeppelin/cairo-contracts.git?tag=v0.10.0#d77082732daab2690ba50742ea41080eb23299d3" +version = "0.11.0" +source = "git+https://github.com/OpenZeppelin/cairo-contracts.git?tag=v0.11.0#a83f36b23f1af6e160288962be4a2701c3ecbcda" [[package]] name = "snforge_std" diff --git a/apps/blockchain/starknet/Scarb.toml b/apps/blockchain/starknet/Scarb.toml index 26d58bdb..0d00037b 100644 --- a/apps/blockchain/starknet/Scarb.toml +++ b/apps/blockchain/starknet/Scarb.toml @@ -4,7 +4,7 @@ version = "0.1.0" [dependencies] starknet = "2.6.3" -openzeppelin = { git = "https://github.com/OpenZeppelin/cairo-contracts.git", tag = "v0.10.0" } +openzeppelin = { git = "https://github.com/OpenZeppelin/cairo-contracts.git", tag = "v0.11.0" } [dev-dependencies] snforge_std = { git = "https://github.com/foundry-rs/starknet-foundry.git", tag = "v0.19.0" } diff --git a/apps/blockchain/starknet/src/bridge.cairo b/apps/blockchain/starknet/src/bridge.cairo index 1429e4f0..a578d4e8 100644 --- a/apps/blockchain/starknet/src/bridge.cairo +++ b/apps/blockchain/starknet/src/bridge.cairo @@ -14,7 +14,13 @@ mod bridge { use starknet::contract_address::ContractAddressZeroable; use starknet::eth_address::EthAddressZeroable; - use starklane::interfaces::{IStarklane, IUpgradeable, IUpgradeableDispatcher, IUpgradeableDispatcherTrait, IStarklaneCollectionAdmin}; + use openzeppelin::access::ownable::interface::{ + IOwnableDispatcher, IOwnableDispatcherTrait + }; + + use starklane::interfaces::{IStarklane, + IUpgradeable, IUpgradeableDispatcher, IUpgradeableDispatcherTrait, + IStarklaneCollectionAdmin}; // events use starklane::interfaces::{ DepositRequestInitiated, @@ -31,8 +37,10 @@ mod bridge { collection_type_from_header, }; use starklane::token::{ - interfaces::{IERC721Dispatcher, IERC721DispatcherTrait, - IERC721BridgeableDispatcher, IERC721BridgeableDispatcherTrait}, + interfaces::{ + IERC721Dispatcher, IERC721DispatcherTrait, + IERC721BridgeableDispatcher, IERC721BridgeableDispatcherTrait, + }, collection_manager::{ CollectionType, deploy_erc721_bridgeable, verify_collection_address, erc721_metadata, @@ -161,10 +169,7 @@ mod bridge { #[abi(embed_v0)] impl BridgeUpgradeImpl of IUpgradeable { fn upgrade(ref self: ContractState, class_hash: ClassHash) { - assert( - starknet::get_caller_address() == self.bridge_admin.read(), - 'Unauthorized replace class' - ); + ensure_is_admin(@self); match starknet::replace_class_syscall(class_hash) { Result::Ok(_) => { @@ -326,13 +331,17 @@ mod bridge { #[abi(embed_v0)] impl BridgeCollectionAdminImpl of IStarklaneCollectionAdmin { fn collection_upgrade(ref self: ContractState, collection: ContractAddress, class_hash: ClassHash) { - assert( - starknet::get_caller_address() == self.bridge_admin.read(), - 'Unauthorized replace class' - ); + ensure_is_admin(@self); IUpgradeableDispatcher { contract_address: collection } .upgrade(class_hash); } + + // transfer owner of the given collection to the given address + fn collection_transfer_ownership(ref self: ContractState, collection: ContractAddress, new_owner: ContractAddress) { + ensure_is_admin(@self); + IOwnableDispatcher { contract_address: collection } + .transfer_ownership(new_owner); + } } // *** INTERNALS *** diff --git a/apps/blockchain/starknet/src/interfaces.cairo b/apps/blockchain/starknet/src/interfaces.cairo index b7051e79..d5cb9df0 100644 --- a/apps/blockchain/starknet/src/interfaces.cairo +++ b/apps/blockchain/starknet/src/interfaces.cairo @@ -45,6 +45,9 @@ trait IUpgradeable { trait IStarklaneCollectionAdmin { // try to upgrade the given collection with given class_hash fn collection_upgrade(ref self: T, collection: ContractAddress, class_hash: ClassHash); + + // transfer owner of the given collection to the given address + fn collection_transfer_ownership(ref self: T, collection: ContractAddress, new_owner: ContractAddress); } ////////////////////////// diff --git a/apps/blockchain/starknet/src/tests/bridge_t.cairo b/apps/blockchain/starknet/src/tests/bridge_t.cairo index 48f71763..a43db473 100644 --- a/apps/blockchain/starknet/src/tests/bridge_t.cairo +++ b/apps/blockchain/starknet/src/tests/bridge_t.cairo @@ -16,6 +16,11 @@ mod tests { use option::OptionTrait; use serde::Serde; use zeroable::Zeroable; + + use openzeppelin::access::ownable::interface::{ + IOwnableTwoStepDispatcher, IOwnableTwoStepDispatcherTrait + }; + use starknet::{ContractAddress, ClassHash, EthAddress}; use starklane::{ request::{Request, compute_request_hash}, @@ -35,6 +40,14 @@ mod tests { use snforge_std::{declare, ContractClass, ContractClassTrait, start_prank, stop_prank, CheatTarget, L1Handler, get_class_hash, spy_events, SpyOn}; + #[derive(Drop)] + struct BridgeDeployedConfig { + admin: ContractAddress, + bridge: ContractAddress, + erc721: ContractAddress, + erc721_class: ClassHash, + } + /// Deploys Starklane. fn deploy_starklane( bridge_admin: ContractAddress, @@ -76,6 +89,60 @@ mod tests { erc721b_contract_class.deploy(@calldata).unwrap() } + /// deploy bridge and ERC721Bridgeable + fn deploy_bridge_and_create_collection( + bridge_l1: EthAddress, + collection_l1: EthAddress, + owner_l1: EthAddress, + owner_l2: ContractAddress, + ids: Span, + ) -> BridgeDeployedConfig { + let erc721b_contract_class = declare("erc721_bridgeable"); + + let BRIDGE_ADMIN = starknet::contract_address_const::<'starklane'>(); + let BRIDGE_L1 = EthAddress { address: 'starklane_l1' }; + + let bridge_address = deploy_starklane(BRIDGE_ADMIN, BRIDGE_L1, erc721b_contract_class.class_hash); + + let collection_l2: ContractAddress = 0x0.try_into().unwrap(); + + let mut req = setup_request( + collection_l1, + collection_l2, + owner_l1, + owner_l2, + "name", + "symbol", + "base_uri" + ); + req.ids = ids; + let mut buf = array![]; + req.serialize(ref buf); + + let mut l1_handler = L1Handler { + contract_address: bridge_address, + // selector: 0x03593216f3a8b22f4cf375e5486e3d13bfde9d0f26976d20ac6f653c73f7e507, + function_selector: selector!("withdraw_auto_from_l1"), + from_address: bridge_l1.into(), + payload: buf.span() + }; + + l1_handler.execute().unwrap(); + let bridge = IStarklaneDispatcher { contract_address: bridge_address }; + + // Deserialize the request and check some expected values. + let mut sp = buf.span(); + let req = Serde::::deserialize(ref sp).unwrap(); + + let deployed_address = bridge.get_l2_collection_address(req.collection_l1.into()); + BridgeDeployedConfig { + admin: BRIDGE_ADMIN, + bridge: bridge_address, + erc721: deployed_address, + erc721_class: erc721b_contract_class.class_hash, + } + } + fn mint_range( address: ContractAddress, collection_owner: ContractAddress, @@ -659,57 +726,92 @@ mod tests { #[test] fn collection_upgrade_as_admin() { - // Need to declare here to get the class hash before deploy anything. - let erc721b_contract_class = declare("erc721_bridgeable"); - - let BRIDGE_ADMIN = starknet::contract_address_const::<'starklane'>(); let BRIDGE_L1 = EthAddress { address: 'starklane_l1' }; + let OWNER_L1: EthAddress = 0xe00.try_into().unwrap(); let OWNER_L2 = starknet::contract_address_const::<'owner_l2'>(); - - let bridge_address = deploy_starklane(BRIDGE_ADMIN, BRIDGE_L1, erc721b_contract_class.class_hash); - - let collection_l1: EthAddress = 0xe0c.try_into().unwrap(); - let collection_l2: ContractAddress = 0x0.try_into().unwrap(); - let owner_l1: EthAddress = 0xe00.try_into().unwrap(); - let owner_l2: ContractAddress = OWNER_L2.into(); + let COLLECTION_L1: EthAddress = 0xe0c.try_into().unwrap(); let ids: Span = array![0_u256, 1_u256].span(); - let mut req = setup_request( - collection_l1, - collection_l2, - owner_l1, - owner_l2, - "name", - "symbol", - "base_uri" - ); - req.ids = ids; - let mut buf = array![]; - req.serialize(ref buf); - - let mut l1_handler = L1Handler { - contract_address: bridge_address, - // selector: 0x03593216f3a8b22f4cf375e5486e3d13bfde9d0f26976d20ac6f653c73f7e507, - function_selector: selector!("withdraw_auto_from_l1"), - from_address: BRIDGE_L1.into(), - payload: buf.span() - }; - let mut spy = spy_events(SpyOn::One(bridge_address)); + let bridge_deployed_config = deploy_bridge_and_create_collection(BRIDGE_L1, + COLLECTION_L1, + OWNER_L1, + OWNER_L2, + ids); + let BRIDGE_ADMIN = bridge_deployed_config.admin; + let bridge_address = bridge_deployed_config.bridge; + let erc721b_class = bridge_deployed_config.erc721_class; - l1_handler.execute().unwrap(); let bridge = IStarklaneDispatcher { contract_address: bridge_address }; - // Deserialize the request and check some expected values. - let mut sp = buf.span(); - let req = Serde::::deserialize(ref sp).unwrap(); - - let deployed_address = bridge.get_l2_collection_address(req.collection_l1.into()); + let deployed_address = bridge.get_l2_collection_address(COLLECTION_L1.into()); assert!(!deployed_address.is_zero(), "Expected deployed erc721"); - assert!(get_class_hash(deployed_address) == erc721b_contract_class.class_hash, "Expected class hash"); + assert!(get_class_hash(deployed_address) == erc721b_class, "Expected class hash"); start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); IStarklaneCollectionAdminDispatcher { contract_address: bridge_address}.collection_upgrade(deployed_address, get_class_hash(bridge_address)); stop_prank(CheatTarget::One(bridge_address)); assert!(get_class_hash(deployed_address) == get_class_hash(bridge_address), "Class hash not updated"); } + + + #[test] + fn collection_transfer_ownership_as_admin() { + let BRIDGE_L1 = EthAddress { address: 'starklane_l1' }; + let OWNER_L1: EthAddress = 0xe00.try_into().unwrap(); + let OWNER_L2 = starknet::contract_address_const::<'owner_l2'>(); + let COLLECTION_L1: EthAddress = 0xe0c.try_into().unwrap(); + let ids: Span = array![0_u256, 1_u256].span(); + + let bridge_deployed_config = deploy_bridge_and_create_collection(BRIDGE_L1, + COLLECTION_L1, + OWNER_L1, + OWNER_L2, + ids); + let BRIDGE_ADMIN = bridge_deployed_config.admin; + let bridge_address = bridge_deployed_config.bridge; + let erc721b = bridge_deployed_config.erc721; + + let ownable = IOwnableTwoStepDispatcher { contract_address: erc721b }; + assert_eq!(ownable.owner(), bridge_address, "Wrong owner"); + + let ALICE = starknet::contract_address_const::<'alice'>(); + + start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); + IStarklaneCollectionAdminDispatcher { contract_address: bridge_address}.collection_transfer_ownership(erc721b, ALICE); + stop_prank(CheatTarget::One(bridge_address)); + + assert_eq!(ownable.owner(), bridge_address, "Wrong owner"); + assert_eq!(ownable.pending_owner(), ALICE, "Wrong pending owner"); + + start_prank(CheatTarget::One(erc721b), ALICE); + ownable.accept_ownership(); + stop_prank(CheatTarget::One(erc721b)); + assert_eq!(ownable.owner(), ALICE, "Wrong owner"); + } + + #[test] + #[should_panic(expected: ('Unauthorized call',))] + fn collection_transfer_ownership_as_not_admin() { + let BRIDGE_L1 = EthAddress { address: 'starklane_l1' }; + let OWNER_L1: EthAddress = 0xe00.try_into().unwrap(); + let OWNER_L2 = starknet::contract_address_const::<'owner_l2'>(); + let COLLECTION_L1: EthAddress = 0xe0c.try_into().unwrap(); + let ids: Span = array![0_u256, 1_u256].span(); + + let bridge_deployed_config = deploy_bridge_and_create_collection(BRIDGE_L1, + COLLECTION_L1, + OWNER_L1, + OWNER_L2, + ids); + let bridge_address = bridge_deployed_config.bridge; + let erc721b = bridge_deployed_config.erc721; + + + let ALICE = starknet::contract_address_const::<'alice'>(); + + start_prank(CheatTarget::One(bridge_address), OWNER_L2); + IStarklaneCollectionAdminDispatcher { contract_address: bridge_address}.collection_transfer_ownership(erc721b, ALICE); + stop_prank(CheatTarget::One(bridge_address)); + } + } diff --git a/apps/blockchain/starknet/src/token/erc721_bridgeable.cairo b/apps/blockchain/starknet/src/token/erc721_bridgeable.cairo index 1ea0f01a..b6938697 100644 --- a/apps/blockchain/starknet/src/token/erc721_bridgeable.cairo +++ b/apps/blockchain/starknet/src/token/erc721_bridgeable.cairo @@ -7,12 +7,14 @@ mod erc721_bridgeable { use openzeppelin::introspection::src5::SRC5Component; use openzeppelin::token::erc721::ERC721Component; + use openzeppelin::access::ownable::OwnableComponent; use starklane::token::interfaces::{IERC721Bridgeable, IERC721Mintable}; use starklane::interfaces::IUpgradeable; component!(path: ERC721Component, storage: erc721, event: ERC721Event); component!(path: SRC5Component, storage: src5, event: SRC5Event); + component!(path: OwnableComponent, storage: ownable, event: OwnableEvent); // ERC721Mixin can't be used since we have a custom implementation for Metadata #[abi(embed_v0)] @@ -26,16 +28,22 @@ mod erc721_bridgeable { #[abi(embed_v0)] impl SRC5Impl = SRC5Component::SRC5Impl; + #[abi(embed_v0)] + impl OwnableTwoStepMixinImpl = OwnableComponent::OwnableTwoStepMixinImpl; + + impl OwnableInternalImpl = OwnableComponent::InternalImpl; + #[storage] struct Storage { bridge: ContractAddress, - collection_owner: ContractAddress, /// token_uris is required if we want uris not derivated from base_uri token_uris: LegacyMap, #[substorage(v0)] erc721: ERC721Component::Storage, #[substorage(v0)] - src5: SRC5Component::Storage + src5: SRC5Component::Storage, + #[substorage(v0)] + ownable: OwnableComponent::Storage, } #[event] @@ -44,7 +52,9 @@ mod erc721_bridgeable { #[flat] ERC721Event: ERC721Component::Event, #[flat] - SRC5Event: SRC5Component::Event + SRC5Event: SRC5Component::Event, + #[flat] + OwnableEvent: OwnableComponent::Event, } #[constructor] @@ -61,7 +71,7 @@ mod erc721_bridgeable { self.erc721.initializer(name, symbol, base_uri); self.bridge.write(bridge); - self.collection_owner.write(collection_owner); + self.ownable.initializer(collection_owner); } @@ -112,10 +122,7 @@ mod erc721_bridgeable { #[abi(embed_v0)] impl ERC721BridgeableUpgradeImpl of IUpgradeable { fn upgrade(ref self: ContractState, class_hash: ClassHash) { - assert( - starknet::get_caller_address() == self.collection_owner.read(), - 'Unauthorized replace class' - ); + self.ownable.assert_only_owner(); match starknet::replace_class_syscall(class_hash) { Result::Ok(_) => (), // emit event @@ -127,10 +134,7 @@ mod erc721_bridgeable { #[abi(embed_v0)] impl ERC721BridgeableMintableImpl of IERC721Mintable { fn mint(ref self: ContractState, to: ContractAddress, token_id: u256) { - assert( - starknet::get_caller_address() == self.collection_owner.read(), - 'ERC721: only col owner can mint' - ); + self.ownable.assert_only_owner(); self.erc721._mint(to, token_id); } @@ -156,6 +160,10 @@ mod erc721_bridgeable { mod tests { use super::erc721_bridgeable; + use openzeppelin::access::ownable::interface::{ + IOwnableTwoStepDispatcher, IOwnableTwoStepDispatcherTrait + }; + use starklane::token::interfaces::{ IERC721BridgeableDispatcher, IERC721BridgeableDispatcherTrait, IERC721Dispatcher, IERC721DispatcherTrait, @@ -325,4 +333,73 @@ mod tests { .expect('token mint failed'); assert_eq!(fetched_uri, new_uri, "bad uri"); } + + #[test] + fn support_two_step_transfer_ownership() { + let COLLECTION_OWNER = collection_owner_addr_mock(); + let ALICE = starknet::contract_address_const::<'alice'>(); + + let contract_address = deploy_everai_collection(); + + let ownable = IOwnableTwoStepDispatcher { contract_address }; + assert_eq!(ownable.owner(), COLLECTION_OWNER, "bad owner"); + + start_prank(CheatTarget::One(contract_address), COLLECTION_OWNER); + ownable.transfer_ownership(ALICE); + stop_prank(CheatTarget::One(contract_address)); + assert_eq!(ownable.owner(), COLLECTION_OWNER, "bad owner"); + + start_prank(CheatTarget::One(contract_address), ALICE); + ownable.accept_ownership(); + stop_prank(CheatTarget::One(contract_address)); + assert_eq!(ownable.owner(), ALICE, "bad owner"); + } + + #[test] + fn two_step_transfer_ownership() { + let COLLECTION_OWNER = collection_owner_addr_mock(); + let ALICE = starknet::contract_address_const::<'alice'>(); + let BOB = starknet::contract_address_const::<'bob'>(); + + let contract_address = deploy_everai_collection(); + + let ownable = IOwnableTwoStepDispatcher { contract_address }; + assert_eq!(ownable.owner(), COLLECTION_OWNER, "bad owner"); + + start_prank(CheatTarget::One(contract_address), COLLECTION_OWNER); + ownable.transfer_ownership(ALICE); + stop_prank(CheatTarget::One(contract_address)); + assert_eq!(ownable.owner(), COLLECTION_OWNER, "bad owner"); + assert_eq!(ownable.pending_owner(), ALICE, "bad pending owner"); + + start_prank(CheatTarget::One(contract_address), COLLECTION_OWNER); + ownable.transfer_ownership(BOB); + stop_prank(CheatTarget::One(contract_address)); + assert_eq!(ownable.owner(), COLLECTION_OWNER, "bad owner"); + assert_eq!(ownable.pending_owner(), BOB, "bad pending owner"); + + start_prank(CheatTarget::One(contract_address), BOB); + ownable.accept_ownership(); + stop_prank(CheatTarget::One(contract_address)); + + assert_eq!(ownable.owner(), BOB, "bad owner"); + } + + + #[test] + #[should_panic(expected: ('Caller is not the owner',))] + fn should_panic_transfer_not_owner() { + let COLLECTION_OWNER = collection_owner_addr_mock(); + let ALICE = starknet::contract_address_const::<'alice'>(); + let BOB = starknet::contract_address_const::<'bob'>(); + + let contract_address = deploy_everai_collection(); + + let ownable = IOwnableTwoStepDispatcher { contract_address }; + assert_eq!(ownable.owner(), COLLECTION_OWNER, "bad owner"); + + start_prank(CheatTarget::One(contract_address), ALICE); + ownable.transfer_ownership(BOB); + stop_prank(CheatTarget::One(contract_address)); + } }