Skip to content
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

feat: deterministic contract address calculation #4391

Merged

Conversation

enitrat
Copy link
Contributor

@enitrat enitrat commented Nov 10, 2023

Closes #4375.

The CASM runner now computes addresses for deployed contracts according to the specification available here.

I tested the code with pre-computed values and verified that they match.
I also tested it in "prod" by running our codebase with this updated version of the runner. It correctly matches the expected contract address that we compute in our Cairo programs.

I am not very familiar with how the architecture is done, so I just added the computation functions inside crates/cairo-lang-starknet/src/contract.rs as it seemed like the most appropriate place.
Also, I'm not entirely sure how I should handle error management or if we should just panic when converting the compiler's Felt252 type to the FieldElement type used in pedersen

Looking forward to your review!


This change is Reviewable

@enitrat enitrat force-pushed the feat/deterministic-runner-contract-address branch from b2a9663 to a855f5b Compare November 10, 2023 08:52
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 5 files at r1, all commit messages.
Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions (waiting on @enitrat)


crates/cairo-lang-runner/src/casm_run/mod.rs line 982 at r1 (raw file):

                .starknet_state
                .open_caller_context((deployed_contract_address.clone(), new_caller_address));
            let res = self.call_entry_point(gas_counter, runner, constructor, calldata, vm);

Suggestion:

        // Call constructor if it exists.
        let (res_data_start, res_data_end) = if let Some(constructor) = &contract_info.constructor {
            let old_addrs = self
                .starknet_state
                .open_caller_context((deployed_contract_address.clone(), deployer_address));
            let res = self.call_entry_point(gas_counter, runner, constructor, calldata, vm);

crates/cairo-lang-starknet/src/contract.rs line 70 at r1 (raw file):

/// Computes Pedersen hash using STARK curve on an array of elements, as defined
/// in <https://docs.starknet.io/documentation/architecture_and_concepts/Hashing/hash-functions/#array_hashing.>
pub fn pedersen_hash_array(felts: &[FieldElement]) -> FieldElement {

none of this is required for this crate.
move all these changes to the runner (in a new submodule)

Copy link
Contributor Author

@enitrat enitrat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 9 files reviewed, 2 unresolved discussions (waiting on @orizi)


crates/cairo-lang-runner/src/casm_run/mod.rs line 982 at r1 (raw file):

                .starknet_state
                .open_caller_context((deployed_contract_address.clone(), new_caller_address));
            let res = self.call_entry_point(gas_counter, runner, constructor, calldata, vm);

Done.


crates/cairo-lang-starknet/src/contract.rs line 70 at r1 (raw file):

Previously, orizi wrote…

none of this is required for this crate.
move all these changes to the runner (in a new submodule)

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 9 files at r2, all commit messages.
Reviewable status: 5 of 9 files reviewed, 9 unresolved discussions (waiting on @enitrat)


crates/cairo-lang-runner/Cargo.toml line 31 at r2 (raw file):

thiserror.workspace = true
starknet-crypto = "0.6.1"
hex = "0.4.3"

add both in the main .toml

Suggestion:

starknet-crypto.workspace = true
hex.workspace = true

crates/cairo-lang-runner/src/contract_address.rs line 15 at r2 (raw file):

}

// 2 ** 251 - 256

Suggestion:

/// 2 ** 251 - 256

crates/cairo-lang-runner/src/contract_address.rs line 23 at r2 (raw file):

]);

// Cairo string of "STARKNET_CONTRACT_ADDRESS"

Suggestion:

/// Cairo string of "STARKNET_CONTRACT_ADDRESS"

crates/cairo-lang-runner/src/contract_address.rs line 31 at r2 (raw file):

]);

fn felt252_to_field_element(input: &Felt252) -> anyhow::Result<FieldElement> {

doc


crates/cairo-lang-runner/src/contract_address.rs line 33 at r2 (raw file):

fn felt252_to_field_element(input: &Felt252) -> anyhow::Result<FieldElement> {
    FieldElement::from_bytes_be(&input.to_be_bytes())
        .map_err(|_| anyhow::anyhow!("Failed to convert felt252 to field element."))

This should never fail - unwrap here would be easier to debug if and when it does happen.

Code quote:

map_err(|_| anyhow::anyhow!("Failed to convert felt252 to field element."))

crates/cairo-lang-runner/src/contract_address.rs line 43 at r2 (raw file):

    constructor_calldata: &[Felt252],
    deployer_address: &Felt252,
) -> anyhow::Result<Felt252> {

as above - can't actually fail.

Code quote:

anyhow::Result

crates/cairo-lang-runner/src/lib.rs line 43 at r2 (raw file):

pub mod casm_run;
pub mod contract_address;

make a non-pub submodule of casm_run.


crates/cairo-lang-runner/src/casm_run/test.rs line 446 at r2 (raw file):

#[test]
fn test_calculate_contract_address() {
    let salt = Felt252::from_bytes_be(&hex::decode("65766d5f61646472657373").unwrap());

also elsewhere
(and remove hex dependency)

Suggestion:

felt252_str!("0x65766d5f61646472657373");

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 7 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @enitrat)


crates/cairo-lang-runner/Cargo.toml line 10 at r3 (raw file):

[dependencies]
anyhow.workspace = true

remove.

Copy link
Contributor Author

@enitrat enitrat left a 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 @enitrat)


crates/cairo-lang-runner/Cargo.toml line 31 at r2 (raw file):

Previously, orizi wrote…

add both in the main .toml

Done. (removed hex)


crates/cairo-lang-runner/src/casm_run/test.rs line 446 at r2 (raw file):

Previously, orizi wrote…

also elsewhere
(and remove hex dependency)

Done. (used decimal value)


crates/cairo-lang-runner/src/lib.rs line 43 at r2 (raw file):

Previously, orizi wrote…

make a non-pub submodule of casm_run.

Done.


crates/cairo-lang-runner/src/contract_address.rs line 15 at r2 (raw file):

}

