Publish State Diff to Eth #7
Conversation
WalkthroughRecent updates to the Changes
Poem
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? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
crates/orchestrator/src/jobs/da_job/mod.rs (1)
Line range hint
198-267: Consider addressing the TODO and hardcoded values instate_update_to_blob_data.The function contains a TODO comment and hardcoded values for
FieldElement::ONE. It's important to clarify these points or resolve them to ensure the robustness and maintainability of the code.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (9)
- Cargo.toml (2 hunks)
- crates/da_clients/da-client-interface/src/lib.rs (1 hunks)
- crates/da_clients/ethereum/.env_example (1 hunks)
- crates/da_clients/ethereum/Cargo.toml (1 hunks)
- crates/da_clients/ethereum/src/lib.rs (1 hunks)
- crates/orchestrator/Cargo.toml (2 hunks)
- crates/orchestrator/src/jobs/constants.rs (1 hunks)
- crates/orchestrator/src/jobs/da_job/mod.rs (6 hunks)
- crates/orchestrator/src/jobs/mod.rs (1 hunks)
Files skipped from review due to trivial changes (2)
- crates/da_clients/ethereum/.env_example
- crates/orchestrator/src/jobs/mod.rs
Additional comments not posted (27)
crates/da_clients/ethereum/Cargo.toml (5)
7-15: Updatedalloydependency to include new blockchain-related features and a specific revision. This aligns with the PR's objectives to enhance Ethereum interactions.
20-20: Updatedreqwestto version0.12.3, likely for improved stability or new minor features.
21-21: Addedc-kzgdependency, likely for cryptographic operations, supporting the PR's blockchain functionality enhancements.
22-22: Addeddotenvfor better management of environment variables, enhancing secure configuration practices.
26-28: Addedrstestandtokioas workspace dependencies for enhanced testing and asynchronous runtime support.Also applies to: 30-30
crates/orchestrator/src/jobs/constants.rs (3)
7-7: AddedBLOB_LENconstant for blob size, supporting data handling in Ethereum-related functionalities.
8-20: Defined cryptographic constantsBLS_MODULUSandGENERATORusinglazy_static, crucial for blockchain operations.
21-21: AddedTWOas a constant, likely used in mathematical operations within blockchain functionalities.crates/da_clients/da-client-interface/src/lib.rs (3)
20-20: Modifiedpublish_state_diffto acceptVec<Vec<u8>>, aligning with the handling of raw byte data for Ethereum transactions.
24-24: Addedmax_blob_per_txnfunction to manage the number of blobs per transaction, enhancing blockchain transaction management.
26-26: Addedmax_bytes_per_blobfunction to set limits on the byte size per blob, supporting efficient data management in blockchain transactions.Cargo.toml (5)
18-18: Addednumlibrary for enhanced numerical operations, likely supporting blockchain-related data handling.
19-19: Addedethereum-da-clientas a direct dependency, central to the PR's focus on enhancing Ethereum interactions.
21-21: Addedda-client-interfaceto standardize interactions across different blockchain platforms, enhancing modularity and interoperability.
39-39: Addednum-bigintfor handling large integers, crucial for cryptographic operations in blockchain functionalities.
41-42: Addednum-traitsfor numeric traits in generic programming andlazy_staticfor efficient use of immutable static values, enhancing performance and flexibility.crates/orchestrator/Cargo.toml (4)
9-9: Repositionedthiserrorwithin workspace dependencies for better organizational clarity and dependency management.
23-25: Addednum-bigint,num-traits, andnumfor enhanced handling of numerical data, crucial for cryptographic calculations in blockchain contexts.
26-26: Addedlazy_staticfor efficient definition of immutable static values, enhancing performance and code clarity.
31-32: Addedmajin-blob-typesandmajin-blob-corefor handling specific data structures in blockchain functionalities, supporting the new features introduced.crates/da_clients/ethereum/src/lib.rs (5)
3-9: Extensive imports fromalloycovering various aspects of Ethereum transactions such as consensus, eips, and network, aligning with the PR's focus on enhancing Ethereum interactions.
36-72: Significantly enhancedpublish_state_diffto handle complex Ethereum transactions with cryptographic proofs, central to the PR's objectives.
79-85: Addedmax_blob_per_txnandmax_bytes_per_blobfunctions to provide crucial configuration limits for Ethereum transactions, enhancing resource management.
98-121: Addedprepare_sidecarfunction to efficiently handle large and complex data structures for Ethereum transactions, supporting the new functionalities.
123-199: Extensive test cases added to ensure robustness and correctness of the new Ethereum transaction functionalities.crates/orchestrator/src/jobs/da_job/mod.rs (2)
147-167: Functionconvert_to_biguintis correctly implemented.This function efficiently converts a vector of
FieldElementtoBigUint, handling edge cases appropriately.
360-370: Functiontest_fft_transformationis correctly implemented.This function effectively tests the
fft_transformationfunction, ensuring its correctness across different scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
crates/orchestrator/src/jobs/da_job/mod.rs (1)
Line range hint
198-267: Refactor thestate_update_to_blob_datafunction to improve readability and maintainability. Consider breaking down the function into smaller, more manageable functions.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (5)
- Cargo.toml (2 hunks)
- crates/da_clients/da-client-interface/src/lib.rs (1 hunks)
- crates/orchestrator/Cargo.toml (2 hunks)
- crates/orchestrator/src/jobs/da_job/mod.rs (6 hunks)
- crates/orchestrator/src/jobs/mod.rs (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- Cargo.toml
- crates/da_clients/da-client-interface/src/lib.rs
- crates/orchestrator/Cargo.toml
- crates/orchestrator/src/jobs/mod.rs
Additional comments not posted (6)
crates/orchestrator/src/jobs/da_job/mod.rs (6)
122-145: Move the constantsBLS_MODULUSandGENERATORto a central location as previously suggested.
169-196: Replace the panic with proper error handling in thedata_to_blobsfunction to enhance robustness and user experience.
339-350: Complete the implementation oftest_state_update_to_blob_datato ensure the reliability and correctness of thestate_update_to_blob_datafunction.
274-305: Optimize nonce handling inda_wordto avoid unnecessary conversions and potential performance issues.
74-100: Ensure proper error handling for potential failures instate_update_to_blob_dataandpublish_state_difffunctions.
Line range hint
313-370: Ensure that the test cases cover all possible scenarios and edge cases to guarantee the robustness of theda_wordfunction.
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
crates/orchestrator/src/jobs/da_job/mod.rs (1)
[!TIP]
Codebase VerificationThe
test_state_update_to_blob_datafunction is present but marked as "not yet done," indicating it is incomplete. This means the function does not currently verify the integrity of thestate_update_to_blob_datafunction as intended.
test_state_update_to_blob_datais incomplete and does not fully test thestate_update_to_blob_datafunction.- Ensure that the test function is completed and covers all critical paths and edge cases for
state_update_to_blob_data.Analysis chain
Line range hint
199-275: Significant changes instate_update_to_blob_datainclude handling of various blockchain state components. Ensure that all mappings and transformations are accurate and that the data integrity is maintained throughout the process.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify data integrity in `state_update_to_blob_data`. # Test: Search for tests related to state update transformations. rg --type rust 'state_update_to_blob_data'Length of output: 517
Script:
#!/bin/bash # Extract the test function `test_state_update_to_blob_data` to review its content and coverage. ast-grep --lang rust --pattern 'async fn test_state_update_to_blob_data($$_) { $$$ }'Length of output: 87
Script:
#!/bin/bash # Locate the `test_state_update_to_blob_data` function and extract its content for review. rg 'async fn test_state_update_to_blob_data' -A 20 crates/orchestrator/src/jobs/da_job/mod.rsLength of output: 1063
Script:
#!/bin/bash # Extract more lines to capture the entire `test_state_update_to_blob_data` function for review. rg 'async fn test_state_update_to_blob_data' -A 100 crates/orchestrator/src/jobs/da_job/mod.rsLength of output: 3041
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (3)
- crates/da_clients/ethereum/src/lib.rs (1 hunks)
- crates/orchestrator/Cargo.toml (2 hunks)
- crates/orchestrator/src/jobs/da_job/mod.rs (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- crates/orchestrator/Cargo.toml
Additional comments not posted (6)
crates/da_clients/ethereum/src/lib.rs (2)
3-11: Imports have been organized and expanded to include necessary modules for the new functionality.
98-121: Theprepare_sidecarfunction handles the creation of sidecar data for Ethereum transactions. It's crucial to ensure that the KZG commitments and proofs are correctly computed and that the sidecar data aligns with Ethereum's requirements.crates/orchestrator/src/jobs/da_job/mod.rs (4)
123-146: Thefft_transformationfunction has been updated to use constants directly within the function. Consider refactoring these constants to a central location for better maintainability and to avoid duplication, as previously suggested.
170-197: Thedata_to_blobsfunction now includes error handling for blob size validation, replacing the previous panic. This is a significant improvement in robustness and user experience.
74-98: Theprocess_jobmethod has been updated to handle new job types and includes transformations necessary for data submission. Ensure that error messages are clear and that all potential failure points are handled gracefully.Verification successful
The script output indicates that error handling using
eyreandErris present incrates/orchestrator/src/jobs/da_job/mod.rs. Let's review these instances to ensure they are clear and cover all potential failure points.Error Handling Instances in
crates/orchestrator/src/jobs/da_job/mod.rs:
These error messages appear to be clear and provide specific details about the failure conditions. The first error message specifies the block number, job ID, allowed number of blobs, and the actual number of blobs, which is helpful for debugging. The second error message includes the error encountered while getting the nonce.
Given this information, it seems that the error handling in
process_jobis clear and covers the potential failure points.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that error messages in `process_job` are clear and cover all failure points. # Test: Search for error handling in `process_job`. rg --type rust 'eyre|Err'Length of output: 5815
282-313: Theda_wordfunction encodes various blockchain-related parameters into a single field element. Ensure that the encoding is correct and that all edge cases are handled.Verification successful
To verify the correctness of the
da_wordfunction encoding and ensure all edge cases are handled, we need to review the implementation of theda_wordfunction and the associated test functionda_word_works.
To ensure the correctness of the
da_wordfunction encoding and that all edge cases are handled, we need to locate and review the test cases forda_word. Let's perform a more comprehensive search to locate the test functionda_word_worksand any other relevant test cases.
To ensure the correctness of the
da_wordfunction encoding and that all edge cases are handled, we need to extract and review the implementation of theda_word_workstest function fromcrates/orchestrator/src/jobs/da_job/mod.rs.
To ensure the correctness of the
da_wordfunction encoding and that all edge cases are handled, we need to perform a broader search to locate theda_word_worksfunction and any other relevant test cases.
The
da_wordfunction's encoding appears to be tested by theda_word_worksfunction, which uses therstestframework to validate various cases. The test cases cover different combinations ofclass_flag,new_nonce, andnum_changes, ensuring that the function handles these inputs correctly.The test cases provided are:
#[case(false, 1, 1, "18446744073709551617")]#[case(false, 1, 0, "18446744073709551616")]#[case(false, 0, 6, "6")]#[case(true, 1, 0, "340282366920938463481821351505477763072")]These cases seem to cover a range of scenarios, including different values for
class_flag,new_nonce, andnum_changes.
- The
da_wordfunction implementation and the associated test functionda_word_worksare located incrates/orchestrator/src/jobs/da_job/mod.rs.Based on the provided context and the test cases, the encoding logic in the
da_wordfunction appears to be correct and well-tested.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of encoding in `da_word`. # Test: Search for tests related to `da_word`. rg --type rust 'da_word'Length of output: 741
Script:
#!/bin/bash # Description: Retrieve the implementation of `da_word` and the test function `da_word_works`. # Extract the `da_word` function implementation ast-grep --lang rust --pattern 'fn da_word($_, $_, $_) -> $_ { $$$ }' crates/orchestrator/src/jobs/da_job/mod.rs # Extract the `da_word_works` test function implementation ast-grep --lang rust --pattern 'fn da_word_works($_) { $$$ }' crates/orchestrator/src/jobs/da_job/mod.rsLength of output: 3225
Script:
#!/bin/bash # Description: Perform a comprehensive search for the test function `da_word_works` and any other relevant test cases for `da_word`. # Search for the `da_word_works` function definition rg --type rust 'fn da_word_works' # Search for test cases related to `da_word` rg --type rust 'da_word'Length of output: 842
Script:
#!/bin/bash # Description: Extract the implementation of the `da_word_works` test function. # Extract the `da_word_works` function implementation ast-grep --lang rust --pattern 'fn da_word_works($_) { $$$ }' crates/orchestrator/src/jobs/da_job/mod.rsLength of output: 106
Script:
#!/bin/bash # Description: Perform a broader search for the `da_word_works` function and any other relevant test cases. # Search for the `da_word_works` function definition rg 'fn da_word_works' --context 10 # Search for test cases related to `da_word` rg 'da_word' --context 10Length of output: 8284
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (4)
- crates/da_clients/ethereum/src/config.rs (1 hunks)
- crates/da_clients/ethereum/src/lib.rs (1 hunks)
- crates/orchestrator/Cargo.toml (2 hunks)
- crates/orchestrator/src/jobs/da_job/mod.rs (6 hunks)
Additional comments not posted (22)
crates/da_clients/ethereum/src/config.rs (2)
8-8: Addedprivate_keyfield toEthereumDaConfig.This addition aligns with the PR's objective to handle Ethereum transactions, which typically require authentication via a private key.
16-16: Ensure environment variablePRIVATE_KEYis well-documented.crates/orchestrator/Cargo.toml (2)
16-17: Added dependencies onmajin-blob-typesandmajin-blob-core.These additions are necessary for handling blob data types, which are part of the new functionality to send state diff data to Ethereum.
Line range hint
18-36: Updated and added various dependencies to support new functionality.These updates include necessary libraries for bigint operations, async handling, and Ethereum interactions, which are crucial for the new features.
crates/da_clients/ethereum/src/lib.rs (10)
3-9: Imported multiple modules fromalloy, which are essential for Ethereum transaction handling.These imports are necessary for the new functionality related to Ethereum transactions.
11-11: Added imports for handling asynchronous traits and HTTP transactions.These are crucial for the async operations and HTTP communications within the Ethereum client.
16-19: Updated imports to reflect changes in dependencies and error handling.This update ensures compatibility with the latest versions of the dependencies and proper error handling.
23-27: Added imports for KZG cryptography operations, which are part of the new sidecar transaction functionality.These additions are essential for the cryptographic operations required by the new Ethereum transaction features.
31-33: UpdatedEthereumDaClientstruct to include new fields for provider, wallet, and trusted setup.These updates are crucial for the enhanced functionality of handling Ethereum transactions with proper authentication and cryptographic settings.
38-73: Refactoredpublish_state_diffto handle new data structures and Ethereum transaction logic.This refactoring is aligned with the PR's objectives to handle state diff data and includes comprehensive handling from preparation to transaction submission.
80-86: Added methods to determine transaction and blob size limits.These methods are necessary for managing the size constraints of Ethereum transactions, which is a new requirement mentioned in the PR.
91-99: Refactored theFromimplementation forEthereumDaClientto include new configurations and error handling.This refactoring ensures that the Ethereum client is correctly configured with environmental settings and error handling.
102-125: Addedprepare_sidecarfunction to handle the preparation of transaction sidecars.This function is crucial for preparing the necessary components of a transaction sidecar, which is part of the new functionality.
127-198: Enhanced test coverage for KZG operations.This enhancement is crucial for ensuring the reliability of the cryptographic operations involved in the new features.
crates/orchestrator/src/jobs/da_job/mod.rs (8)
5-14: Added multiple imports to support new functionality in job processing.These imports are necessary for handling bigint operations, async traits, and other functionalities required by the new features.
22-40: Defined constants usinglazy_staticfor cryptographic operations.Defining these constants is essential for the cryptographic operations required by the new Ethereum transaction features.
73-98: Enhanced job processing logic to handle new blob data transformations and Ethereum transaction constraints.This enhancement is aligned with the PR's objectives to handle state diff data and includes comprehensive logic for data transformation and transaction handling.
123-140: Addedfft_transformationfunction to perform Fast Fourier Transform on data.This function is crucial for the data transformation required by the new features, particularly in preparing data for Ethereum transactions.
142-191: Addedconvert_to_biguintanddata_to_blobsfunctions to handle data conversions necessary for blob data preparation.These functions are essential for converting state update data into the appropriate formats for Ethereum transactions.
Line range hint
193-268: Refactoredstate_update_to_blob_datato handle new data structures and processing logic.This refactoring is crucial for converting state update data into blob data, aligning with the new requirements for Ethereum transactions.
275-306: Refinedda_wordencoding logic to handle new nonce and class flag settings.This refinement is necessary for the correct encoding of data attributes, which is crucial for the new Ethereum transaction features.
Line range hint
314-417: Enhanced test coverage for state update to blob data conversions and FFT transformations.This enhancement is crucial for ensuring the reliability and correctness of the data transformations involved in the new features.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
crates/da_clients/da-client-interface/src/lib.rs (1)
25-27: Addition ofmax_blob_per_txnandmax_bytes_per_blobfunctions to manage transaction size limits. Please provide documentation for these functions to clarify their usage and constraints.crates/orchestrator/src/tests/jobs/da_job/mod.rs (1)
58-61: The testtest_process_jobuses the new blob size limits effectively to simulate Ethereum transaction conditions. Consider adding more edge cases to ensure robustness.crates/orchestrator/src/jobs/da_job/mod.rs (1)
Line range hint
193-268: Refine the state update to blob data transformation.Consider refactoring the
state_update_to_blob_datafunction to reduce complexity and improve readability. The function currently handles multiple tasks, which could be split into smaller, more focused functions. Additionally, the handling of special cases and error conditions could be separated out to make the main logic clearer.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (9)
- crates/da_clients/da-client-interface/src/lib.rs (2 hunks)
- crates/da_clients/ethereum/Cargo.toml (1 hunks)
- crates/da_clients/ethereum/src/lib.rs (1 hunks)
- crates/orchestrator/src/jobs/da_job/mod.rs (6 hunks)
- crates/orchestrator/src/jobs/da_job/test_data/nonces_from_block_631861.txt (1 hunks)
- crates/orchestrator/src/jobs/da_job/test_data/nonces_from_block_638353.txt (1 hunks)
- crates/orchestrator/src/jobs/da_job/test_data/nonces_from_block_640641.txt (1 hunks)
- crates/orchestrator/src/tests/common/constants.rs (1 hunks)
- crates/orchestrator/src/tests/jobs/da_job/mod.rs (2 hunks)
Files skipped from review due to trivial changes (3)
- crates/orchestrator/src/jobs/da_job/test_data/nonces_from_block_631861.txt
- crates/orchestrator/src/jobs/da_job/test_data/nonces_from_block_638353.txt
- crates/orchestrator/src/tests/common/constants.rs
Additional comments not posted (9)
crates/da_clients/ethereum/Cargo.toml (3)
7-15: Thealloydependency has been updated with several new features necessary for Ethereum operations. Ensure all new features are utilized appropriately in the codebase.
21-22: Addition ofc-kzganddotenvfor cryptographic operations and environment management is appropriate. Ensure that the cryptographic operations are secure and correctly implemented.
20-20: Update toreqwestversion0.12.3noted. Please ensure there are no breaking changes from this update that might affect existing functionality.Verification successful
The
reqwestversion update to0.12.3does not introduce any breaking changes as per the changelog. Thereqwest::Clientis used incrates/da_clients/ethereum/src/lib.rs, but no specific breaking changes were found that would affect this usage. Additionally, there are tests in place that indirectly cover the functionality that relies onreqwest.
crates/da_clients/ethereum/src/lib.rs: Usage ofreqwest::Client.crates/da_clients/ethereum/src/lib.rs: Tests available undermod tests.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for breaking changes in reqwest update. # Test: Search for migration guides or breaking changes in reqwest documentation. curl https://docs.rs/reqwest/0.12.3Length of output: 354
Script:
#!/bin/bash # Find all Cargo.toml files and search for reqwest dependency to confirm all versions in use. fd Cargo.toml --exec grep -H 'reqwest' # Find all Rust files that use reqwest to identify usage patterns. fd .rs --exec grep -H 'reqwest' # Check if there are any tests related to reqwest usage. fd .rs --exec grep -H 'reqwest' | grep -H 'test'Length of output: 371
Script:
#!/bin/bash # Extract the code in crates/da_clients/ethereum/src/lib.rs that uses reqwest::Client. grep -A 10 -B 10 'reqwest::Client' crates/da_clients/ethereum/src/lib.rs # Check the reqwest changelog for version 0.12.3. curl -s https://raw.githubusercontent.com/seanmonstar/reqwest/master/CHANGELOG.md | grep -A 10 -B 10 '0.12.3' # Identify functions in crates/da_clients/ethereum/src/lib.rs using reqwest and check for corresponding tests. ast-grep --lang rust --pattern 'fn $_($$) -> $_ { $$$ }' crates/da_clients/ethereum/src/lib.rs | grep -A 10 'reqwest'Length of output: 1600
Script:
#!/bin/bash # Extract more lines around the reqwest::Client usage in crates/da_clients/ethereum/src/lib.rs to understand the context better. grep -A 50 -B 50 'reqwest::Client' crates/da_clients/ethereum/src/lib.rs # Search for test files that may indirectly cover reqwest functionality. fd .rs --exec grep -H 'test'Length of output: 9307
crates/da_clients/da-client-interface/src/lib.rs (1)
21-21: The change in parameter type forpublish_state_difffromVec<FieldElement>toVec<Vec<u8>>is noted. Ensure that all callers of this function are updated to use the new data type.Verification successful
The search results show multiple occurrences of
publish_state_diff. To ensure the function call incrates/orchestrator/src/jobs/da_job/mod.rsuses the new signature, we need to examine the specific code around that call.
The function
data_to_blobsreturns aResult<Vec<Vec<u8>>>, which confirms thatblob_arrayis of typeVec<Vec<u8>>and matches the new signature ofpublish_state_diff.
crates/orchestrator/src/jobs/da_job/mod.rs: let blob_array = data_to_blobs(max_bytes_per_blob, transformed_data).expect("error while converting blob data to vec<u8>");crates/orchestrator/src/jobs/da_job/mod.rs: let external_id = config.da_client().publish_state_diff(blob_array).await?;This confirms that all callers of
publish_state_diffhave been updated to use the new data type.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `publish_state_diff` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust $'publish_state_diff'Length of output: 562
Script:
#!/bin/bash # Description: Verify the function call in `crates/orchestrator/src/jobs/da_job/mod.rs` uses the new signature. # Test: Extract the relevant code around the function call. rg -C 5 'publish_state_diff' crates/orchestrator/src/jobs/da_job/mod.rsLength of output: 390
Script:
#!/bin/bash # Description: Verify the type of `blob_array` used in the `publish_state_diff` call. # Test: Search for `blob_array` in the file to find its definition and type. rg 'blob_array' crates/orchestrator/src/jobs/da_job/mod.rsLength of output: 244
Script:
#!/bin/bash # Description: Extract the full initialization code of `blob_array`. # Test: Extract the full line where `blob_array` is initialized to verify its type. rg -A 1 'let blob_array =' crates/orchestrator/src/jobs/da_job/mod.rsLength of output: 215
Script:
#!/bin/bash # Description: Verify the return type of `data_to_blobs`. # Test: Search for the definition of `data_to_blobs` to confirm its return type. rg 'fn data_to_blobs' --type rustLength of output: 163
crates/orchestrator/src/tests/jobs/da_job/mod.rs (1)
9-12: Importing new constants for Ethereum blob size limits is appropriate. Ensure these values are consistent with Ethereum's current specifications.crates/da_clients/ethereum/src/lib.rs (2)
94-99: The methodsmax_blob_per_txnandmax_bytes_per_blobhave been added to manage transaction size limits. Ensure these values are configurable and validated against Ethereum's current capabilities.
37-77: Thepublish_state_diffmethod has been significantly refactored to handle Ethereum transactions. Ensure that this method and its dependencies are thoroughly tested, especially the new transaction creation and signing logic.crates/orchestrator/src/jobs/da_job/mod.rs (2)
22-40: Review the use oflazy_static!for constants.The use of
lazy_static!for defining constants likeGENERATOR,BLS_MODULUS, andBLOB_LENis appropriate given their constant nature and the need for complex initialization. Good use of resources.
161-190: Validate and handle blob size correctly.The function
data_to_blobscorrectly checks the blob size to ensure it meets the minimum requirements and handles the conversion of data into blobs efficiently. Good error handling and data processing practices are observed.
crates/orchestrator/src/jobs/da_job/test_data/nonces_from_block_640641.txt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
crates/orchestrator/src/jobs/da_job/mod.rs (1)
Line range hint
193-268: Complete the implementation oftest_state_update_to_blob_data.The test function
test_state_update_to_blob_datais marked as not yet done. Completing this test is crucial for ensuring the reliability and correctness of thestate_update_to_blob_datafunction. Would you like me to help by providing a draft implementation or opening a GitHub issue to track this task?
| lazy_static! { | ||
| /// EIP-4844 BLS12-381 modulus. | ||
| /// | ||
| /// As defined in https://eips.ethereum.org/EIPS/eip-4844 | ||
|
|
||
| /// Generator of the group of evaluation points (EIP-4844 parameter). | ||
| pub static ref GENERATOR: BigUint = BigUint::from_str( | ||
| "39033254847818212395286706435128746857159659164139250548781411570340225835782", | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| pub static ref BLS_MODULUS: BigUint = BigUint::from_str( | ||
| "52435875175126190479447740508185965837690552500527637822603658699938581184513", | ||
| ) | ||
| .unwrap(); | ||
| pub static ref TWO: BigUint = 2u32.to_biguint().unwrap(); | ||
|
|
||
| pub static ref BLOB_LEN: usize = 4096; | ||
| } |
There was a problem hiding this comment.
Move common constants to a central location for better maintainability.
The constants GENERATOR, BLS_MODULUS, TWO, and BLOB_LEN are defined within a lazy_static! block. For better maintainability and to avoid duplication, consider moving these constants to a central configuration or constants file. This change would align with the previously discussed improvements and centralize the configuration, making the codebase easier to manage.
| fn data_to_blobs(blob_size: u64, block_data: Vec<BigUint>) -> Result<Vec<Vec<u8>>> { | ||
| // Validate blob size | ||
| if blob_size < 32 { | ||
| return Err(eyre!( | ||
| "Blob size must be at least 32 bytes to accommodate a single FieldElement/BigUint, but was {}", | ||
| blob_size, | ||
| )); | ||
| } |
There was a problem hiding this comment.
Replace panic with error handling in data_to_blobs.
Instead of panicking when the blob size is too small, consider returning an error. This approach enhances the robustness of the function and improves user experience by providing a clear error message instead of terminating the application.
if blob_size < 32 {
return Err("Blob size must be at least 32 bytes to accommodate a single FieldElement/BigUint".into());
}| // padding of 127 bits | ||
| let mut binary_string = "0".repeat(127); | ||
|
|
||
| // class flag of one bit | ||
| if class_flag { | ||
| word += FieldElement::from_hex_be(CLASS_FLAG_TRUE).unwrap(); | ||
| binary_string += "1" | ||
| } else { | ||
| binary_string += "0" | ||
| } | ||
| if let Some(new_nonce) = nonce_change { | ||
| word += new_nonce + FieldElement::from_hex_be(NONCE_BASE).unwrap(); | ||
|
|
||
| // checking for nonce here | ||
| if let Some(_new_nonce) = nonce_change { | ||
| let bytes: [u8; 32] = nonce_change.unwrap().to_bytes_be(); | ||
| let biguint = BigUint::from_bytes_be(&bytes); | ||
| let binary_string_local = format!("{:b}", biguint); | ||
| let padded_binary_string = format!("{:0>64}", binary_string_local); | ||
| binary_string += &padded_binary_string; | ||
| } else { | ||
| let binary_string_local = "0".repeat(64); | ||
| binary_string += &binary_string_local; | ||
| } | ||
|
|
||
| word += FieldElement::from(num_changes); | ||
| let binary_representation = format!("{:b}", num_changes); | ||
| let padded_binary_string = format!("{:0>64}", binary_representation); | ||
| binary_string += &padded_binary_string; | ||
|
|
||
| let biguint = BigUint::from_str_radix(binary_string.as_str(), 2).expect("Invalid binary string"); | ||
|
|
||
| // Now convert the BigUint to a decimal string | ||
| let decimal_string = biguint.to_str_radix(10); | ||
|
|
||
| let word = FieldElement::from_dec_str(&decimal_string).expect("issue while converting to fieldElement"); |
There was a problem hiding this comment.
Optimize nonce handling in da_word.
Consider refactoring the nonce handling to avoid unnecessary conversions and potential performance issues. Here's a proposed change:
if let Some(new_nonce) = nonce_change {
let biguint = BigUint::from_bytes_be(&new_nonce.to_bytes_be());
binary_string += &format!("{:0>64}", biguint.to_str_radix(2));
} else {
binary_string += &"0".repeat(64);
}This change avoids redundant conversions and directly formats the BigUint into a binary string.
|
|
||
| let tx = TxEip4844 { | ||
| chain_id, | ||
| nonce: 0, // can be block number |
There was a problem hiding this comment.
If I'm not missing something, it won't be set right?
There should be NonceFiller in the alloy provider chain to handle that, couldn't find it explicitly set.
There was a problem hiding this comment.
ya makes sense, @Mohiiit let's fetch the nonce from the RPC itself
| gas_limit: 30_000_000, | ||
| max_fee_per_gas: eip1559_est.max_fee_per_gas.to_string().parse()?, | ||
| max_priority_fee_per_gas, | ||
| to: addr, // maybe to the L1 contract for verification?? |
There was a problem hiding this comment.
to, input, and gas_limit should probably be function arguments, if the goal is to reuse Starknet core contract, that would be its address, updateStateKzgDA calldata, and appropriate gas_limit to account for the spendings
There was a problem hiding this comment.
right. i think we should fill in to and gas_limit for now. input can be done in a refactor later on when we integrate updateState
@Mohiiit we can ideally take to as an input and gas_limit can be estimated?
There was a problem hiding this comment.
Hey @unstark, I have set the nonce by fetching it directly and used the to address as an input. Created an issue for the gas limit for now :)
* strucutre for sending blob added * [test]: function defined * added condition to fetch nonce when the state_diff len is not-zero * updated the condition for nonce getting, to not get it for one contract address * test added for fft function * removed some unused comments * updated test_state_update_to_blob_Data test * deleted extra files * issue resolved * mocking added for state update to blob function * estimation added for txn and chain id dynamically setting * txn inclusion logic added and code cleanup * error message updated for blob length error * linting fix * linting fix * toml linting fixed * added nonce and to address --------- Co-authored-by: apoorvsadana <[email protected]>
functionality added to send state diff data to eth as blob
added tests
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
state_update_to_blob_datafor better nonce retrieval and data processing.publish_state_diffmethod to support new transaction handling logic.Documentation
Tests