Fixup of escrow clearing after withdrawing, added tests to check #246
Conversation
|
@od-hunter is attempting to deploy a commit to the Screenshot Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Thanks for your contribution,
it seems that your new test is failing, maybe you forgot to push some modification.
To ensure indentation is correct, you can run scarb fmt.
Also please take a look at https://foundry-rs.github.io/starknet-foundry/testing/testing-contract-internals.html to avoid adding an useless view in contract ABI.
| fn is_token_escrowed(ref self: ContractState, collection: ContractAddress, token_id: u256) -> bool { | ||
| !self.escrow.read((collection, token_id)).is_zero() | ||
| } |
There was a problem hiding this comment.
No need for a ref since there is no state modification
There was a problem hiding this comment.
By the way I'm not sure this view is really needed since it's only use for testing purpose.
Please see https://foundry-rs.github.io/starknet-foundry/testing/testing-contract-internals.html for a better solution.
| @@ -34,6 +34,7 @@ trait IStarklane<T> { | |||
| fn is_enabled(self: @T) -> bool; | |||
|
|
|||
| fn set_l1_l2_collection_mapping(ref self: T , collection_l1: EthAddress, collection_l2: ContractAddress); | |||
| fn is_token_escrowed(ref self: T, collection: ContractAddress, token_id: u256) -> bool; | |||
There was a problem hiding this comment.
No need for ref since there is no state modification
There was a problem hiding this comment.
By the way I'm not sure this view is really needed since it's only use for testing purpose.
Please see https://foundry-rs.github.io/starknet-foundry/testing/testing-contract-internals.html for a better solution.
There was a problem hiding this comment.
hi @ptisserand , i had implemented the view cause the method mentioned in this docs , isnt really working as it should . Something about the mapping being a little more complex to read its value and no clear resource pointing how to go about it
There was a problem hiding this comment.
@ptisserand please reply
| //get mapping value before withdrawal | ||
|
|
| //get mapping value after withdrawal , then compare to see if it has been cleared | ||
|
|
|
|
||
| let null_value = starknet::contract_address_const::<0>(); | ||
| self.escrow.write((collection_l2, token_id), null_value); |
There was a problem hiding this comment.
Use scarb fmt for indentation
| @@ -1214,4 +1215,80 @@ mod tests { | |||
| assert_eq!(bridge.get_l1_collection_address(erc721b_address), collection_l1); | |||
| assert_eq!(bridge.get_l2_collection_address(collection_l1), erc721b_address); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_escrow_clearing_after_withdrawal() { | |||
There was a problem hiding this comment.
This test is failing on your branch
[FAIL] starklane::tests::bridge_t::tests::test_escrow_clearing_after_withdrawal
Failure data:
0x526573756c743a3a756e77726170206661696c65642e ('Result::unwrap failed.')
| @@ -1214,4 +1215,80 @@ mod tests { | |||
| assert_eq!(bridge.get_l1_collection_address(erc721b_address), collection_l1); | |||
| assert_eq!(bridge.get_l2_collection_address(collection_l1), erc721b_address); | |||
| } | |||
|
|
|||
| #[test] | |||
There was a problem hiding this comment.
Use scarb fmt for indentation
|
With starknet-foundry 0.19.0, you can directly query storage with the following: use snforge_std::{load, map_entry_address}; let escrowed = load(bridge_address, map_entry_address(selector!("escrow"), array![erc721b_address.into(), 0, 0].span()), 1);For details see https://book.cairo-lang.org/ch14-01-01-storage-mappings.html and https://docs.starknet.io/architecture-and-concepts/cryptography/hash-functions/#array_hashing |
od-hunter
force-pushed
the
od-hunter-refac
branch
from
253f27f to
1990109
Compare
|
@ptisserand Just did a fix , tests are passing on my end. |
|
@ptisserand , thanks for the opportunity ser🫡 |
Resolves #226