-
Notifications
You must be signed in to change notification settings - Fork 65
blockifier_test_utils: add functionality to test contract for test_os_logic #9483
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
blockifier_test_utils: add functionality to test contract for test_os_logic #9483
Conversation
0d96759 to
40cdcc7
Compare
9754472 to
8d45e17
Compare
40cdcc7 to
7ae9c81
Compare
8d45e17 to
18289c2
Compare
7ae9c81 to
f661e8b
Compare
18289c2 to
22ee8e9
Compare
005607e to
015129c
Compare
b1c6cea to
a010edf
Compare
015129c to
da00d3d
Compare
a010edf to
1460e98
Compare
da00d3d to
726ee8b
Compare
1460e98 to
68be87e
Compare
726ee8b to
ade5ad1
Compare
68be87e to
87b8d4e
Compare
ade5ad1 to
7860b39
Compare
87b8d4e to
4fddda3
Compare
AvivYossef-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not adding every util in the PR which you need to use it?
@AvivYossef-starkware reviewed 10 of 10 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @meship-starkware and @Yoni-Starkware)
crates/blockifier_test_utils/resources/feature_contracts/cairo0/test_contract.cairo line 41 at r1 (raw file):
func set_value(address: felt, value: felt) { } }
I done see it in py test contracrt.
Consider create a new file test_contract_interface.cairo like we have in python
Code quote:
@contract_interface
namespace TestContract {
func set_value(address: felt, value: felt) {
}
}crates/blockifier_test_utils/resources/feature_contracts/cairo0/test_contract.cairo line 74 at r1 (raw file):
assert new_value = value + 1; return (); }
I know that its coppied from pyhton but consider giving better name to the function
Code quote:
@external
func entry_point{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*}() {
const address = 15;
let (value) = storage_read(address=address);
storage_write(address=address, value=value + 1);
let (new_value) = storage_read(address=address);
assert new_value = value + 1;
return ();
}crates/blockifier_test_utils/resources/feature_contracts/cairo0/test_contract.cairo line 115 at r1 (raw file):
) { let (diffs: felt*) = alloc(); assert (diffs[0], diffs[1]) = (amount, 0);
does diffs represent the amount of deposit for 2 different tokens?
why we update the balance index is according the l1_address?
Code quote:
assert (diffs[0], diffs[1]) = (amount, 0);
Yoni-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @meship-starkware)
crates/blockifier_test_utils/resources/feature_contracts/cairo0/test_contract.cairo line 116 at r1 (raw file):
let (diffs: felt*) = alloc(); assert (diffs[0], diffs[1]) = (amount, 0); advance_counter(index=from_address, diff_0=amount, diff_1=0);
Suggestion:
advance_counter(index=from_address, diff_0=amount, diff_1=0);4fddda3 to
a6e3a01
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tried to do that for most utils, but adding things to contracts makes the PR large sometimes + involves fixing existing tests
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @meship-starkware, and @Yoni-Starkware)
crates/blockifier_test_utils/resources/feature_contracts/cairo0/test_contract.cairo line 41 at r1 (raw file):
Previously, AvivYossef-starkware wrote…
I done see it in py test contracrt.
Consider create a new filetest_contract_interface.cairolike we have in python
the issue is that in the blockifier we compile each contract as a standalone file; we cannot compile several modules into one JSON
crates/blockifier_test_utils/resources/feature_contracts/cairo0/test_contract.cairo line 115 at r1 (raw file):
Previously, AvivYossef-starkware wrote…
does diffs represent the amount of deposit for 2 different tokens?
why we update the balance index is according the l1_address?
removed the diffs, it wasn't needed
crates/blockifier_test_utils/resources/feature_contracts/cairo0/test_contract.cairo line 116 at r1 (raw file):
let (diffs: felt*) = alloc(); assert (diffs[0], diffs[1]) = (amount, 0); advance_counter(index=from_address, diff_0=amount, diff_1=0);
Done.
|
Benchmark movements: No major performance changes detected. |
AvivYossef-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AvivYossef-starkware reviewed 10 of 10 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @meship-starkware and @Yoni-Starkware)
Yoni-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @meship-starkware)
a6e3a01 to
5309233
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dorimedini-starkware reviewed 10 of 10 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @meship-starkware)

No description provided.