chore: split transaction file part one fields dedicated file #1732

ArniStarkware · 2024-10-31T15:58:07Z

chore: split transaction file part one fields dedicated file

chore: remove public imports from starknet api transaction

reviewable-StarkWare · 2024-10-31T15:58:13Z

This change is

ArniStarkware · 2024-10-31T15:58:24Z

chore: split transaction file part one fields dedicated file #1732 👈
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @ArniStarkware and the rest of your teammates on Graphite

github-actions · 2024-10-31T15:58:39Z

Artifacts upload triggered. View details here

github-actions · 2024-10-31T16:05:09Z

Artifacts upload triggered. View details here

github-actions · 2024-10-31T16:06:21Z

Artifacts upload triggered. View details here

github-actions · 2024-10-31T17:23:21Z

Artifacts upload triggered. View details here

github-actions · 2024-10-31T17:31:54Z

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [33.545 ms 33.582 ms 33.623 ms]
change: [-1.5088% -1.3302% -1.1529%] (p = 0.00 < 0.05)
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
3 (3.00%) high mild
5 (5.00%) high severe

github-actions · 2024-11-01T15:22:41Z

Artifacts upload triggered. View details here

github-actions · 2024-11-01T15:31:07Z

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.153 ms 34.627 ms 35.186 ms]
change: [-5.2610% -3.1550% -1.1393%] (p = 0.00 < 0.05)
Performance has improved.
Found 14 outliers among 100 measurements (14.00%)
3 (3.00%) high mild
11 (11.00%) high severe

codecov · 2024-11-01T15:37:13Z

Codecov Report

Attention: Patch coverage is 68.12227% with 73 lines in your changes missing coverage. Please review.

Project coverage is 62.50%. Comparing base (e3165c4) to head (cdd768f).
Report is 241 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/starknet_api/src/transaction/fields.rs	68.46%	58 Missing and 12 partials ⚠️
crates/native_blockifier/src/py_transaction.rs	0.00%	2 Missing ⚠️
crates/papyrus_rpc/src/v0_8/transaction.rs	80.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1732       +/-   ##
===========================================
+ Coverage   40.10%   62.50%   +22.39%     
===========================================
  Files          26      331      +305     
  Lines        1895    35458    +33563     
  Branches     1895    35458    +33563     
===========================================
+ Hits          760    22163    +21403     
- Misses       1100    11406    +10306     
- Partials       35     1889     +1854

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-11-04T08:27:56Z

Artifacts upload triggered. View details here

github-actions · 2024-11-04T08:36:20Z

Artifacts upload triggered. View details here

github-actions · 2024-11-04T08:46:02Z

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [33.846 ms 33.905 ms 33.967 ms]
change: [-5.9661% -4.4597% -3.1312%] (p = 0.00 < 0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mild

MohammadNassar1

Reviewable status: 0 of 116 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware and @dorimedini-starkware)

crates/starknet_api/src/transaction.rs line 118 at r1 (raw file):

}
}

Please move this struct as well.

crates/starknet_api/src/transaction.rs line 716 at r1 (raw file):

    #[serde(flatten)]
    pub output: TransactionOutput,
}

This as well?

Code quote:

/// A transaction receipt.
#[derive(Debug, Clone, Eq, PartialEq, Deserialize, Serialize)]
pub struct TransactionReceipt {
    pub transaction_hash: TransactionHash,
    pub block_hash: BlockHash,
    pub block_number: BlockNumber,
    #[serde(flatten)]
    pub output: TransactionOutput,
}

crates/starknet_api/src/transaction.rs line 843 at r1 (raw file):

#[derive(
    Debug, Default, Copy, Clone, Eq, PartialEq, Hash, Deserialize, Serialize, PartialOrd, Ord,
)]

these as well?

Code quote:

#[derive(Debug, Default, Clone, Eq, PartialEq, Hash, Deserialize, Serialize, PartialOrd, Ord)]
pub struct MessageToL2 {
    pub from_address: EthAddress,
    pub payload: L1ToL2Payload,
}

