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

L2 sync: verify and apply task tests #260

Merged
merged 17 commits into from
Sep 23, 2024
Merged

L2 sync: verify and apply task tests #260

merged 17 commits into from
Sep 23, 2024

Conversation

Mohiiit
Copy link
Contributor

@Mohiiit Mohiiit commented Sep 13, 2024

This PR covers the tests for the l2 sync, verify and apply task

Pull Request type

Please add the labels corresponding to the type of changes your PR introduces:

  • Testing

What is the current behavior?

Resolves: #NA

Does this introduce a breaking change?

No

@antiyro antiyro added the tests The label for testings label Sep 13, 2024
@Mohiiit Mohiiit marked this pull request as ready for review September 14, 2024 09:05
CHANGELOG.md Outdated Show resolved Hide resolved
///
/// This function generates an UnverifiedHeader with predefined values,
/// useful for creating consistent test scenarios.
fn create_dummy_unverified_header() -> UnverifiedHeader {
Copy link
Member

Choose a reason for hiding this comment

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

please include the dummy functions directly in the structure declaration file to enable easy reuse throughout the codebase. For example, the create_dummy_unverified_header function could be modified to take a protocol_version parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when you say structure declaration file, what does it mean? should have these helpers in a different tesl util file?

Copy link
Member

Choose a reason for hiding this comment

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

for example, I would see this function fitting well in the same file as the UnverifiedHeader struct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved it to another file, wdyt?

crates/client/block_import/src/verify_apply/classes.rs Outdated Show resolved Hide resolved
class_hash: Felt::from_hex_unchecked("0xfedcba0987654321fedcba0987654321fedcba0987654321fedcba0987654321"),
}];

let replaced_classes = vec![ReplacedClassItem {
Copy link
Member

Choose a reason for hiding this comment

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

I don’t think this case is possible because the contract is not deployed yet. I don’t believe we can use replaced_classes within a single genesis block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here we are doing the functional testing only, hence using dummy values. and I agree this case might not exist in real.

Copy link
Collaborator

@Trantorian1 Trantorian1 left a comment

Choose a reason for hiding this comment

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

Seems good to me. I agree with @jbcaron it would be nice to refactor dummy functions into a separate file. I also left a few minor issues to fix, nothing huge. Ping me once you've done the refactor and I will mark this as approved :)

crates/client/block_import/src/verify_apply.rs Outdated Show resolved Hide resolved
crates/client/block_import/src/verify_apply.rs Outdated Show resolved Hide resolved
crates/client/block_import/src/verify_apply.rs Outdated Show resolved Hide resolved
crates/client/block_import/src/verify_apply.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Trantorian1 Trantorian1 left a comment

Choose a reason for hiding this comment

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

All seems good!

@Trantorian1 Trantorian1 merged commit b599f60 into main Sep 23, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests The label for testings
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants