refactor(blockifier): replace WORD_WIDTH w FELT_WIDTH in contract_class #1967

@@             Coverage Diff             @@
##             main    #1967       +/-   ##
===========================================
+ Coverage   40.10%   75.67%   +35.56%     
===========================================
  Files          26      103       +77     
  Lines        1895    13827    +11932     
  Branches     1895    13827    +11932     
===========================================
+ Hits          760    10463     +9703     
- Misses       1100     2909     +1809     
- Partials       35      455      +420

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

github-actions · 2024-11-12T07:32:33Z

Artifacts upload triggered. View details here

ArniStarkware

Reviewed 2 of 3 files at r1.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @avivg-starkware and @Yoni-Starkware)

crates/blockifier/src/fee/eth_gas_constants.rs line 5 at r1 (raw file):

pub const GAS_PER_MEMORY_ZERO_BYTE: usize = 4;
pub const GAS_PER_MEMORY_BYTE: usize = 16;
pub const GAS_PER_MEMORY_WORD: usize = GAS_PER_MEMORY_BYTE * WORD_WIDTH;

Be consistent.
Imports come before everything.

Suggestion:

pub const GAS_PER_MEMORY_ZERO_BYTE: usize = 4;
pub const GAS_PER_MEMORY_BYTE: usize = 16;
pub const GAS_PER_MEMORY_WORD: usize = GAS_PER_MEMORY_BYTE * starknet_api::core::WORD_WIDTH;

github-actions · 2024-11-12T07:58:49Z

Artifacts upload triggered. View details here

avivg-starkware

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @Yoni-Starkware)

crates/blockifier/src/fee/eth_gas_constants.rs line 5 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Be consistent.

Imports come before everything.

1- by consistent, you're referring to the use of "starknet_api::core::WORD_WIDTH" in the other files?
(that is used without 'use' but rather explicitly? )

if so, I did so in this file as well. Otherwise, I'm sorry not sure i understand.

2- I see, I thought comments are excluded.

ArniStarkware

Reviewed 1 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @avivg-starkware and @Yoni-Starkware)

crates/blockifier/src/fee/eth_gas_constants.rs line 5 at r1 (raw file):

Previously, avivg-starkware wrote…

1- by consistent, you're referring to the use of "starknet_api::core::WORD_WIDTH" in the other files?
(that is used without 'use' but rather explicitly? )

if so, I did so in this file as well. Otherwise, I'm sorry not sure i understand.

2- I see, I thought comments are excluded.

Yes - this is what I meant.

a discussion (no related file):
I apologize - because this is contradictory to my latest comment. Please import the const in each file using: starknet_api::core::WORD_WIDTH.

ArniStarkware

Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @avivg-starkware and @Yoni-Starkware)

a discussion (no related file):

Previously, ArniStarkware (Arnon Hod) wrote…

I apologize - because this is contradictory to my latest comment. Please import the const in each file using: starknet_api::core::WORD_WIDTH.

use starknet_api::core::WORD_WIDTH;

github-actions · 2024-11-12T09:08:38Z

Artifacts upload triggered. View details here

avivg-starkware

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @Yoni-Starkware)

a discussion (no related file):

Previously, ArniStarkware (Arnon Hod) wrote…

use starknet_api::core::WORD_WIDTH;

all good, Done :)

ArniStarkware

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yoni-Starkware)

github-actions · 2024-11-12T12:27:43Z

Artifacts upload triggered. View details here

github-actions · 2024-11-12T12:37:15Z

Artifacts upload triggered. View details here

ArniStarkware

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yoni-Starkware)

Yoni-Starkware

Blocking

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

Yoni-Starkware

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

crates/blockifier/src/fee/eth_gas_constants.rs line 1 at r4 (raw file):

use starknet_api::core::WORD_WIDTH;

Remove this const, and instead define FELT_WIDTH = 32 in contract_class.rs
WORD_WIDTH is the size of a word in ethereum.
In code_size, we approximate the code size by adding the max size of each element, which is felt, which is accidentally 32. Felt width could have been different.

Code quote:

