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

tests: unit tests for hash computations in transaction.rs #2216

Merged
merged 10 commits into from
Jul 26, 2024

Conversation

g4titanx
Copy link
Contributor

@g4titanx g4titanx commented Jul 26, 2024

unit tests for the hash computation functions:

  • DeployAccount v3
  • Declare v1,v2,v3
  • Invoke v1,v3
  • L1Handler v1

issue number: 1942

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features
    • Introduced a new version (V3) of transaction hash computation for deploy and invoke transactions, enhancing the system's capability to handle various transaction types.
  • Bug Fixes
    • Improved accuracy of transaction hash calculations through updated logic, ensuring backward compatibility and reliability.
  • Tests
    • Added comprehensive test cases to verify the correctness of the new V3 hash computation functions, covering essential parameters for transaction execution.

Copy link

coderabbitai bot commented Jul 26, 2024

Warning

Rate limit exceeded

@kariy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 26 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

Commits

Files that changed from the base of the PR and between 1135e02 and 10b4f89.

Walkthrough

Ohayo, sensei! The recent updates significantly enhance the transaction hash computation within the Katana primitives module by upgrading the deploy and invoke transaction hash functions from version "V1" to "V3". This refinement not only improves their logic and structure but also introduces new test cases, ensuring correctness and robustness. These changes boost the system's capability to handle various transaction types while maintaining backward compatibility.

Changes

Files Change Summary
crates/katana/primitives/src/utils/transaction.rs Upgraded hash computation functions from V1 to V3, including renaming and adding comprehensive test cases to verify functionality.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant TransactionProcessor
    participant HashFunction

    User->>TransactionProcessor: Initiate Transaction
    TransactionProcessor->>HashFunction: Compute V3 Hash
    HashFunction-->>TransactionProcessor: Return V3 Hash
    TransactionProcessor-->>User: Confirm Transaction
Loading

This diagram illustrates the high-level interaction between the user and the transaction processing system, emphasizing the transition to the new V3 hash computation.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

@kariy kariy self-requested a review July 26, 2024 14:08
crates/katana/primitives/src/utils/transaction.rs Outdated Show resolved Hide resolved
crates/katana/primitives/src/utils/transaction.rs Outdated Show resolved Hide resolved
crates/katana/primitives/src/utils/transaction.rs Outdated Show resolved Hide resolved
crates/katana/primitives/src/utils/transaction.rs Outdated Show resolved Hide resolved
crates/katana/primitives/src/utils/transaction.rs Outdated Show resolved Hide resolved
crates/katana/primitives/src/utils/transaction.rs Outdated Show resolved Hide resolved
crates/katana/primitives/src/utils/transaction.rs Outdated Show resolved Hide resolved
crates/katana/primitives/src/utils/transaction.rs Outdated Show resolved Hide resolved
@g4titanx
Copy link
Contributor Author

resolved

@g4titanx
Copy link
Contributor Author

i run out of memory when i try to run test, ran the part i could and it was successful

@kariy
Copy link
Member

kariy commented Jul 26, 2024

i run out of memory when i try to run test, ran the part i could and it was successful

did you run the tests for the entire workspace? in this case running the tests only for the katana-primitives crate should be enough

@g4titanx
Copy link
Contributor Author

i did try to run it specifically for that module, but it's compiling for the entire crate instead

@g4titanx
Copy link
Contributor Author

great, test passed

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (3)
crates/katana/primitives/src/utils/transaction.rs (3)

Line range hint 70-98:
Consider using constants for repeated values.

The value FieldElement::THREE is used multiple times. Consider defining a constant for better readability and maintainability.

const VERSION_THREE: FieldElement = FieldElement::THREE;

poseidon_hash_many(&[
    PREFIX_INVOKE,
    if is_query { QUERY_VERSION_OFFSET + VERSION_THREE } else { VERSION_THREE }, /* version */
    // ...
])

Line range hint 168-180:
Consider using constants for repeated values.

The value FieldElement::THREE is used multiple times. Consider defining a constant for better readability and maintainability.

const VERSION_THREE: FieldElement = FieldElement::THREE;

poseidon_hash_many(&[
    PREFIX_DECLARE,
    if is_query { QUERY_VERSION_OFFSET + VERSION_THREE } else { VERSION_THREE }, /* version */
    // ...
])

Line range hint 203-229:
Consider using constants for repeated values.

The value FieldElement::THREE is used multiple times. Consider defining a constant for better readability and maintainability.

const VERSION_THREE: FieldElement = FieldElement::THREE;

poseidon_hash_many(&[
    PREFIX_INVOKE,
    if is_query { QUERY_VERSION_OFFSET + VERSION_THREE } else { VERSION_THREE }, /* version */
    // ...
])

@kariy
Copy link
Member

kariy commented Jul 26, 2024

great, test passed

ok, i've fixed some of the failing ones bcs the fields in the hash computations were literally in the wrong order.

thanks for helping out on this

Copy link

codecov bot commented Jul 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.86%. Comparing base (71b1f1a) to head (10b4f89).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2216      +/-   ##
==========================================
+ Coverage   69.07%   69.86%   +0.79%     
==========================================
  Files         339      341       +2     
  Lines       44052    44719     +667     
==========================================
+ Hits        30428    31244     +816     
+ Misses      13624    13475     -149     

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

@kariy kariy merged commit ea1b6a8 into dojoengine:main Jul 26, 2024
15 checks passed
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