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

fix(common): v3 transaction hash calculation #1210

Merged
merged 1 commit into from
Oct 10, 2023
Merged

Conversation

yoavGrs
Copy link
Contributor

@yoavGrs yoavGrs commented Sep 27, 2023

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this introduce a breaking change?

  • Yes
  • No

Other information


This change is Reviewable

@yoavGrs yoavGrs self-assigned this Sep 27, 2023
@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

Merging #1210 (c9b0674) into main (2cf68ee) will increase coverage by 3.10%.
Report is 1 commits behind head on main.
The diff coverage is 94.41%.

❗ Current head c9b0674 differs from pull request most recent head a12b43a. Consider uploading reports for the commit a12b43a to get more accurate results

@@            Coverage Diff             @@
##             main    #1210      +/-   ##
==========================================
+ Coverage   71.05%   74.16%   +3.10%     
==========================================
  Files          83       78       -5     
  Lines        7947     7560     -387     
  Branches     7947     7560     -387     
==========================================
- Hits         5647     5607      -40     
+ Misses       1390     1066     -324     
+ Partials      910      887      -23     
Files Coverage Δ
crates/papyrus_common/src/block_hash.rs 83.17% <100.00%> (-1.57%) ⬇️
crates/papyrus_common/src/transaction_hash.rs 93.64% <94.18%> (+38.13%) ⬆️

... and 22 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@yoavGrs yoavGrs changed the title declare v3 fix(common): v3 transaction hash calculation Sep 27, 2023
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 4 of 4 files at r1.
Reviewable status: all files reviewed (commit messages unreviewed), 4 unresolved discussions (waiting on @yoavGrs)


crates/papyrus_common/src/transaction_hash.rs line 228 at r1 (raw file):

        .chain(&l1_resource)
        .chain(&l2_resource)
        .get_poseidon_hash())

Add comment why is this poseidon (with maybe reference to starknet docs)


crates/papyrus_common/src/transaction_hash.rs line 231 at r1 (raw file):

}

// Concat of: [0 | resource_name (56 bit) | max_amount (64 bit) | max_price_per_unit (128 bit)].

Same as below


crates/papyrus_common/src/transaction_hash.rs line 244 at r1 (raw file):

}

// Concat of [0...0 (192 bit) | nonce_mode (32 bit) | fee_mode (32 bit)].

You're already describing the concat in the expression. Consider instead
Receives nonce_mode and fee_mode and returns [0...0 ...


crates/papyrus_common/src/transaction_hash_test.rs line 47 at r1 (raw file):

    // The details were taken from Starknet Goerli. You can found the transactions by hash in:
    // https://alpha4.starknet.io/feeder_gateway/get_transaction?transactionHash=<transaction_hash>
    // The V3 transaction hashes generated by internal simulations.

generated -> are generated

@yoavGrs yoavGrs force-pushed the yoav/hash/v3_txs branch 2 times, most recently from d1a5dae to 3e002e0 Compare September 28, 2023 06:28
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 4 files reviewed, 3 unresolved discussions (waiting on @ShahakShama)


crates/papyrus_common/src/transaction_hash.rs line 228 at r1 (raw file):

Previously, ShahakShama wrote…

Add comment why is this poseidon (with maybe reference to starknet docs)

  1. A node doesn't design the hash.
  2. It is not in the docs yet.

crates/papyrus_common/src/transaction_hash.rs line 231 at r1 (raw file):

Previously, ShahakShama wrote…

Same as below

Same as below.


crates/papyrus_common/src/transaction_hash_test.rs line 47 at r1 (raw file):

Previously, ShahakShama wrote…

generated -> are generated

were generated?

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 r2.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @yoavGrs)


crates/papyrus_common/src/transaction_hash.rs line 231 at r1 (raw file):

Previously, yoavGrs wrote…

Same as below.

you did not do the same fix as below

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: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @yoavGrs)


crates/papyrus_common/src/transaction_hash.rs line 231 at r1 (raw file):

Previously, ShahakShama wrote…

you did not do the same fix as below

Done.
(I did the same as above :( )

ShahakShama
ShahakShama previously approved these changes Sep 28, 2023
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 r3, 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.

:lgtm:

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

@yoavGrs yoavGrs added this pull request to the merge queue Oct 10, 2023
Merged via the queue into main with commit e4b1ca8 Oct 10, 2023
17 of 18 checks passed
@yoavGrs yoavGrs deleted the yoav/hash/v3_txs branch October 10, 2023 08:46
@github-actions github-actions bot locked and limited conversation to collaborators Oct 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants