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: concat_counts into a single felt #247

Merged
merged 1 commit into from
May 9, 2024

Conversation

yoavGrs
Copy link
Contributor

@yoavGrs yoavGrs commented May 9, 2024

This change is Reviewable

@yoavGrs yoavGrs requested a review from ShahakShama May 9, 2024 08:29
@yoavGrs yoavGrs self-assigned this May 9, 2024
Copy link
Contributor

@ShahakShama ShahakShama 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 @yoavGrs)


src/block_hash/block_hash_calculator.rs line 1 at r1 (raw file):

use crate::data_availability::L1DataAvailabilityMode;

Rename this file to util


src/block_hash/block_hash_calculator.rs line 37 at r1 (raw file):

fn to_64_bits(num: usize, error_message: String) -> Result<[u8; 8], StarknetApiError> {
    let sized_transaction_count: u64 =
        num.try_into().map_err(|_| StarknetApiError::OutOfRange { string: error_message })?;

usize to u64 will always succeed on almost all computers. You can use expect instead of map_err


src/block_hash/block_hash_calculator_test.rs line 1 at r1 (raw file):

use super::concat_counts;

And this to util_test

@yoavGrs yoavGrs force-pushed the yoav/block_hash/concat_counts branch from e60bb7a to 3533d8c Compare May 9, 2024 08:57
Copy link
Contributor Author

@yoavGrs yoavGrs 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: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @ShahakShama)


src/block_hash/block_hash_calculator.rs line 1 at r1 (raw file):

Previously, ShahakShama wrote…

Rename this file to util

I plan to add fn calculate_block_hash to this file in the next PR.


src/block_hash/block_hash_calculator.rs line 37 at r1 (raw file):

Previously, ShahakShama wrote…

usize to u64 will always succeed on almost all computers. You can use expect instead of map_err

Done.


src/block_hash/block_hash_calculator_test.rs line 1 at r1 (raw file):

Previously, ShahakShama wrote…

And this to util_test

Same as above

Copy link
Contributor

@ShahakShama ShahakShama 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 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yoavGrs)

Copy link
Contributor

@ShahakShama ShahakShama 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 @yoavGrs)


src/block_hash/block_hash_calculator.rs line 19 at r2 (raw file):

    state_diff_length: usize,
    l1_data_availability_mode: L1DataAvailabilityMode,
) -> Result<StarkFelt, StarknetApiError> {

Change the return type

@yoavGrs yoavGrs force-pushed the yoav/block_hash/concat_counts branch from 3533d8c to b4d75a9 Compare May 9, 2024 09:01
Copy link
Contributor Author

@yoavGrs yoavGrs 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 3 files reviewed, 1 unresolved discussion (waiting on @ShahakShama)


src/block_hash/block_hash_calculator.rs line 19 at r2 (raw file):

Previously, ShahakShama wrote…

Change the return type

This Result comes from the new StrakFelt.
IMO new_unchecked is fine here.

Copy link
Contributor

@ShahakShama ShahakShama 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 3 files reviewed, 1 unresolved discussion (waiting on @yoavGrs)


src/block_hash/block_hash_calculator.rs line 19 at r2 (raw file):

Previously, yoavGrs wrote…

This Result comes from the new StrakFelt.
IMO new_unchecked is fine here.

I agree with you. use new_unchecked

Copy link
Contributor

@ShahakShama ShahakShama 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 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yoavGrs)

@yoavGrs yoavGrs merged commit 77ff5a3 into main May 9, 2024
6 checks passed
@yoavGrs yoavGrs deleted the yoav/block_hash/concat_counts branch May 9, 2024 09:05
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.

2 participants