/// An L2 to L1 message.
#[derive(Debug, Default, Clone, Eq, PartialEq, Hash, Deserialize, Serialize, PartialOrd, Ord)]
pub struct MessageToL1 {
    pub from_address: ContractAddress,
    pub to_address: EthAddress,
    pub payload: L2ToL1Payload,
}

/// The payload of [`MessageToL2`].
#[derive(Debug, Clone, Default, Eq, PartialEq, Hash, Deserialize, Serialize, PartialOrd, Ord)]
pub struct L1ToL2Payload(pub Vec<Felt>);

/// The payload of [`MessageToL1`].
#[derive(Debug, Clone, Default, Eq, PartialEq, Hash, Deserialize, Serialize, PartialOrd, Ord)]
pub struct L2ToL1Payload(pub Vec<Felt>);

/// An event.
#[derive(Debug, Clone, Default, Eq, PartialEq, Hash, Deserialize, Serialize, PartialOrd, Ord)]
pub struct Event {
    // TODO: Add a TransactionHash element to this struct, and then remove EventLeafElements.
    pub from_address: ContractAddress,
    #[serde(flatten)]
    pub content: EventContent,
}

/// An event content.
#[derive(Debug, Clone, Default, Eq, PartialEq, Hash, Deserialize, Serialize, PartialOrd, Ord)]
pub struct EventContent {
    pub keys: Vec<EventKey>,
    pub data: EventData,
}

/// An event key.
#[derive(Debug, Clone, Default, Eq, PartialEq, Hash, Deserialize, Serialize, PartialOrd, Ord)]
pub struct EventKey(pub Felt);

/// An event data.
#[derive(Debug, Clone, Default, Eq, PartialEq, Hash, Deserialize, Serialize, PartialOrd, Ord)]
pub struct EventData(pub Vec<Felt>);

/// The index of a transaction in [BlockBody](`crate::block::BlockBody`).
#[derive(
    Debug, Default, Copy, Clone, Eq, PartialEq, Hash, Deserialize, Serialize, PartialOrd, Ord,
)]
pub struct TransactionOffsetInBlock(pub usize);

/// The index of an event in [TransactionOutput](`crate::transaction::TransactionOutput`).
#[derive(
    Debug, Default, Copy, Clone, Eq, PartialEq, Hash, Deserialize, Serialize, PartialOrd, Ord,
)]

crates/starknet_api/src/transaction/fields.rs line 1 at r1 (raw file):

use std::collections::BTreeMap;

What do you think about these names? Are they better names?
transaction_components.rs
transaction_structs.rs
transaction_inner.rs

Code quote:

use std::collections::BTreeMap;

ArniStarkware

Reviewable status: 0 of 116 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware and @MohammadNassar1)

crates/starknet_api/src/transaction.rs line 118 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Please move this struct as well.

I thought about it!

Let us not deal with this in this PR - it is big enough as it is.
This struct is used only for transaction hash calculation - as an add on to transaction version. I think this one should be in a different file related to transaction hash.

crates/starknet_api/src/transaction.rs line 716 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

This as well?

This is not a field related to transactions. This is used as an output of the execution.

crates/starknet_api/src/transaction.rs line 843 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

these as well?

About almost all structs mentioned - I suggest creating a file for L1<-> L2 messages in a different PR.

About TransactionOffsetInBlock - I suspect it is closer related to TransactionReceipt - as they are both related to execution output (I think... I am not sure - I am sure these are not fields of a transaction).

crates/starknet_api/src/transaction/fields.rs line 1 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

What do you think about these names? Are they better names?
transaction_components.rs
transaction_structs.rs
transaction_inner.rs

transaction_components.rs is my favorite so far.

ArniStarkware

Reviewable status: 0 of 116 files reviewed, 6 unresolved discussions (waiting on @dorimedini-starkware and @MohammadNassar1)

crates/starknet_api/src/transaction/fields.rs line 16 at r1 (raw file):

// TODO(Noa, 14/11/2023): Replace QUERY_VERSION_BASE_BIT with a lazy calculation.
//      pub static QUERY_VERSION_BASE: Lazy<Felt> = ...
pub const QUERY_VERSION_BASE_BIT: u32 = 128;

Related to tx version and tx hash...

Code quote:

