From 93e9eace0981c550e0e2c10b88c471fecb2923ca Mon Sep 17 00:00:00 2001 From: JoE11-y Date: Mon, 30 Sep 2024 10:44:24 +0100 Subject: [PATCH 1/4] fix: resolve infinite loop in _white_list_collection --- apps/blockchain/starknet/src/bridge.cairo | 1 + .../starknet/src/tests/bridge_t.cairo | 96 +++++++++++++++++++ 2 files changed, 97 insertions(+) diff --git a/apps/blockchain/starknet/src/bridge.cairo b/apps/blockchain/starknet/src/bridge.cairo index 0b734875..2478e2e1 100644 --- a/apps/blockchain/starknet/src/bridge.cairo +++ b/apps/blockchain/starknet/src/bridge.cairo @@ -540,6 +540,7 @@ mod bridge { self.white_listed_list.write(prev, (active, target)); break; } + prev = next; }; self.white_listed_list.write(collection, (false, no_value)); } diff --git a/apps/blockchain/starknet/src/tests/bridge_t.cairo b/apps/blockchain/starknet/src/tests/bridge_t.cairo index cafde030..fa4bfa88 100644 --- a/apps/blockchain/starknet/src/tests/bridge_t.cairo +++ b/apps/blockchain/starknet/src/tests/bridge_t.cairo @@ -831,6 +831,102 @@ mod tests { } + #[test] + fn admin_remove_whitelist_correctly() { + 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 bridge = IStarklaneDispatcher { contract_address: bridge_address }; + + let collection1 = starknet::contract_address_const::<'collection1'>(); + let collection2 = starknet::contract_address_const::<'collection2'>(); + let collection3 = starknet::contract_address_const::<'collection3'>(); + let collection4 = starknet::contract_address_const::<'collection4'>(); + let collection5 = starknet::contract_address_const::<'collection5'>(); + start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); + bridge.enable_white_list(true); + stop_prank(CheatTarget::One(bridge_address)); + + /// add whitelists + let mut spy = spy_events(SpyOn::One(bridge_address)); + start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); + bridge.white_list_collection(collection1, true); + bridge.white_list_collection(collection2, true); + bridge.white_list_collection(collection3, true); + bridge.white_list_collection(collection4, true); + bridge.white_list_collection(collection5, true); + stop_prank(CheatTarget::One(bridge_address)); + + spy.assert_emitted(@array![ + ( + bridge_address, + bridge::Event::CollectionWhiteListUpdated( + bridge::CollectionWhiteListUpdated { + collection: collection1, + enabled: true, + } + ) + ), + ( + bridge_address, + bridge::Event::CollectionWhiteListUpdated( + bridge::CollectionWhiteListUpdated { + collection: collection2, + enabled: true, + } + ) + ), + ( + bridge_address, + bridge::Event::CollectionWhiteListUpdated( + bridge::CollectionWhiteListUpdated { + collection: collection3, + enabled: true, + } + ) + ), + ( + bridge_address, + bridge::Event::CollectionWhiteListUpdated( + bridge::CollectionWhiteListUpdated { + collection: collection4, + enabled: true, + } + ) + ), + ( + bridge_address, + bridge::Event::CollectionWhiteListUpdated( + bridge::CollectionWhiteListUpdated { + collection: collection5, + enabled: true, + } + ) + ) + ]); + + // now try to remove from whitelist collection 3. + // this tests the loop + start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); + bridge.white_list_collection(collection3, false); + stop_prank(CheatTarget::One(bridge_address)); + + spy.assert_emitted(@array![ + ( + bridge_address, + bridge::Event::CollectionWhiteListUpdated( + bridge::CollectionWhiteListUpdated { + collection: collection3, + enabled: false, + } + ) + ), + ]); + } + #[test] #[should_panic] fn deposit_token_not_enabled() { From 69e91dc3d09edb5afbac230b9fc32bfe09bbdadc Mon Sep 17 00:00:00 2001 From: JoE11-y Date: Tue, 1 Oct 2024 12:35:02 +0100 Subject: [PATCH 2/4] update tests --- .../starknet/src/tests/bridge_t.cairo | 184 +++++++----------- 1 file changed, 74 insertions(+), 110 deletions(-) diff --git a/apps/blockchain/starknet/src/tests/bridge_t.cairo b/apps/blockchain/starknet/src/tests/bridge_t.cairo index fa4bfa88..443c7064 100644 --- a/apps/blockchain/starknet/src/tests/bridge_t.cairo +++ b/apps/blockchain/starknet/src/tests/bridge_t.cairo @@ -694,74 +694,85 @@ mod tests { let collection1 = starknet::contract_address_const::<'collection1'>(); let collection2 = starknet::contract_address_const::<'collection2'>(); let collection3 = starknet::contract_address_const::<'collection3'>(); - + let collection4 = starknet::contract_address_const::<'collection4'>(); + let collection5 = starknet::contract_address_const::<'collection5'>(); + + let mut spy = spy_events(SpyOn::One(bridge_address)); start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); bridge.white_list_collection(collection1, true); bridge.white_list_collection(collection2, true); bridge.white_list_collection(collection3, true); + bridge.white_list_collection(collection4, true); + bridge.white_list_collection(collection5, true); stop_prank(CheatTarget::One(bridge_address)); let white_listed = bridge.get_white_listed_collections(); - assert_eq!(white_listed.len(), 3, "White list shall contain 3 elements"); + assert_eq!(white_listed.len(), 5, "White list shall contain 5 elements"); assert_eq!(*white_listed.at(0), collection1, "Wrong collection address in white list"); assert_eq!(*white_listed.at(1), collection2, "Wrong collection address in white list"); assert_eq!(*white_listed.at(2), collection3, "Wrong collection address in white list"); + assert_eq!(*white_listed.at(3), collection4, "Wrong collection address in white list"); + assert_eq!(*white_listed.at(4), collection5, "Wrong collection address in white list"); assert!(bridge.is_white_listed(collection1), "Collection1 should be whitelisted"); assert!(bridge.is_white_listed(collection2), "Collection1 should be whitelisted"); assert!(bridge.is_white_listed(collection3), "Collection1 should be whitelisted"); + assert!(bridge.is_white_listed(collection4), "Collection1 should be whitelisted"); + assert!(bridge.is_white_listed(collection5), "Collection1 should be whitelisted"); start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); - bridge.white_list_collection(collection2, false); + bridge.white_list_collection(collection3, false); stop_prank(CheatTarget::One(bridge_address)); let white_listed = bridge.get_white_listed_collections(); - assert_eq!(white_listed.len(), 2, "White list shall contain 2 elements"); + assert_eq!(white_listed.len(), 4, "White list shall contain 4 elements"); assert_eq!(*white_listed.at(0), collection1, "Wrong collection address in white list"); - assert_eq!(*white_listed.at(1), collection3, "Wrong collection address in white list"); + assert_eq!(*white_listed.at(1), collection2, "Wrong collection address in white list"); + assert_eq!(*white_listed.at(2), collection4, "Wrong collection address in white list"); + assert_eq!(*white_listed.at(3), collection5, "Wrong collection address in white list"); assert!(bridge.is_white_listed(collection1), "Collection1 should be whitelisted"); - assert!(!bridge.is_white_listed(collection2), "Collection1 should not be whitelisted"); - assert!(bridge.is_white_listed(collection3), "Collection1 should be whitelisted"); + assert!(bridge.is_white_listed(collection2), "Collection1 should be whitelisted"); + assert!(!bridge.is_white_listed(collection3), "Collection1 should not be whitelisted"); + assert!(bridge.is_white_listed(collection4), "Collection1 should be whitelisted"); + assert!(bridge.is_white_listed(collection5), "Collection1 should be whitelisted"); start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); bridge.white_list_collection(collection1, false); - bridge.white_list_collection(collection3, false); + bridge.white_list_collection(collection4, false); stop_prank(CheatTarget::One(bridge_address)); - let white_listed = bridge.get_white_listed_collections(); - assert!(white_listed.is_empty(), "White list shall be empty"); + assert_eq!(white_listed.len(), 2, "White list shall contain 2 elements"); + assert_eq!(*white_listed.at(0), collection2, "Wrong collection address in white list"); + assert_eq!(*white_listed.at(1), collection5, "Wrong collection address in white list"); assert!(!bridge.is_white_listed(collection1), "Collection1 should not be whitelisted"); - assert!(!bridge.is_white_listed(collection2), "Collection1 should not be whitelisted"); - assert!(!bridge.is_white_listed(collection3), "Collection1 should not be whitelisted"); + assert!(!bridge.is_white_listed(collection4), "Collection1 should not be whitelisted"); start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); - bridge.white_list_collection(collection1, true); bridge.white_list_collection(collection3, true); + bridge.white_list_collection(collection2, false); stop_prank(CheatTarget::One(bridge_address)); let white_listed = bridge.get_white_listed_collections(); assert_eq!(white_listed.len(), 2, "White list shall contain 2 elements"); - assert_eq!(*white_listed.at(0), collection1, "Wrong collection address in white list"); + assert_eq!(*white_listed.at(0), collection5, "Wrong collection address in white list"); assert_eq!(*white_listed.at(1), collection3, "Wrong collection address in white list"); - assert!(bridge.is_white_listed(collection1), "Collection1 should be whitelisted"); + assert!(bridge.is_white_listed(collection5), "Collection1 should be whitelisted"); assert!(!bridge.is_white_listed(collection2), "Collection1 should not be whitelisted"); assert!(bridge.is_white_listed(collection3), "Collection1 should be whitelisted"); start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); - bridge.white_list_collection(collection1, false); + bridge.white_list_collection(collection3, false); bridge.white_list_collection(collection2, true); stop_prank(CheatTarget::One(bridge_address)); let white_listed = bridge.get_white_listed_collections(); assert_eq!(white_listed.len(), 2, "White list shall contain 2 elements"); - assert_eq!(*white_listed.at(0), collection3, "Wrong collection address in white list"); + assert_eq!(*white_listed.at(0), collection5, "Wrong collection address in white list"); assert_eq!(*white_listed.at(1), collection2, "Wrong collection address in white list"); - assert!(!bridge.is_white_listed(collection1), "Collection1 should not be whitelisted"); + assert!(!bridge.is_white_listed(collection3), "Collection1 should not be whitelisted"); assert!(bridge.is_white_listed(collection2), "Collection1 should be whitelisted"); - assert!(bridge.is_white_listed(collection3), "Collection1 should be whitelisted"); + assert!(bridge.is_white_listed(collection5), "Collection1 should be whitelisted"); start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); + bridge.white_list_collection(collection5, false); bridge.white_list_collection(collection2, false); - bridge.white_list_collection(collection3, false); - bridge.white_list_collection(collection1, false); - bridge.white_list_collection(collection1, false); stop_prank(CheatTarget::One(bridge_address)); let white_listed = bridge.get_white_listed_collections(); @@ -769,6 +780,8 @@ mod tests { assert!(!bridge.is_white_listed(collection1), "Collection1 should not be whitelisted"); assert!(!bridge.is_white_listed(collection2), "Collection1 should not be whitelisted"); assert!(!bridge.is_white_listed(collection3), "Collection1 should not be whitelisted"); + assert!(!bridge.is_white_listed(collection4), "Collection1 should not be whitelisted"); + assert!(!bridge.is_white_listed(collection5), "Collection1 should not be whitelisted"); } #[test] @@ -831,101 +844,52 @@ mod tests { } - #[test] - fn admin_remove_whitelist_correctly() { - let erc721b_contract_class = declare("erc721_bridgeable"); + // #[test] + // fn admin_remove_whitelist_correctly() { + // let erc721b_contract_class = declare("erc721_bridgeable"); - let BRIDGE_ADMIN = starknet::contract_address_const::<'starklane'>(); - let BRIDGE_L1 = EthAddress { address: 'starklane_l1' }; + // 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 bridge = IStarklaneDispatcher { contract_address: bridge_address }; + // let bridge_address = deploy_starklane(BRIDGE_ADMIN, BRIDGE_L1, erc721b_contract_class.class_hash); + // let bridge = IStarklaneDispatcher { contract_address: bridge_address }; - let collection1 = starknet::contract_address_const::<'collection1'>(); - let collection2 = starknet::contract_address_const::<'collection2'>(); - let collection3 = starknet::contract_address_const::<'collection3'>(); - let collection4 = starknet::contract_address_const::<'collection4'>(); - let collection5 = starknet::contract_address_const::<'collection5'>(); - start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); - bridge.enable_white_list(true); - stop_prank(CheatTarget::One(bridge_address)); + // let collection1 = starknet::contract_address_const::<'collection1'>(); + // let collection2 = starknet::contract_address_const::<'collection2'>(); + // let collection3 = starknet::contract_address_const::<'collection3'>(); + // start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); + // bridge.enable_white_list(true); + // stop_prank(CheatTarget::One(bridge_address)); - /// add whitelists - let mut spy = spy_events(SpyOn::One(bridge_address)); - start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); - bridge.white_list_collection(collection1, true); - bridge.white_list_collection(collection2, true); - bridge.white_list_collection(collection3, true); - bridge.white_list_collection(collection4, true); - bridge.white_list_collection(collection5, true); - stop_prank(CheatTarget::One(bridge_address)); + // /// add whitelists + // let mut spy = spy_events(SpyOn::One(bridge_address)); + // start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); + // bridge.white_list_collection(collection1, true); + // bridge.white_list_collection(collection2, true); + // bridge.white_list_collection(collection3, true); + + // stop_prank(CheatTarget::One(bridge_address)); - spy.assert_emitted(@array![ - ( - bridge_address, - bridge::Event::CollectionWhiteListUpdated( - bridge::CollectionWhiteListUpdated { - collection: collection1, - enabled: true, - } - ) - ), - ( - bridge_address, - bridge::Event::CollectionWhiteListUpdated( - bridge::CollectionWhiteListUpdated { - collection: collection2, - enabled: true, - } - ) - ), - ( - bridge_address, - bridge::Event::CollectionWhiteListUpdated( - bridge::CollectionWhiteListUpdated { - collection: collection3, - enabled: true, - } - ) - ), - ( - bridge_address, - bridge::Event::CollectionWhiteListUpdated( - bridge::CollectionWhiteListUpdated { - collection: collection4, - enabled: true, - } - ) - ), - ( - bridge_address, - bridge::Event::CollectionWhiteListUpdated( - bridge::CollectionWhiteListUpdated { - collection: collection5, - enabled: true, - } - ) - ) - ]); - // now try to remove from whitelist collection 3. - // this tests the loop - start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); - bridge.white_list_collection(collection3, false); - stop_prank(CheatTarget::One(bridge_address)); - spy.assert_emitted(@array![ - ( - bridge_address, - bridge::Event::CollectionWhiteListUpdated( - bridge::CollectionWhiteListUpdated { - collection: collection3, - enabled: false, - } - ) - ), - ]); - } + // // now try to remove from whitelist collection 3. + // // this tests the loop + // start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); + // bridge.white_list_collection(collection3, false); + // stop_prank(CheatTarget::One(bridge_address)); + + // spy.assert_emitted(@array![ + // ( + // bridge_address, + // bridge::Event::CollectionWhiteListUpdated( + // bridge::CollectionWhiteListUpdated { + // collection: collection3, + // enabled: false, + // } + // ) + // ), + // ]); + // } #[test] #[should_panic] From 42820989b62b68810177999c27d2d2a619b41153 Mon Sep 17 00:00:00 2001 From: JoE11-y Date: Tue, 1 Oct 2024 16:11:49 +0100 Subject: [PATCH 3/4] fix tests and cleanup --- .../starknet/src/tests/bridge_t.cairo | 102 ++++++++++-------- 1 file changed, 55 insertions(+), 47 deletions(-) diff --git a/apps/blockchain/starknet/src/tests/bridge_t.cairo b/apps/blockchain/starknet/src/tests/bridge_t.cairo index 443c7064..44c91863 100644 --- a/apps/blockchain/starknet/src/tests/bridge_t.cairo +++ b/apps/blockchain/starknet/src/tests/bridge_t.cairo @@ -733,17 +733,51 @@ mod tests { assert!(!bridge.is_white_listed(collection3), "Collection1 should not be whitelisted"); assert!(bridge.is_white_listed(collection4), "Collection1 should be whitelisted"); assert!(bridge.is_white_listed(collection5), "Collection1 should be whitelisted"); + spy.assert_emitted(@array![ + ( + bridge_address, + bridge::Event::CollectionWhiteListUpdated( + bridge::CollectionWhiteListUpdated { + collection: collection3, + enabled: false, + } + ) + ) + ]); start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); bridge.white_list_collection(collection1, false); bridge.white_list_collection(collection4, false); stop_prank(CheatTarget::One(bridge_address)); + + let white_listed = bridge.get_white_listed_collections(); assert_eq!(white_listed.len(), 2, "White list shall contain 2 elements"); assert_eq!(*white_listed.at(0), collection2, "Wrong collection address in white list"); assert_eq!(*white_listed.at(1), collection5, "Wrong collection address in white list"); assert!(!bridge.is_white_listed(collection1), "Collection1 should not be whitelisted"); assert!(!bridge.is_white_listed(collection4), "Collection1 should not be whitelisted"); + spy.assert_emitted(@array![ + ( + bridge_address, + bridge::Event::CollectionWhiteListUpdated( + bridge::CollectionWhiteListUpdated { + collection: collection1, + enabled: false, + } + ) + ), + ( + bridge_address, + bridge::Event::CollectionWhiteListUpdated( + bridge::CollectionWhiteListUpdated { + collection: collection4, + enabled: false, + } + ) + ) + ]); + start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); bridge.white_list_collection(collection3, true); bridge.white_list_collection(collection2, false); @@ -757,6 +791,27 @@ mod tests { assert!(!bridge.is_white_listed(collection2), "Collection1 should not be whitelisted"); assert!(bridge.is_white_listed(collection3), "Collection1 should be whitelisted"); + spy.assert_emitted(@array![ + ( + bridge_address, + bridge::Event::CollectionWhiteListUpdated( + bridge::CollectionWhiteListUpdated { + collection: collection3, + enabled: true, + } + ) + ), + ( + bridge_address, + bridge::Event::CollectionWhiteListUpdated( + bridge::CollectionWhiteListUpdated { + collection: collection2, + enabled: false, + } + ) + ) + ]); + start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); bridge.white_list_collection(collection3, false); bridge.white_list_collection(collection2, true); @@ -844,53 +899,6 @@ mod tests { } - // #[test] - // fn admin_remove_whitelist_correctly() { - // 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 bridge = IStarklaneDispatcher { contract_address: bridge_address }; - - // let collection1 = starknet::contract_address_const::<'collection1'>(); - // let collection2 = starknet::contract_address_const::<'collection2'>(); - // let collection3 = starknet::contract_address_const::<'collection3'>(); - // start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); - // bridge.enable_white_list(true); - // stop_prank(CheatTarget::One(bridge_address)); - - // /// add whitelists - // let mut spy = spy_events(SpyOn::One(bridge_address)); - // start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); - // bridge.white_list_collection(collection1, true); - // bridge.white_list_collection(collection2, true); - // bridge.white_list_collection(collection3, true); - - // stop_prank(CheatTarget::One(bridge_address)); - - - - // // now try to remove from whitelist collection 3. - // // this tests the loop - // start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); - // bridge.white_list_collection(collection3, false); - // stop_prank(CheatTarget::One(bridge_address)); - - // spy.assert_emitted(@array![ - // ( - // bridge_address, - // bridge::Event::CollectionWhiteListUpdated( - // bridge::CollectionWhiteListUpdated { - // collection: collection3, - // enabled: false, - // } - // ) - // ), - // ]); - // } - #[test] #[should_panic] fn deposit_token_not_enabled() { From 32f0935f0a2579cbd40965623065e84e7aba783b Mon Sep 17 00:00:00 2001 From: JoE11-y Date: Wed, 2 Oct 2024 07:51:35 +0100 Subject: [PATCH 4/4] adjust position of event checker --- apps/blockchain/starknet/src/tests/bridge_t.cairo | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/blockchain/starknet/src/tests/bridge_t.cairo b/apps/blockchain/starknet/src/tests/bridge_t.cairo index 44c91863..0cf22a77 100644 --- a/apps/blockchain/starknet/src/tests/bridge_t.cairo +++ b/apps/blockchain/starknet/src/tests/bridge_t.cairo @@ -697,7 +697,6 @@ mod tests { let collection4 = starknet::contract_address_const::<'collection4'>(); let collection5 = starknet::contract_address_const::<'collection5'>(); - let mut spy = spy_events(SpyOn::One(bridge_address)); start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); bridge.white_list_collection(collection1, true); bridge.white_list_collection(collection2, true); @@ -719,6 +718,8 @@ mod tests { assert!(bridge.is_white_listed(collection4), "Collection1 should be whitelisted"); assert!(bridge.is_white_listed(collection5), "Collection1 should be whitelisted"); + let mut spy = spy_events(SpyOn::One(bridge_address)); + start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN); bridge.white_list_collection(collection3, false); stop_prank(CheatTarget::One(bridge_address)); @@ -733,6 +734,7 @@ mod tests { assert!(!bridge.is_white_listed(collection3), "Collection1 should not be whitelisted"); assert!(bridge.is_white_listed(collection4), "Collection1 should be whitelisted"); assert!(bridge.is_white_listed(collection5), "Collection1 should be whitelisted"); + spy.assert_emitted(@array![ ( bridge_address,