-
Notifications
You must be signed in to change notification settings - Fork 81
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
Add support for declaring contract compiled with cairo 2.6 #1314
Conversation
@pytest.mark.parametrize( | ||
"casm_contract_class_source, expected_casm_class_hash", | ||
[ | ||
("starknet_contract.casm", 0x603dd72504d8b0bc54df4f1102fdcf87fc3b2b94750a9083a5876913eec08e4), |
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.
Taken from Slack conversation with the Staknet team
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.
people will see this link and won't be able to access an info behind it.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #1314 +/- ##
===============================================
- Coverage 98.01% 97.82% -0.20%
===============================================
Files 93 94 +1
Lines 4580 4728 +148
===============================================
+ Hits 4489 4625 +136
- Misses 91 103 +12
|
acc06e4
to
2934b7e
Compare
5722a86
to
866018a
Compare
.github/workflows/checks.yml
Outdated
|
||
- uses: actions/checkout@v4 | ||
with: | ||
repository: starkware-libs/cairo |
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 use scarb?
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.
These changes are moved to the new PR #1322
@pytest.mark.parametrize( | ||
"casm_contract_class_source, expected_casm_class_hash", | ||
[ | ||
("starknet_contract.casm", 0x603dd72504d8b0bc54df4f1102fdcf87fc3b2b94750a9083a5876913eec08e4), |
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.
people will see this link and won't be able to access an info behind it.
MANIFEST_PATH=`cat starknet_py/tests/e2e/manifest-path` | ||
if [ -z "$MANIFEST_PATH" ] | ||
then | ||
echo "File 'manifest-path' was not found in directory 'starknet_py/tests/e2e'. More info here: https://starknetpy.readthedocs.io/en/latest/development.html#setup" |
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.
nitpick: in bash, use 2 spaces instead of 4 for indentation
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.
These changes will be added in #1322
@@ -0,0 +1,58 @@ | |||
#!/bin/bash |
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.
This script has several smaller issues. Did you copy it over from some place, or did you write it from scratch? (I'm wondering if we should just make an issue to fix those, or should this script be fixed in this PR)
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.
it's copied from starknet_py/tests/e2e/mock/compile_contracts_v2.sh
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.
IMO we should make an issue and change this scripts to use scarb
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.
It seems that the only difference between this script and the ones for v1
and v2
are the paths for CONTRACTS_DIRECTORY
and CONTRACTS_COMPILED_DIRECTORY
. I'm wondering if we really need three separate scripts for this. Could you please combine them into a single file?
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.
It will be done in #1322. Probably we will migrate scripts to Scarb
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.
What is this contract? Can we also have the source code in cairo
?
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.
This contract is taken from a conversation with Starknet on Slack. They sent only this contract file and the correct class_hash for it
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.
If this is supposed to be a contract for Cairo v2.6, let's add some specific features from v2.6 here. Otherwise let's use the same contract from the contracts_v2
directory for compilation.
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.
it's cleaning up in #1322, so the mentioned PR aims is to compile contracts_v2 with the current Cairo version and run all tests with it. Extended with precompiled contract tests to verify if class_hashs are computed well for previous Sierra versions
): | ||
declare_tx = await account.sign_declare_v3( | ||
compiled_contract=abi_types_compiled_contract_and_class_hash_v2_6[0], | ||
compiled_class_hash=abi_types_compiled_contract_and_class_hash_v2_6[1], |
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.
As you've noticed this is not a very valuable test, given that in the devnet we can pass anything to compiled_class_hash
and it will be successful. Nevertheless, I'm fine with leaving it, as perhaps it will be fixed at some point. However, instead of adding a new test, please make test_declare_v3
a parameterized test for 2 contracts.
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.
test with starknet_contract.casm
cover this test case
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.
I think we can remove this test and most of the things related to the 2.6 contract. The truth is that we need only starknet_contract.casm
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.
looks good to me, but let's wait for @ddoktorski approval as well
Closes #1315
Introduced changes