// TODO(Noa, 14/11/2023): Replace QUERY_VERSION_BASE_BIT with a lazy calculation.
//      pub static QUERY_VERSION_BASE: Lazy<Felt> = ...
pub const QUERY_VERSION_BASE_BIT: u32 = 128;

crates/starknet_api/src/transaction/fields.rs line 129 at r1 (raw file):

    /// [TransactionVersion] constant that's equal to 3.
    pub const THREE: Self = { Self(Felt::THREE) };
}

Related to tx version and tx hash...

Code quote:

/// A transaction version.
#[derive(
    Debug,
    Copy,
    Clone,
    Default,
    Eq,
    PartialEq,
    Hash,
    Deserialize,
    Serialize,
    PartialOrd,
    Ord,
    derive_more::Deref,
)]
pub struct TransactionVersion(pub Felt);

impl TransactionVersion {
    /// [TransactionVersion] constant that's equal to 0.
    pub const ZERO: Self = { Self(Felt::ZERO) };

    /// [TransactionVersion] constant that's equal to 1.
    pub const ONE: Self = { Self(Felt::ONE) };

    /// [TransactionVersion] constant that's equal to 2.
    pub const TWO: Self = { Self(Felt::TWO) };

    /// [TransactionVersion] constant that's equal to 3.
    pub const THREE: Self = { Self(Felt::THREE) };
}

MohammadNassar1

Reviewable status: 0 of 116 files reviewed, 6 unresolved discussions (waiting on @ArniStarkware and @dorimedini-starkware)

crates/starknet_api/src/transaction.rs line 118 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

I thought about it!

Let us not deal with this in this PR - it is big enough as it is.

This struct is used only for transaction hash calculation - as an add on to transaction version. I think this one should be in a different file related to transaction hash.

Sounds good.

crates/starknet_api/src/transaction.rs line 716 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

This is not a field related to transactions. This is used as an output of the execution.

Agree.

crates/starknet_api/src/transaction.rs line 843 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

About almost all structs mentioned - I suggest creating a file for L1<-> L2 messages in a different PR.

About TransactionOffsetInBlock - I suspect it is closer related to TransactionReceipt - as they are both related to execution output (I think... I am not sure - I am sure these are not fields of a transaction).

Why do we need a new file for l1 messages?
Why it's different from other fields (e.g. fee).

I agree that TxOffset.. is unrelated.

ArniStarkware

Reviewable status: 0 of 116 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware and @MohammadNassar1)

crates/starknet_api/src/transaction.rs line 118 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Sounds good.

Done.

crates/starknet_api/src/transaction.rs line 716 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Agree.

Done.

crates/starknet_api/src/transaction.rs line 843 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Why do we need a new file for l1 messages?
Why it's different from other fields (e.g. fee).

I agree that TxOffset.. is unrelated.

About something like l1 messages:
I am not an expert about these type of fields which is why:

I did not want to move them to this file if I was not sure they belonged there.
Again - this PR is big enough as it is - let us deal with them in a different PR.

Added a TODO.

crates/starknet_api/src/transaction/fields.rs line 1 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

transaction_components.rs is my favorite so far.

In rust - members of a struct are called fields.
@ShahakShama seems to prefer this name. Other people I asked don't have any input.

github-actions · 2024-11-05T09:04:10Z

Artifacts upload triggered. View details here

github-actions · 2024-11-05T09:04:27Z

Artifacts upload triggered. View details here

github-actions · 2024-11-05T09:15:09Z

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.520 ms 35.003 ms 35.578 ms]
change: [+2.3880% +3.8901% +5.6801%] (p = 0.00 < 0.05)
Performance has regressed.
Found 13 outliers among 100 measurements (13.00%)
4 (4.00%) high mild
9 (9.00%) high severe

full_committer_flow performance regressed!
full_committer_flow time: [29.945 ms 30.004 ms 30.071 ms]
change: [+1.6165% +1.9029% +2.2074%] (p = 0.00 < 0.05)
Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) high mild
3 (3.00%) high severe

github-actions · 2024-11-05T16:36:17Z

Artifacts upload triggered. View details here

github-actions · 2024-11-05T16:45:05Z

