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: Add Unit Tests for get_batch_pubdata endpoint #160

Open
wants to merge 15 commits into
base: feat_get_pubdata_endpoint
Choose a base branch
from

Conversation

toni-calvin
Copy link

@toni-calvin toni-calvin commented Feb 20, 2024

Description:

Overview:

  • This pull request aims to enhance the test coverage for the get_batch_pubdata endpoint within the EthSender module. The primary objective is to validate the correctness of the endpoint by comparing the expected pubdata with the actual result obtained through the API call.

Changes Made:

  • Introduces unit tests for the get_batch_pubdata endpoint.
  • Utilizes the TestHelper struct within the EthSender module to facilitate the creation of batches, addition of pubdata, insertion, and commitment of batches.
  • The testing process involves invoking the endpoint for a specific batch number and comparing the obtained result with the expected pubdata inserted during batch creation.
  • For the current phase, the PR includes logging statements to display the inserted and committed L1 batch, as well as the result obtained from the endpoint.

For running the test: zk test rust -- get_batch_pubdata_impl --nocapture

@toni-calvin toni-calvin self-assigned this Feb 20, 2024
@toni-calvin toni-calvin changed the title Add Unit Tests for get_batch_pubdata endpoint in EthSender Module Add Unit Tests for get_batch_pubdata endpoint Feb 20, 2024
@toni-calvin toni-calvin changed the title Add Unit Tests for get_batch_pubdata endpoint feat: Add Unit Tests for get_batch_pubdata endpoint Feb 21, 2024
@toni-calvin toni-calvin force-pushed the feat_test_get_batch_pubdata_endpoint branch from 9fdd035 to 75271c5 Compare March 8, 2024 14:11
@toni-calvin toni-calvin changed the base branch from feat_get_batch_pubdata_endpoint to feat_get_pubdata_endpoint March 11, 2024 13:28
@toni-calvin toni-calvin force-pushed the feat_test_get_batch_pubdata_endpoint branch from b5a3e35 to 421897f Compare March 15, 2024 14:57
@toni-calvin toni-calvin marked this pull request as ready for review March 15, 2024 15:05
.map(|l1_batch_with_metadata| {
let partial_pubdata = l1_batch_with_metadata.construct_pubdata();
// The encoding of empty pubdata results in a vector of zeroes. We should return
// `None in this case.

Choose a reason for hiding this comment

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

I think you accidentally left out the character ` before None.

let partial_pubdata = l1_batch_with_metadata.construct_pubdata();
// The encoding of empty pubdata results in a vector of zeroes. We should return
// `None in this case.
if partial_pubdata == vec![0u8; partial_pubdata.len()] {

Choose a reason for hiding this comment

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

Could we consider using iter().all() to compare if all elements of partial_pubdata are zeroes?
This approach can potentially optimize performance by avoiding unnecessary allocation of a new vector of zeroes, particularly beneficial for larger vectors. The suggested change would look like:

if partial_pubdata.iter().all(|&x| x == 0) {
    Bytes::default()
} else {
    partial_pubdata.into()
}

Your thoughts on this optimization would be appreciated. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add unit tests for core/lib/dal/src/blocks_dal.rs's get_batch_pubdata function
3 participants