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

refactor: move transaction hash calculation to sn api #204

Conversation

MohammadNassar1
Copy link
Contributor

@MohammadNassar1 MohammadNassar1 commented Mar 25, 2024

This change is Reviewable

@MohammadNassar1 MohammadNassar1 force-pushed the mohammad/transaction-hash/move-calculate-transaction-hash branch from a142044 to 1d65e66 Compare March 25, 2024 10:17
Copy link
Collaborator

@yair-starkware yair-starkware 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 r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @giladchase and @MohammadNassar1)


src/transaction_hash.rs line 44 at r1 (raw file):

    static ref ONE: StarkFelt = StarkFelt::from(1_u8);
    static ref TWO: StarkFelt = StarkFelt::from(2_u8);
    static ref THREE: StarkFelt = StarkFelt::from(3_u8);

You can use StarkFelt::ZERO etc.

Code quote:

    pub(crate) static ref ZERO: StarkFelt = StarkFelt::from(0_u8);
    static ref ONE: StarkFelt = StarkFelt::from(1_u8);
    static ref TWO: StarkFelt = StarkFelt::from(2_u8);
    static ref THREE: StarkFelt = StarkFelt::from(3_u8);

src/transaction_hash.rs line 48 at r1 (raw file):

/// Calculates hash of a Starknet transaction.
pub fn get_transaction_hash(

Consider implementing as methods (can be in a separate PR)


src/transaction_hash.rs line 216 at r1 (raw file):

        poseidon_hash_many(&self.elements).into()
    }
}

Consider moving to the crypto module

Code quote:

// Collect elements for applying hash chain.
pub(crate) struct HashChain {
    elements: Vec<FieldElement>,
}

impl HashChain {
    pub fn new() -> HashChain {
        HashChain { elements: Vec::new() }
    }

    // Chains a felt to the hash chain.
    pub fn chain(mut self, felt: &StarkFelt) -> Self {
        self.elements.push(FieldElement::from(*felt));
        self
    }

    // Chains the result of a function to the hash chain.
    pub fn chain_if_fn<F: Fn() -> Option<StarkFelt>>(self, f: F) -> Self {
        match f() {
            Some(felt) => self.chain(&felt),
            None => self,
        }
    }

    // Chains many felts to the hash chain.
    pub fn chain_iter<'a>(self, felts: impl Iterator<Item = &'a StarkFelt>) -> Self {
        felts.fold(self, |current, felt| current.chain(felt))
    }

    // Returns the pedersen hash of the chained felts, hashed with the length of the chain.
    pub fn get_pedersen_hash(&self) -> StarkHash {
        let current_hash = self
            .elements
            .iter()
            .fold(FieldElement::ZERO, |current_hash, felt| pedersen_hash(&current_hash, felt));
        let n_elements = FieldElement::from(self.elements.len());
        pedersen_hash(&current_hash, &n_elements).into()
    }

    // Returns the poseidon hash of the chained felts.
    pub fn get_poseidon_hash(&self) -> StarkHash {
        poseidon_hash_many(&self.elements).into()
    }
}

Copy link
Contributor

@giladchase giladchase left a comment

Choose a reason for hiding this comment

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

@MohammadNassar1,

To clarify, this PR is only extracting code from papyrus into SNAPI, without any logic changes, with a refactor to follow, correct?

Should we wait to review until after the refactor, or would you like initial, non-blocking feedback now, with the understanding that fixes are for subsequent PRs?

Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @MohammadNassar1)

@MohammadNassar1 MohammadNassar1 force-pushed the mohammad/transaction-hash/move-calculate-transaction-hash branch from 1d65e66 to 2e0c20e Compare March 28, 2024 09:43
Copy link
Contributor Author

@MohammadNassar1 MohammadNassar1 left a comment

Choose a reason for hiding this comment

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

Yes, this PR is without any changes.
The next PR is for refactoring. I haven't opened it yet.
Please review it, so we can merge it and use transaction hash calculation in mempool.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @MohammadNassar1)

@MohammadNassar1 MohammadNassar1 force-pushed the mohammad/transaction-hash/move-calculate-transaction-hash branch 2 times, most recently from ea496e7 to 23c3276 Compare March 28, 2024 12:42
Copy link
Contributor Author

@MohammadNassar1 MohammadNassar1 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: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @giladchase and @yair-starkware)


src/transaction_hash.rs line 44 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

You can use StarkFelt::ZERO etc.

Will be done in the next PR.


src/transaction_hash.rs line 48 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

Consider implementing as methods (can be in a separate PR)

Will be done in the next PR.

yair-starkware
yair-starkware previously approved these changes Mar 31, 2024
Copy link
Collaborator

@yair-starkware yair-starkware 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 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @MohammadNassar1)

giladchase
giladchase previously approved these changes Mar 31, 2024
Copy link
Contributor

@giladchase giladchase left a comment

Choose a reason for hiding this comment

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

:lgtm:

:lgtm:

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @MohammadNassar1)

@MohammadNassar1 MohammadNassar1 force-pushed the mohammad/transaction-hash/move-calculate-transaction-hash branch from 23c3276 to cc8b2a6 Compare April 2, 2024 12:18
Copy link
Contributor

@giladchase giladchase left a comment

Choose a reason for hiding this comment

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

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

@MohammadNassar1 MohammadNassar1 merged commit be04271 into main Apr 3, 2024
6 checks passed
@MohammadNassar1 MohammadNassar1 deleted the mohammad/transaction-hash/move-calculate-transaction-hash branch April 3, 2024 11:41
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.

3 participants