Benchmark movements:
full_committer_flow performance regressed!
full_committer_flow time: [29.823 ms 29.866 ms 29.913 ms]
change: [+1.2564% +1.6569% +1.9838%] (p = 0.00 < 0.05)
Performance has regressed.
Found 5 outliers among 100 measurements (5.00%)
2 (2.00%) high mild
3 (3.00%) high severe

github-actions · 2024-11-07T11:40:59Z

Artifacts upload triggered. View details here

github-actions · 2024-11-07T11:48:32Z

Artifacts upload triggered. View details here

ArniStarkware · 2024-11-07T11:50:30Z

crates/starknet_api/src/transaction/fields.rs line 16 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Related to tx version and tx hash...

Now not a part of this PR. In a Future PR we will handle:

TransactionVersion
TransactionHash

ArniStarkware

Reviewable status: 0 of 117 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @MohammadNassar1)

crates/starknet_api/src/transaction/fields.rs line 129 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Related to tx version and tx hash...

Now not a part of this PR. In a future PR we will have a file dedicated to tx version.

github-actions · 2024-11-07T12:01:25Z

Artifacts upload triggered. View details here

github-actions · 2024-11-07T12:11:17Z

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [33.952 ms 34.002 ms 34.055 ms]
change: [-5.1314% -3.4750% -2.0119%] (p = 0.00 < 0.05)
Performance has improved.
Found 11 outliers among 100 measurements (11.00%)
9 (9.00%) high mild
2 (2.00%) high severe

MohammadNassar1

Reviewed 1 of 116 files at r3, 117 of 117 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @dorimedini-starkware)

crates/l1-provider/src/lib.rs line 45 at r4 (raw file):

        todo!(
            "Purges txs from internal buffers, if was proposer clear staging buffer, 
            reset state to Pending until we get proposing/validating notice from consensus."

What's this change?

Code quote:

            "Purges txs from internal buffers, if was proposer clear staging buffer,
            reset state to Pending until we get proposing/validating notice from consensus."

MohammadNassar1

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @dorimedini-starkware)

github-actions · 2024-11-07T17:48:42Z

Artifacts upload triggered. View details here

ArniStarkware

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @dorimedini-starkware)

crates/l1-provider/src/lib.rs line 45 at r4 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

What's this change?

The autoformatter added this change. Reverting.
Out of scope for this PR.

ArniStarkware

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

ArniStarkware requested a review from dorimedini-starkware October 31, 2024 15:58

ArniStarkware requested a review from MohammadNassar1 October 31, 2024 15:58

ArniStarkware marked this pull request as ready for review October 31, 2024 15:58

ArniStarkware force-pushed the arni/snapi/split_file/transaction/fields branch from dcd1085 to e0a8262 Compare October 31, 2024 16:04

ArniStarkware force-pushed the arni/snapi/split_file/transaction/fields branch from e0a8262 to 1474378 Compare October 31, 2024 16:05

ArniStarkware force-pushed the arni/snapi/split_file/transaction/fields branch from 1474378 to 794b619 Compare October 31, 2024 17:21

ArniStarkware force-pushed the arni/snapi/split_file/transaction/fields branch from 794b619 to c415283 Compare November 1, 2024 15:21

ArniStarkware force-pushed the arni/snapi/split_file/transaction/fields branch from c415283 to 795d02e Compare November 4, 2024 08:26

ArniStarkware force-pushed the arni/snapi/split_file/transaction/fields branch from 795d02e to b22dc1a Compare November 4, 2024 08:35

MohammadNassar1 requested changes Nov 4, 2024

View reviewed changes

ArniStarkware requested a review from MohammadNassar1 November 4, 2024 10:54

ArniStarkware commented Nov 4, 2024

View reviewed changes

MohammadNassar1 requested changes Nov 4, 2024

View reviewed changes

ArniStarkware requested a review from MohammadNassar1 November 5, 2024 08:35

ArniStarkware commented Nov 5, 2024

View reviewed changes

ArniStarkware force-pushed the arni/snapi/split_file/transaction/fields branch from b22dc1a to d667cfa Compare November 5, 2024 09:03

ArniStarkware force-pushed the arni/snapi/split_file/transaction/fields branch from d667cfa to 7cc2daf Compare November 5, 2024 09:04

ArniStarkware force-pushed the arni/snapi/split_file/transaction/fields branch from 7cc2daf to 19723b1 Compare November 5, 2024 16:34

ArniStarkware force-pushed the arni/snapi/split_file/transaction/fields branch from 19723b1 to 4e59211 Compare November 7, 2024 11:36

chore: split transaction file part one fields dedicated file

63332e3

ArniStarkware force-pushed the arni/snapi/split_file/transaction/fields branch from 4e59211 to 3da583c Compare November 7, 2024 11:40

ArniStarkware force-pushed the arni/snapi/split_file/transaction/fields branch from 3da583c to 36cfdfe Compare November 7, 2024 11:48

ArniStarkware commented Nov 7, 2024

View reviewed changes

ArniStarkware force-pushed the arni/snapi/split_file/transaction/fields branch from 36cfdfe to bb70300 Compare November 7, 2024 12:00

MohammadNassar1 approved these changes Nov 7, 2024

View reviewed changes

chore: remove public imports from starknet api transaction

cdd768f

ArniStarkware force-pushed the arni/snapi/split_file/transaction/fields branch from bb70300 to cdd768f Compare November 7, 2024 17:48

ArniStarkware commented Nov 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: split transaction file part one fields dedicated file #1732

chore: split transaction file part one fields dedicated file #1732

ArniStarkware commented Oct 31, 2024

reviewable-StarkWare commented Oct 31, 2024

ArniStarkware commented Oct 31, 2024

github-actions bot commented Oct 31, 2024

github-actions bot commented Oct 31, 2024

github-actions bot commented Oct 31, 2024

github-actions bot commented Oct 31, 2024

github-actions bot commented Oct 31, 2024

github-actions bot commented Nov 1, 2024

github-actions bot commented Nov 1, 2024

codecov bot commented Nov 1, 2024 •

edited

Loading

github-actions bot commented Nov 4, 2024

github-actions bot commented Nov 4, 2024

github-actions bot commented Nov 4, 2024

MohammadNassar1 left a comment

ArniStarkware left a comment

ArniStarkware left a comment

MohammadNassar1 left a comment

ArniStarkware left a comment

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 7, 2024

github-actions bot commented Nov 7, 2024

ArniStarkware commented Nov 7, 2024

ArniStarkware left a comment

github-actions bot commented Nov 7, 2024

github-actions bot commented Nov 7, 2024

MohammadNassar1 left a comment

MohammadNassar1 left a comment

github-actions bot commented Nov 7, 2024

ArniStarkware left a comment

ArniStarkware left a comment

chore: split transaction file part one fields dedicated file #1732

Are you sure you want to change the base?

chore: split transaction file part one fields dedicated file #1732

Conversation

ArniStarkware commented Oct 31, 2024

reviewable-StarkWare commented Oct 31, 2024

ArniStarkware commented Oct 31, 2024

github-actions bot commented Oct 31, 2024

github-actions bot commented Oct 31, 2024

github-actions bot commented Oct 31, 2024

github-actions bot commented Oct 31, 2024

github-actions bot commented Oct 31, 2024

github-actions bot commented Nov 1, 2024

github-actions bot commented Nov 1, 2024

codecov bot commented Nov 1, 2024 • edited Loading

Codecov Report

github-actions bot commented Nov 4, 2024

github-actions bot commented Nov 4, 2024

github-actions bot commented Nov 4, 2024

MohammadNassar1 left a comment

Choose a reason for hiding this comment

ArniStarkware left a comment

Choose a reason for hiding this comment

ArniStarkware left a comment

Choose a reason for hiding this comment

MohammadNassar1 left a comment

Choose a reason for hiding this comment

ArniStarkware left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 7, 2024

github-actions bot commented Nov 7, 2024

ArniStarkware commented Nov 7, 2024

ArniStarkware left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 7, 2024

github-actions bot commented Nov 7, 2024

MohammadNassar1 left a comment

Choose a reason for hiding this comment

MohammadNassar1 left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 7, 2024

ArniStarkware left a comment

Choose a reason for hiding this comment

ArniStarkware left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 1, 2024 •

edited

Loading