// 2 ** 251 - 256

Done.


crates/cairo-lang-runner/src/contract_address.rs line 23 at r2 (raw file):

]);

// Cairo string of "STARKNET_CONTRACT_ADDRESS"

Done.


crates/cairo-lang-runner/src/contract_address.rs line 31 at r2 (raw file):

Previously, orizi wrote…

doc

Done.


crates/cairo-lang-runner/src/contract_address.rs line 33 at r2 (raw file):

Previously, orizi wrote…

This should never fail - unwrap here would be easier to debug if and when it does happen.

Done.


crates/cairo-lang-runner/src/contract_address.rs line 43 at r2 (raw file):

Previously, orizi wrote…

as above - can't actually fail.

Done.

Copy link
Contributor Author

@enitrat enitrat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 10 files reviewed, 1 unresolved discussion (waiting on @orizi)


crates/cairo-lang-runner/Cargo.toml line 10 at r3 (raw file):

Previously, orizi wrote…

remove.

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @enitrat)

Copy link
Contributor Author

@enitrat enitrat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @enitrat)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @enitrat)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @enitrat)

@enitrat
Copy link
Contributor Author

enitrat commented Nov 17, 2023

(waiting on @enitrat)

I'm not super familiar with Reviewable do I need to do something?

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, will merge on Sunday

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @enitrat)

Copy link
Collaborator

@orizi orizi left a 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 @enitrat)


crates/cairo-lang-runner/src/casm_run/test.rs line 445 at r4 (raw file):

#[test]
fn test_calculate_contract_address() {

where did you bring these values from?
just to know the validation.

Copy link
Contributor Author

@enitrat enitrat left a 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 @orizi)


crates/cairo-lang-runner/src/casm_run/test.rs line 445 at r4 (raw file):

Previously, orizi wrote…

where did you bring these values from?
just to know the validation.

I just used some values I used in my tests in Kakarot. I tested these values against the starknet_api crate and made sure the results were correct.

#[cfg(test)]
mod test {
    use starknet_api::{
        calldata,
        core::{calculate_contract_address, ClassHash, ContractAddress, PatriciaKey},
        hash::{StarkFelt, StarkHash},
        transaction::{Calldata, ContractAddressSalt},
    };
    use starknet_crypto::FieldElement;

    #[test]
    fn test_calc_ca() {
        let salt_felt = StarkFelt::try_from("0x65766d5f61646472657373").unwrap();
        let salt: ContractAddressSalt = ContractAddressSalt(salt_felt);
        let deployer_address_felt = StarkFelt::try_from("1").unwrap();
        let deployer_address: ContractAddress = ContractAddress(PatriciaKey::from(1_u128));
        let class_hash = StarkHash::try_from(
            "0x03ef34708a5a14ee92cfa571a1afdf331aa231d10d3f2f99ff1f8f7516a8c6d2",
        )
        .unwrap();
        let class_hash = ClassHash(class_hash);
        let calldata: Calldata = calldata!(deployer_address_felt, salt_felt);
        let deployed_contract_address =
            calculate_contract_address(salt, class_hash, &calldata, deployer_address).unwrap();

        let expected = ContractAddress(
            PatriciaKey::try_from(
                StarkHash::try_from(
                    "0x050f2821ed90360ac0508d52f8db1f87e541811773bce3dbcaf863c572cd696f",
                )
                .unwrap(),
            )
            .unwrap(),
        );

        assert_eq!(expected, deployed_contract_address);
    }
}

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @enitrat)

@orizi orizi added this pull request to the merge queue Nov 19, 2023
Merged via the queue into starkware-libs:main with commit 2850a16 Nov 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: CASM runner assigns arbitrary contract addresses instead of using correct address computation
2 participants