use starknet_api::core::WORD_WIDTH;

github-actions · 2024-11-13T08:23:38Z

Artifacts upload triggered. View details here

github-actions · 2024-11-13T08:23:40Z

Artifacts upload triggered. View details here

avivg-starkware

Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @Yoni-Starkware)

crates/blockifier/src/fee/eth_gas_constants.rs line 1 at r4 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Remove this const, and instead define FELT_WIDTH = 32 in contract_class.rs
WORD_WIDTH is the size of a word in ethereum.
In code_size, we approximate the code size by adding the max size of each element, which is felt, which is accidentally 32. Felt width could have been different.

Done (please tell me if I missed something) .
are the comments clear enough?

ArniStarkware

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)

Yoni-Starkware

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

crates/blockifier/src/fee/eth_gas_constants.rs line 1 at r4 (raw file):

Previously, avivg-starkware wrote…

Done (please tell me if I missed something) .
are the comments clear enough?

Move starknet_api::core::WORD_WIDTH back to this file.

Please rename the commit and PR message

github-actions · 2024-11-13T10:03:58Z

Artifacts upload triggered. View details here

github-actions · 2024-11-13T10:06:14Z

Artifacts upload triggered. View details here

avivg-starkware

Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @Yoni-Starkware)

crates/blockifier/src/fee/eth_gas_constants.rs line 1 at r4 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Move starknet_api::core::WORD_WIDTH back to this file.

Please rename the commit and PR message

Done.

github-actions · 2024-11-13T10:07:22Z

Artifacts upload triggered. View details here

github-actions · 2024-11-13T10:09:46Z

Artifacts upload triggered. View details here

github-actions · 2024-11-13T10:10:32Z

Artifacts upload triggered. View details here

ArniStarkware

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)

github-actions · 2024-11-13T10:20:55Z

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.359 ms 34.415 ms 34.504 ms]
change: [-3.8995% -2.3566% -1.0151%] (p = 0.00 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
2 (2.00%) high mild
4 (4.00%) high severe

ArniStarkware

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)

ArniStarkware

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)

Yoni-Starkware

Reviewed 1 of 2 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)

ArniStarkware

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)

avivg-starkware changed the title ~~avivg/blockifier/use_api_WORD_WIDTH_and_remove_blockifier_WORD_WIDTH~~ refactor(blockifier): remove WORD_WIDTH from blockifier, use starkn et_api instead Nov 12, 2024

avivg-starkware marked this pull request as ready for review November 12, 2024 06:41

avivg-starkware force-pushed the avivg/blockifier/remove_WORD_WIDTH_from_blockifier_use_starknet_api_instead branch from cb06e92 to de3c5c0 Compare November 12, 2024 06:52

avivg-starkware requested review from ArniStarkware and Yoni-Starkware November 12, 2024 06:53

avivg-starkware force-pushed the avivg/blockifier/remove_WORD_WIDTH_from_blockifier_use_starknet_api_instead branch from de3c5c0 to 170a607 Compare November 12, 2024 07:32

ArniStarkware requested changes Nov 12, 2024

View reviewed changes

avivg-starkware force-pushed the avivg/blockifier/remove_WORD_WIDTH_from_blockifier_use_starknet_api_instead branch from 170a607 to 4632773 Compare November 12, 2024 07:58

avivg-starkware commented Nov 12, 2024

View reviewed changes

ArniStarkware requested changes Nov 12, 2024

View reviewed changes

avivg-starkware force-pushed the avivg/blockifier/remove_WORD_WIDTH_from_blockifier_use_starknet_api_instead branch from 4632773 to 595fcf0 Compare November 12, 2024 09:08

avivg-starkware commented Nov 12, 2024

View reviewed changes

ArniStarkware approved these changes Nov 12, 2024

View reviewed changes

avivg-starkware force-pushed the avivg/blockifier/remove_WORD_WIDTH_from_blockifier_use_starknet_api_instead branch from 595fcf0 to 0bd4c0d Compare November 12, 2024 12:27

