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.

Sign up for GitHub

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

Jump to bottom

add unit tests for BlobTransactionSidecar #6431

Merged
merged 5 commits into from
Feb 6, 2024
Merged

add unit tests for BlobTransactionSidecar #6431

merged 5 commits into from
Feb 6, 2024

Conversation

tcoratger
Copy link
Contributor

Description

This pull request adds unit tests for the BlobTransactionSidecar module to ensure its functionality and reliability. The tests cover various aspects, including sidecar generation, RLP encoding, and decoding.

Should be part of #6417.

@Evalir I just saw that you are assigned to the issue, feel free to use this, merge or close if you don't need it. I think this is far from covering the whole issue, so maybe you can merge this part and continue with the rest of the tests, don't hesitate if you need help on another part.

Changes

  • Added unit tests for BlobTransactionSidecar.
  • Improved test coverage for data extraction, sidecar generation, and RLP encoding/decoding.

@tcoratger tcoratger requested a review from gakonst as a code owner February 6, 2024 00:36
mattsse
mattsse requested changes Feb 6, 2024
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

ty for this, could we add some context where the blob data is coming from, like etherscan hash to the transaction

all the file reads should make use of the env!("CARGO_MANIFEST_DIR") path

the file reads can be simplified with fs::read_to_string

@mattsse mattsse added the C-test A change that impacts how or what we test label Feb 6, 2024
mattsse
mattsse requested changes Feb 6, 2024
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

last nits

ptal @Rjected

@tcoratger
Copy link
Contributor Author

ty for this, could we add some context where the blob data is coming from, like etherscan hash to the transaction

all the file reads should make use of the env!("CARGO_MANIFEST_DIR") path

the file reads can be simplified with fs::read_to_string

@mattsse I've added a .md file to provide references to the used blob data. Tell me what you think.

mattsse
mattsse approved these changes Feb 6, 2024
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

pending @Rjected

Rjected
Rjected requested changes Feb 6, 2024
View reviewed changes
Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

looks like a reasonable test, I just have a question about where the json format is from

crates/primitives/src/transaction/sidecar.rs
Comment on lines +520 to +531
let json_content = fs::read_to_string(
PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("src/transaction/blob_data/blob1.json"),
)
.expect("Failed to read the blob data file");

// Parse the JSON contents into a serde_json::Value
let json_value: serde_json::Value =
serde_json::from_str(&json_content).expect("Failed to deserialize JSON");

// Extract blob data from JSON and convert it to Blob
let blobs: Vec<Blob> = vec![Blob::from_hex(
json_value.get("data").unwrap().as_str().expect("Data is not a valid string"),
Copy link
Member

Choose a reason for hiding this comment

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

where is the json format from? was it hand-rolled or is it from an API somewhere? If hand-rolled, maybe this could instead just be hand rolled into the BlobTransactionSidecar json format. If it's from an API, I'd consider adding a test helper type that derives Serialize, Deserialize instead of using json.Value

Copy link
Collaborator

Choose a reason for hiding this comment

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

there's an api spec:

https://api.blobscan.com/

I can create a good first issue on the block explorers crate for adding a blob client

Copy link
Member

Choose a reason for hiding this comment

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

there's an api spec:

https://api.blobscan.com/

I can create a good first issue on the block explorers crate for adding a blob client

ah yeah, that would be great

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Rjected Indeed it is done by hand here, because I do not need all the data (proof for example as long as I generate the BlobTransactionSidecar via Blob uniquely).

For the blob for which I am testing the rlp, I also stored in the JSON the rlp that I calculated myself same aside to make the assertion, so this rlp (which is too big to be in a .rs file does not fit in the BlobTransactionSidecar spec).

For this PR, do we stay like this knowing that the client blob will arrive with the next issue or do you want me to make changes?

Copy link
Member

Choose a reason for hiding this comment

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

For this PR, do we stay like this knowing that the client blob will arrive with the next issue or do you want me to make changes?

Yeah, I think this is ok for now

Rjected
Rjected approved these changes Feb 6, 2024
View reviewed changes
Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

lgtm!

@Rjected Rjected added this pull request to the merge queue Feb 6, 2024
Merged via the queue into paradigmxyz:main with commit 93e213c Feb 6, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattsse mattsse mattsse approved these changes

@Rjected Rjected Rjected approved these changes

@gakonst gakonst Awaiting requested review from gakonst

Assignees
No one assigned
Labels
C-test A change that impacts how or what we test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tcoratger @Rjected @mattsse