avivg-starkware force-pushed the avivg/blockifier/remove_WORD_WIDTH_from_blockifier_use_starknet_api_instead branch from 0bd4c0d to e759d81 Compare November 12, 2024 12:36

ArniStarkware approved these changes Nov 12, 2024

View reviewed changes

Yoni-Starkware requested changes Nov 13, 2024

View reviewed changes

avivg-starkware force-pushed the avivg/blockifier/remove_WORD_WIDTH_from_blockifier_use_starknet_api_instead branch 2 times, most recently from 30842e9 to 7c22e27 Compare November 13, 2024 08:22

avivg-starkware commented Nov 13, 2024

View reviewed changes

ArniStarkware requested a review from Yoni-Starkware November 13, 2024 08:47

ArniStarkware approved these changes Nov 13, 2024

View reviewed changes

Yoni-Starkware requested changes Nov 13, 2024

View reviewed changes

avivg-starkware changed the title ~~refactor(blockifier): remove WORD_WIDTH from blockifier, use starkn et_api instead~~ refactor(blockifier): replace WORD_WIDTH w FELT_WIDTH in contract_class Nov 13, 2024

avivg-starkware force-pushed the avivg/blockifier/remove_WORD_WIDTH_from_blockifier_use_starknet_api_instead branch 2 times, most recently from d8064e7 to bdf6489 Compare November 13, 2024 10:06

avivg-starkware commented Nov 13, 2024

View reviewed changes

refactor(blockifier): replace WORD_WIDTH w FELT_WIDTH in contract_class

2aa0c56

avivg-starkware changed the title ~~refactor(blockifier): replace WORD_WIDTH w FELT_WIDTH in contract_class~~ refactor(blockifier): replace WORD_WIDTH w FELT_WIDTH in contract_class Nov 13, 2024

avivg-starkware force-pushed the avivg/blockifier/remove_WORD_WIDTH_from_blockifier_use_starknet_api_instead branch from bdf6489 to 2aa0c56 Compare November 13, 2024 10:10

ArniStarkware requested a review from Yoni-Starkware November 13, 2024 10:20

ArniStarkware approved these changes Nov 13, 2024

View reviewed changes

Yoni-Starkware approved these changes Nov 13, 2024

View reviewed changes

ArniStarkware approved these changes Nov 13, 2024

View reviewed changes

avivg-starkware merged commit 6e3cb93 into main Nov 13, 2024
14 checks passed

avivg-starkware deleted the avivg/blockifier/remove_WORD_WIDTH_from_blockifier_use_starknet_api_instead branch November 13, 2024 14:45

github-actions bot locked and limited conversation to collaborators Nov 15, 2024

refactor(blockifier): replace WORD_WIDTH w FELT_WIDTH in contract_class #1967

refactor(blockifier): replace WORD_WIDTH w FELT_WIDTH in contract_class #1967

Conversation

avivg-starkware commented Nov 12, 2024

reviewable-StarkWare commented Nov 12, 2024

avivg-starkware commented Nov 12, 2024

github-actions bot commented Nov 12, 2024

github-actions bot commented Nov 12, 2024

github-actions bot commented Nov 12, 2024

codecov bot commented Nov 12, 2024 • edited Loading

Codecov Report

github-actions bot commented Nov 12, 2024

ArniStarkware left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 12, 2024

avivg-starkware 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

github-actions bot commented Nov 12, 2024

avivg-starkware 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 12, 2024

github-actions bot commented Nov 12, 2024

ArniStarkware left a comment

Choose a reason for hiding this comment

Yoni-Starkware left a comment

Choose a reason for hiding this comment

Yoni-Starkware left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 13, 2024

avivg-starkware left a comment

Choose a reason for hiding this comment

ArniStarkware left a comment

Choose a reason for hiding this comment

Yoni-Starkware left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 13, 2024

avivg-starkware left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 13, 2024

ArniStarkware left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 13, 2024

ArniStarkware left a comment

Choose a reason for hiding this comment

ArniStarkware left a comment

Choose a reason for hiding this comment

Yoni-Starkware 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 12, 2024 •

edited

Loading