feat(eth-bytecode-db): integrate verifier_alliance schema v1 #1202
Conversation
…bytecode-db-logic' crate
…ses into separate crate
…nt with verifier_alliance_v1 database schema
…ification-common crates for verifier alliance code processing and insertion
…tity and verifier-alliance-migration crates
WalkthroughThis pull request introduces substantial changes to the Assessment against linked issues
Possibly related PRs
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 10
🔭 Outside diff range comments (1)
eth-bytecode-db/verifier-alliance-database-tests/src/test_case.rs (1)
Line range hint
119-308: Add timeout handling for database operations.Database operations could potentially hang. Consider adding timeouts to prevent test hangs.
+ use tokio::time::timeout; + use std::time::Duration; - async fn validate_contract_deployments_table( + async fn validate_contract_deployments_table( &self, database_connection: &DatabaseConnection, ) -> contract_deployments::Model { - let contract_deployments = contract_deployments::Entity::find() + let contract_deployments = timeout( + Duration::from_secs(5), + contract_deployments::Entity::find() + .all(database_connection) + ) + .await + .map_err(|_| "Database operation timed out")? .all(database_connection) .await .expect("error while retrieving contract deployments");
🧹 Nitpick comments (27)
eth-bytecode-db/verifier-alliance-database/src/internal.rs (4)
289-321: Add documentation comments for the new retrieval functions.While the functions are well-implemented, they would benefit from documentation comments explaining their purpose, parameters, and return values. This would improve code maintainability and help other developers understand the intended usage.
Add documentation comments like this:
+/// Retrieves all contract deployments for a given chain ID and contract address. +/// +/// # Arguments +/// * `database_connection` - The database connection +/// * `chain_id` - The chain ID to filter by +/// * `contract_address` - The contract address to filter by +/// +/// # Returns +/// A vector of contract deployment models matching the criteria pub async fn retrieve_contract_deployments_by_chain_id_and_address<C: ConnectionTrait>(
441-469: Consider optimizing database queries for source retrieval.The current implementation makes a separate database query for each source model. This could be optimized by using a join operation or fetching all sources in a single query.
Consider refactoring to use a join operation:
- let compiled_contract_source_models = compiled_contracts_sources::Entity::find() - .filter(compiled_contracts_sources::Column::CompilationId.eq(compilation_id)) - .all(database_connection) - .await - .context("select from \"compiled_contracts_sources\"")?; - - let mut sources = BTreeMap::new(); - for compiled_contract_source_model in compiled_contract_source_models { - let source_model = - sources::Entity::find_by_id(compiled_contract_source_model.source_hash.clone()) - .one(database_connection) - .await - .context("select from \"sources\"")? - .ok_or_else(|| { - anyhow!( - "source hash does not exist: {}", - compiled_contract_source_model.source_hash.to_hex() - ) - })?; - - sources.insert(compiled_contract_source_model.path, source_model.content); - } + let source_models = compiled_contracts_sources::Entity::find() + .filter(compiled_contracts_sources::Column::CompilationId.eq(compilation_id)) + .find_also_related(sources::Entity) + .all(database_connection) + .await + .context("select from \"compiled_contracts_sources\" join \"sources\"")?; + + let sources = source_models + .into_iter() + .map(|(compiled_source, source)| { + let source = source.ok_or_else(|| { + anyhow!( + "source hash does not exist: {}", + compiled_source.source_hash.to_hex() + ) + })?; + Ok((compiled_source.path, source.content)) + }) + .collect::<Result<BTreeMap<_, _>, Error>>()?;
501-502: Enhance the database constraint documentation.The comment about database constraints could be more descriptive to help developers understand the specific constraints being relied upon.
Improve the comment like this:
- // We can safely unwrap thanks to `verified_contracts_creation_match_integrity` and `verified_contracts_runtime_match_integrity` database constraints + // Database constraints ensure data integrity: + // - `verified_contracts_creation_match_integrity`: When creation_match is true, ensures creation_metadata_match, creation_transformations, and creation_values are present + // - `verified_contracts_runtime_match_integrity`: When runtime_match is true, ensures runtime_metadata_match, runtime_transformations, and runtime_values are present
481-567: Consider adding error context for JSON parsing operations.While the error handling is generally good, the JSON parsing operations could benefit from more specific error context to help with debugging.
Add more specific error context:
- serde_json::from_value(transformations).context("parsing match transformations")?; + serde_json::from_value(transformations).with_context(|| format!("failed to parse match transformations: {}", transformations))?;eth-bytecode-db/verifier-alliance-database-tests/src/lib.rs (2)
19-24: Consider using a more efficient and deterministic database prefix generation.The current approach of using backtrace for generating database prefixes has several drawbacks:
- Performance overhead from capturing backtraces
- Potential for very long database names
- Non-deterministic prefixes that could complicate debugging
Consider using a simpler approach like:
- let database_prefix = format!( - "{}_{}", - std::backtrace::Backtrace::force_capture(), - test_case.test_case_name - ) - .replace(|c: char| !c.is_ascii_alphanumeric(), "_"); + let database_prefix = format!( + "test_{}_{}", + test_case.test_case_name, + uuid::Uuid::new_v4() + );
58-59: Consider re-enabling test logging.The commented out
#[test_log::test]suggests that test logging was previously used. Consider re-enabling it to help with test debugging and failure analysis.- // #[test_log::test(tokio::test)] - #[tokio::test] + #[test_log::test(tokio::test)]eth-bytecode-db/verifier-alliance-database-tests/src/test_case.rs (1)
Line range hint
83-117: Consider splitting validate_final_database_state into smaller functions.The method is quite long and handles multiple validations. Consider extracting the validation logic into separate private methods for better maintainability and readability.
+ async fn validate_code(&self, database_connection: &DatabaseConnection, contract: &contracts::Model) { + self.validate_code_value( + database_connection, + contract.creation_code_hash.clone(), + self.deployed_creation_code.clone(), + ) + .await; + self.validate_code_value( + database_connection, + contract.runtime_code_hash.clone(), + self.deployed_runtime_code.clone(), + ) + .await; + }eth-bytecode-db/eth-bytecode-db-server/tests/verifier_alliance_integration/main.rs (5)
11-11: Consider using a more secure-looking placeholder API key.While this is a test constant, using a more realistic API key format (e.g., a UUID or a base64 string) would make the tests more representative of production scenarios.
-const API_KEY: &str = "some api key"; +const API_KEY: &str = "test_3ff06cd9-f2c3-4f48-8eba-d5fd8456ed91";
13-17: Add documentation for SetupResult struct.Consider adding documentation comments to explain the purpose of this struct and its role in the test infrastructure.
#[derive(Debug, Clone)] +/// Represents the result of setting up the test environment. +/// Contains the HTTP client configured to interact with the test server. pub struct SetupResult { + /// HTTP client configured with the test server URL and API key. service_client: http_client::Client, }
18-26: Improve database prefix generation robustness.The current approach of using backtrace for generating unique database names could be fragile:
- Backtraces might change with compiler versions or optimizations
- The resulting string could be very long
- No maximum length check for database name
Consider using a more deterministic approach:
- let bytecode_database_prefix = format!( - "{}_{}", - std::backtrace::Backtrace::force_capture(), - test_case_name - ) - .replace(|c: char| !c.is_ascii_alphanumeric(), "_"); + let bytecode_database_prefix = format!( + "test_{}_{}_{}", + test_case_name, + std::process::id(), + chrono::Utc::now().timestamp() + ) + .chars() + .filter(|c| c.is_ascii_alphanumeric() || *c == '_') + .take(63) + .collect::<String>();
33-35: Document why metrics, tracing, and jaeger are disabled.Add comments explaining why these features are disabled in the test environment.
+ // Disable monitoring features in test environment to reduce noise and improve test performance settings.metrics.enabled = false; settings.tracing.enabled = false; settings.jaeger.enabled = false;
18-54: Add comprehensive documentation for the setup function.The setup function lacks documentation explaining its purpose, requirements, and usage.
+/// Sets up a test environment for verifier alliance integration tests. +/// +/// # Arguments +/// +/// * `test_case_name` - Unique identifier for the test case, used in database naming +/// * `alliance_database` - Database guard for the verifier alliance database +/// +/// # Returns +/// +/// Returns a `SetupResult` containing the configured service client. +/// +/// # Panics +/// +/// Panics if: +/// - The verifier URL is invalid +/// - The server fails to initialize +/// - The HTTP client fails to initialize async fn setup(test_case_name: &str, alliance_database: TestDbGuard) -> SetupResult {eth-bytecode-db/eth-bytecode-db-server/tests/verifier_alliance_integration/solidity_verify.rs (2)
5-16: Enhance error handling and debugging capabilities.While the function structure is good, consider these improvements:
- Replace
expect("sending verification request failed")with more descriptive error handling that includes the test case name and request details.- Add debug logging to help diagnose test failures.
async fn initialize(database: TestDbGuard, test_case: TestCase) { + tracing::debug!("Initializing test case: {}", test_case.test_case_name); let setup_result = crate::setup(&test_case.test_case_name, database).await; let request = helpers::eth_bytecode_db_request(&test_case); + tracing::debug!("Sending verification request: {:?}", request); let _verify_response = http_client::solidity_verifier_client::verify_standard_json( &setup_result.service_client, request, ) .await - .expect("sending verification request failed"); + .unwrap_or_else(|e| panic!( + "Failed to verify test case '{}': {:#}", + test_case.test_case_name, e + )); + tracing::debug!("Verification completed successfully"); }
39-49: Add input validation for request creation.The
eth_bytecode_db_requestfunction should validate inputs before creating the request:
- Verify bytecode is not empty
- Validate compiler version format
- Check bytecode format
pub fn eth_bytecode_db_request( test_case: &TestCase, ) -> eth_bytecode_db_v2::VerifySolidityStandardJsonRequest { + // Validate inputs + if test_case.deployed_creation_code.is_empty() { + panic!("Creation bytecode cannot be empty"); + } + if test_case.version.trim().is_empty() { + panic!("Compiler version cannot be empty"); + } + eth_bytecode_db_v2::VerifySolidityStandardJsonRequest { bytecode: test_case.deployed_creation_code.to_hex(), bytecode_type: eth_bytecode_db_v2::BytecodeType::CreationInput.into(),eth-bytecode-db/eth-bytecode-db-server/tests/verifier_alliance_integration/solidity_batch_import.rs (1)
5-17: Consider adding error assertions for negative test cases.The
initializefunction only tests the happy path. Consider adding test cases for invalid inputs, such as mismatched contract sizes or invalid compiler versions.eth-bytecode-db/eth-bytecode-db-proto/tests/http_client.rs (1)
244-282: Consider adding negative test cases.While the happy path is tested, consider adding test cases for:
- Invalid requests (malformed data)
- Error responses from the service
- Rate limiting scenarios
Example test case structure:
#[tokio::test] async fn verifier_alliance_service_error_handling() { let mock_service = { let mut mock_service = mock::MockVerifierAllianceService::default(); mock_service .expect_batch_import_solidity_multi_part() .returning(|_| Err(Status::invalid_argument("Invalid request").into())); mock_service }; // ... test error handling }eth-bytecode-db/verifier-alliance-database/tests/integration/main.rs (1)
6-10: Consider handling deserialization errors gracefully.The
from_jsonmacro usesunwrap()which will panic if deserialization fails. For test code, this might lead to unclear test failures.Consider this safer alternative:
macro_rules! from_json { ($($json:tt)+) => { - serde_json::from_value(serde_json::json!($($json)+)).unwrap() + serde_json::from_value(serde_json::json!($($json)+)) + .expect("Failed to deserialize JSON value") }; }eth-bytecode-db/verifier-alliance-database/tests/integration/transformations.rs (1)
32-45: Consider adding validation for test case data.The contract deployment data construction could benefit from input validation to catch potential test data issues early.
Consider adding checks for:
- Non-empty address and transaction hash
- Valid block number and transaction index ranges
eth-bytecode-db/verifier-alliance-database/tests/integration/internal_compiled_contracts.rs (2)
26-26: Consider extracting common compiler settings.The compiler settings JSON is duplicated between test cases. Consider extracting it to a constant or helper function for better maintainability.
const DEFAULT_COMPILER_SETTINGS: &str = r#"{"evmVersion":"paris","libraries":{},"optimizer":{"enabled":true,"runs":200},"outputSelection":{"*":{"*":["*"]}},"remappings":[],"viaIR":false}"#; // Usage compiler_settings: from_json!(DEFAULT_COMPILER_SETTINGS),Also applies to: 70-70
29-32: Consider using more realistic test data.The current test JSON values (
"abi": "value", etc.) are oversimplified. Consider using more realistic contract artifacts to better represent real-world scenarios.eth-bytecode-db/verifier-alliance-database/tests/integration/verified_contracts.rs (1)
190-193: Replace placeholder JSON values with realistic test data.The current JSON values (
"value") are placeholders. Consider using realistic ABI, documentation, and storage layout data to make the tests more meaningful.eth-bytecode-db/eth-bytecode-db/src/verification/types.rs (2)
347-348: Enhance error messages for missing artifacts.The error message "missing compilation_artifacts" could be more descriptive. Consider including the contract name or other identifying information to help with debugging.
- .ok_or_else(|| anyhow::anyhow!("missing compilation_artifacts"))?; + .ok_or_else(|| anyhow::anyhow!("missing compilation_artifacts for contract '{}'", source.contract_name))?;
364-371: Add type annotations for deserialized JSON values.The JSON deserialization could fail silently with incorrect types. Consider adding type annotations to make the expected types explicit and catch type mismatches at compile time.
- compilation_artifacts: serde_json::from_value(compilation_artifacts) + compilation_artifacts: serde_json::from_value::<CompilationArtifacts>(compilation_artifacts) .context("deserializing compilation_artifacts")?, - creation_code_artifacts: serde_json::from_value(creation_code_artifacts) + creation_code_artifacts: serde_json::from_value::<CreationCodeArtifacts>(creation_code_artifacts) .context("deserializing creation_code_artifacts")?, - runtime_code_artifacts: serde_json::from_value(runtime_code_artifacts) + runtime_code_artifacts: serde_json::from_value::<RuntimeCodeArtifacts>(runtime_code_artifacts) .context("deserializing runtime_code_artifacts")?,Where
CompilationArtifacts,CreationCodeArtifacts, andRuntimeCodeArtifactsare the expected types from theverifier_alliance_databasecrate.eth-bytecode-db/eth-bytecode-db/src/search/alliance_db.rs (1)
7-8: Consider using a genericConnectionTraitinstead ofDatabaseConnectionBy changing
find_contractto accept a&DatabaseConnectioninstead of a genericC: ConnectionTrait, the function becomes less flexible and harder to test. Using a genericConnectionTraitallows for easier mocking and supports different types of database connections, enhancing reusability.eth-bytecode-db/eth-bytecode-db/src/verification/handlers/mod.rs (3)
467-474: Fix typo in error messageThe error message uses a double negative: "contract metadata does not correspond neither to genesis nor regular contract". Consider rephrasing to: "contract metadata does not correspond to either genesis or regular contract"
Apply this diff:
anyhow::bail!( - "parsing contract metadata: contract metadata does not correspond neither to genesis nor regular contract: creation_code_exists={}, block_number_exists={}, transaction_hash_exists={}, transaction_index_exists={}, deployer_exists={}", + "parsing contract metadata: contract metadata does not correspond to either genesis or regular contract: creation_code_exists={}, block_number_exists={}, transaction_hash_exists={}, transaction_index_exists={}, deployer_exists={}", deployment_data.creation_code.is_some(), deployment_data.block_number.is_some(), deployment_data.transaction_hash.is_some(), deployment_data.transaction_index.is_some(), deployment_data.deployer.is_some(), )
537-539: Correct double negative in error messageThe error message contains a double negative: "Neither creation code nor runtime code have not matched". This can be confusing. Consider rephrasing to: "Neither creation code nor runtime code have matched"
Apply this diff to fix the typo:
(None, None) => Err(anyhow::anyhow!( - "Neither creation code nor runtime code have not matched" + "Neither creation code nor runtime code have matched" )),
486-506: Refactor duplicated deserialization codeThe code for deserializing
compilation_artifacts,creation_code_artifacts, andruntime_code_artifactsis similar and can be refactored to reduce duplication.Consider creating a helper function to handle the deserialization:
fn deserialize_artifacts<T>( value: &serde_json::Value, artifact_name: &str, ) -> Result<T, anyhow::Error> where T: serde::de::DeserializeOwned, { serde::Deserialize::deserialize(value) .with_context(|| format!("deserializing {} artifact", artifact_name)) } ... // Then, replace the code as follows: let compilation_artifacts: CompilationArtifacts = deserialize_artifacts( compilation_artifacts_value, "compilation_artifacts", )?; let creation_code_artifacts: CreationCodeArtifacts = deserialize_artifacts( creation_code_artifacts_value, "creation_code_artifacts", )?; let runtime_code_artifacts: RuntimeCodeArtifacts = deserialize_artifacts( runtime_code_artifacts_value, "runtime_code_artifacts", )?;This refactoring improves maintainability and reduces code duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
eth-bytecode-db/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (52)
eth-bytecode-db/Cargo.toml(5 hunks)eth-bytecode-db/eth-bytecode-db-proto/src/http_client/client.rs(1 hunks)eth-bytecode-db/eth-bytecode-db-proto/src/http_client/mock.rs(6 hunks)eth-bytecode-db/eth-bytecode-db-proto/src/http_client/mod.rs(1 hunks)eth-bytecode-db/eth-bytecode-db-proto/tests/http_client.rs(2 hunks)eth-bytecode-db/eth-bytecode-db-server/Cargo.toml(2 hunks)eth-bytecode-db/eth-bytecode-db-server/src/server.rs(1 hunks)eth-bytecode-db/eth-bytecode-db-server/tests/alliance_test_cases/constructor_arguments.json(1 hunks)eth-bytecode-db/eth-bytecode-db-server/tests/alliance_test_cases/partial_match.json(2 hunks)eth-bytecode-db/eth-bytecode-db-server/tests/alliance_test_cases/partial_match_2.json(2 hunks)eth-bytecode-db/eth-bytecode-db-server/tests/alliance_test_cases/partial_match_double_auxdata.json(2 hunks)eth-bytecode-db/eth-bytecode-db-server/tests/database_search.rs(1 hunks)eth-bytecode-db/eth-bytecode-db-server/tests/integration/main.rs(1 hunks)eth-bytecode-db/eth-bytecode-db-server/tests/verification_test_helpers/verifier_alliance_types.rs(1 hunks)eth-bytecode-db/eth-bytecode-db-server/tests/verifier_alliance.rs(1 hunks)eth-bytecode-db/eth-bytecode-db-server/tests/verifier_alliance_integration/main.rs(1 hunks)eth-bytecode-db/eth-bytecode-db-server/tests/verifier_alliance_integration/solidity_batch_import.rs(1 hunks)eth-bytecode-db/eth-bytecode-db-server/tests/verifier_alliance_integration/solidity_verify.rs(1 hunks)eth-bytecode-db/eth-bytecode-db/Cargo.toml(1 hunks)eth-bytecode-db/eth-bytecode-db/src/search/alliance_db.rs(3 hunks)eth-bytecode-db/eth-bytecode-db/src/verification/db/mod.rs(0 hunks)eth-bytecode-db/eth-bytecode-db/src/verification/db/verifier_alliance_db.rs(0 hunks)eth-bytecode-db/eth-bytecode-db/src/verification/handlers/mod.rs(6 hunks)eth-bytecode-db/eth-bytecode-db/src/verification/mod.rs(0 hunks)eth-bytecode-db/eth-bytecode-db/src/verification/types.rs(3 hunks)eth-bytecode-db/eth-bytecode-db/src/verification/verifier_alliance/action_helpers.rs(0 hunks)eth-bytecode-db/eth-bytecode-db/src/verification/verifier_alliance/mod.rs(0 hunks)eth-bytecode-db/eth-bytecode-db/src/verification/verifier_alliance/parsers.rs(0 hunks)eth-bytecode-db/eth-bytecode-db/src/verification/verifier_alliance/transformations.rs(0 hunks)eth-bytecode-db/eth-bytecode-db/tests/alliance_test_cases/constructor_arguments.json(0 hunks)eth-bytecode-db/eth-bytecode-db/tests/alliance_test_cases/full_match.json(0 hunks)eth-bytecode-db/eth-bytecode-db/tests/alliance_test_cases/immutables.json(0 hunks)eth-bytecode-db/eth-bytecode-db/tests/alliance_test_cases/libraries_linked_by_compiler.json(0 hunks)eth-bytecode-db/eth-bytecode-db/tests/alliance_test_cases/libraries_manually_linked.json(0 hunks)eth-bytecode-db/eth-bytecode-db/tests/alliance_test_cases/metadata_hash_absent.json(0 hunks)eth-bytecode-db/eth-bytecode-db/tests/alliance_test_cases/partial_match.json(0 hunks)eth-bytecode-db/eth-bytecode-db/tests/alliance_test_cases/partial_match_double_auxdata.json(0 hunks)eth-bytecode-db/eth-bytecode-db/tests/verification_test_helpers/mod.rs(0 hunks)eth-bytecode-db/eth-bytecode-db/tests/verification_test_helpers/verifier_alliance_types.rs(0 hunks)eth-bytecode-db/eth-bytecode-db/tests/verifier_alliance.rs(0 hunks)eth-bytecode-db/verifier-alliance-database-tests/Cargo.toml(1 hunks)eth-bytecode-db/verifier-alliance-database-tests/src/lib.rs(1 hunks)eth-bytecode-db/verifier-alliance-database-tests/src/test_case.rs(2 hunks)eth-bytecode-db/verifier-alliance-database/Cargo.toml(1 hunks)eth-bytecode-db/verifier-alliance-database/src/internal.rs(4 hunks)eth-bytecode-db/verifier-alliance-database/src/lib.rs(2 hunks)eth-bytecode-db/verifier-alliance-database/src/types.rs(2 hunks)eth-bytecode-db/verifier-alliance-database/tests/integration/contract_deployments.rs(1 hunks)eth-bytecode-db/verifier-alliance-database/tests/integration/internal_compiled_contracts.rs(3 hunks)eth-bytecode-db/verifier-alliance-database/tests/integration/main.rs(1 hunks)eth-bytecode-db/verifier-alliance-database/tests/integration/transformations.rs(1 hunks)eth-bytecode-db/verifier-alliance-database/tests/integration/verified_contracts.rs(4 hunks)
💤 Files with no reviewable changes (18)
- eth-bytecode-db/eth-bytecode-db/tests/verification_test_helpers/mod.rs
- eth-bytecode-db/eth-bytecode-db/src/verification/db/mod.rs
- eth-bytecode-db/eth-bytecode-db/src/verification/mod.rs
- eth-bytecode-db/eth-bytecode-db/tests/alliance_test_cases/partial_match.json
- eth-bytecode-db/eth-bytecode-db/tests/alliance_test_cases/libraries_linked_by_compiler.json
- eth-bytecode-db/eth-bytecode-db/tests/alliance_test_cases/immutables.json
- eth-bytecode-db/eth-bytecode-db/tests/alliance_test_cases/partial_match_double_auxdata.json
- eth-bytecode-db/eth-bytecode-db/tests/alliance_test_cases/libraries_manually_linked.json
- eth-bytecode-db/eth-bytecode-db/tests/alliance_test_cases/metadata_hash_absent.json
- eth-bytecode-db/eth-bytecode-db/tests/alliance_test_cases/constructor_arguments.json
- eth-bytecode-db/eth-bytecode-db/tests/alliance_test_cases/full_match.json
- eth-bytecode-db/eth-bytecode-db/src/verification/verifier_alliance/mod.rs
- eth-bytecode-db/eth-bytecode-db/src/verification/verifier_alliance/transformations.rs
- eth-bytecode-db/eth-bytecode-db/tests/verification_test_helpers/verifier_alliance_types.rs
- eth-bytecode-db/eth-bytecode-db/src/verification/verifier_alliance/action_helpers.rs
- eth-bytecode-db/eth-bytecode-db/src/verification/verifier_alliance/parsers.rs
- eth-bytecode-db/eth-bytecode-db/tests/verifier_alliance.rs
- eth-bytecode-db/eth-bytecode-db/src/verification/db/verifier_alliance_db.rs
✅ Files skipped from review due to trivial changes (5)
- eth-bytecode-db/eth-bytecode-db-server/tests/alliance_test_cases/constructor_arguments.json
- eth-bytecode-db/eth-bytecode-db-server/tests/alliance_test_cases/partial_match.json
- eth-bytecode-db/eth-bytecode-db-server/tests/integration/main.rs
- eth-bytecode-db/eth-bytecode-db-server/tests/alliance_test_cases/partial_match_double_auxdata.json
- eth-bytecode-db/eth-bytecode-db-server/tests/verifier_alliance.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Linting / lint
- GitHub Check: Unit, doc and integration tests
🔇 Additional comments (40)
eth-bytecode-db/eth-bytecode-db-server/tests/alliance_test_cases/partial_match_2.json (2)
Line range hint
1-150: LGTM! Schema updates are consistently applied.The changes are well-structured and maintain consistency across all sections:
- Source code parameter renaming
- ABI updates
- Documentation updates
- Transformation rules
69-69: Verify schema compliance for the new "cborAuxdata" transformation reason.The change from "auxdata" to "cborAuxdata" is more precise and better reflects the CBOR-encoded nature of Solidity metadata. Let's verify this change against the schema specification.
Also applies to: 84-84
✅ Verification successful
The "cborAuxdata" transformation reason is used consistently across test cases
All test files in the alliance test cases consistently use "cborAuxdata" for CBOR-encoded metadata transformations, along with other standardized reasons like "immutable", "constructorArguments", and "library". The change makes the transformation reason more precise and appears to be part of a systematic update.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if other test cases use consistent transformation reasons # and verify against schema definition if available # Check other test cases for consistency echo "Checking other test cases for transformation reason consistency..." rg -l '"reason":\s*"(auxdata|cborAuxdata)"' eth-bytecode-db/eth-bytecode-db-server/tests/alliance_test_cases/ # Look for schema definition echo "Looking for schema definition..." fd -e json -e yaml -e yml schema eth-bytecode-dbLength of output: 649
Script:
#!/bin/bash # Check the actual content of test files for "reason" field values echo "Checking actual reason values in test files..." rg -A 1 '"reason":\s*"[^"]*"' eth-bytecode-db/eth-bytecode-db-server/tests/alliance_test_cases/ # Look for validation code that might define allowed values echo -e "\nLooking for validation code..." rg -l "reason.*auxdata|cborAuxdata" eth-bytecode-db/eth-bytecode-db-server/src/Length of output: 3097
eth-bytecode-db/verifier-alliance-database/src/internal.rs (1)
9-9: LGTM! Clean and well-organized imports.The new imports are properly organized and necessary for the added functionality.
Also applies to: 12-14
eth-bytecode-db/verifier-alliance-database-tests/src/lib.rs (1)
38-40: 🛠️ Refactor suggestionAdd error handling for test case loading.
The current implementation will panic if the test case file is missing or malformed. Consider adding proper error handling and validation.
- const TEST_CASE_CONTENT: &str = - include_str!(concat!("../test_cases/", stringify!($test_name), ".json")); - TestCase::from_content(stringify!($test_name), TEST_CASE_CONTENT) + const TEST_CASE_PATH: &str = concat!("../test_cases/", stringify!($test_name), ".json"); + const TEST_CASE_CONTENT: &str = include_str!(TEST_CASE_PATH); + match TestCase::from_content(stringify!($test_name), TEST_CASE_CONTENT) { + Ok(test_case) => test_case, + Err(e) => panic!("Failed to load test case from {}: {}", TEST_CASE_PATH, e), + }Likely invalid or redundant comment.
eth-bytecode-db/eth-bytecode-db-server/tests/verifier_alliance_integration/main.rs (2)
1-10: LGTM! Clean module structure and relevant imports.The module organization and imports are well-structured for integration testing purposes.
39-40: Verify handling of authorized keys in logs.Ensure that the API key is not logged during test execution, as it could expose sensitive information in CI logs.
eth-bytecode-db/eth-bytecode-db-server/tests/verifier_alliance_integration/solidity_verify.rs (2)
1-4: LGTM! Dependencies are well-organized.The imports are appropriate for integration testing, providing necessary test utilities and database handling.
18-30: Document test scenarios and verify commented-out tests.The test suite covers various scenarios, but:
- Test cases lack documentation explaining their purpose and expected behavior.
- Two test cases are commented out without explanation:
libraries_manually_linkedmetadata_hash_absenteth-bytecode-db/eth-bytecode-db-server/tests/verification_test_helpers/verifier_alliance_types.rs (3)
185-185: LGTM! Well-structured field addition.The new
cbor_auxdatafield is well-designed with appropriate types for optional, ordered storage of named auxdata sections.
193-193: LGTM! Improved error handling.Using
unwrap_or_default()is a safer approach that gracefully handles the case when no CBOR auxdata exists.
Line range hint
1-248: Verify test coverage migration.According to the PR summary, this file is being removed as part of restructuring. Please ensure that the test functionality provided by these types has been properly migrated to the new
verifier-alliance-database-testspackage to maintain test coverage.✅ Verification successful
Test functionality successfully migrated.
The test functionality has been properly migrated to
eth-bytecode-db/verifier-alliance-database-tests/src/test_case.rswith all essential features preserved and enhanced with additional database validation capabilities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that test functionality has been migrated # Check for similar structs/types in the new package echo "Searching for TestCase struct in new location..." rg -l "struct TestCase" "verifier-alliance-database-tests/" # Look for similar test helper functionality echo "Searching for similar test helper functionality..." rg -l "verify_response::ExtraData|BytecodePart|CborAuxdata" "verifier-alliance-database-tests/"Length of output: 521
Script:
#!/bin/bash # First, find all directories that might contain the migrated tests echo "Searching for potential test directories..." fd -t d -H "alliance.*test|test.*alliance" # Search for similar test structures and functionality across the codebase echo -e "\nSearching for TestCase struct..." rg -l "struct TestCase" echo -e "\nSearching for similar test helper functionality..." rg -l "verify_response::ExtraData|BytecodePart|CborAuxdata" # Look for any new test files related to alliance verification echo -e "\nSearching for alliance verification test files..." fd -t f ".*alliance.*verify.*test.*\.rs$|.*verify.*alliance.*test.*\.rs$"Length of output: 4287
Script:
#!/bin/bash # Check the content of the new test case implementation echo "Content of new test case implementation:" cat "eth-bytecode-db/verifier-alliance-database-tests/src/test_case.rs"Length of output: 14219
eth-bytecode-db/eth-bytecode-db-proto/src/http_client/mod.rs (1)
9-9: LGTM!The
verifier_alliance_clientis correctly added to the exports, maintaining alphabetical order.eth-bytecode-db/eth-bytecode-db-server/tests/verifier_alliance_integration/solidity_batch_import.rs (1)
19-31: Uncommented test cases need verification.There are commented-out test cases (
libraries_manually_linked,metadata_hash_absent). Please verify if these are intentionally disabled or if they need to be implemented.✅ Verification successful
Test cases are intentionally disabled pending fixes
The commented-out test cases are implemented but intentionally disabled:
metadata_hash_absent: Disabled due to a known issue with auxdata retrieval that needs to be fixedlibraries_manually_linked: Consistently disabled across integration tests🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if these test cases exist in the test suite rg -A 5 "libraries_manually_linked|metadata_hash_absent" -g "!target/*"Length of output: 6183
eth-bytecode-db/eth-bytecode-db-server/src/server.rs (1)
93-103: LGTM! Well-structured database configuration.The refactoring to use
DatabaseSettingsimproves code organization and maintainability. The settings are clearly structured with separate concerns for connection URL, options, and database operations.eth-bytecode-db/eth-bytecode-db-proto/src/http_client/mock.rs (4)
80-90: LGTM! Mock implementation follows established patterns.The
VerifierAllianceServicemock is well-structured and consistent with other mock services in the file, properly implementing the required trait methods.
98-98: LGTM! Struct changes maintain consistency.The addition of
verifier_alliance_servicefield and its initialization follow the established pattern of other service fields in the struct.Also applies to: 108-108
138-144: LGTM! Builder method follows established patterns.The
verifier_alliance_servicebuilder method correctly implements the builder pattern with proper Arc wrapping and maintains the fluent interface.
154-154: LGTM! Service configuration is properly integrated.The integration of verifier alliance service in the start method follows the established pattern of other services with proper cloning and conditional configuration.
Also applies to: 169-172
eth-bytecode-db/eth-bytecode-db-proto/src/http_client/client.rs (1)
268-290: LGTM! Client implementation is secure and consistent.The
verifier_alliance_clientmodule follows best practices:
- Proper error handling with Result type
- Secure API key header inclusion
- RESTful API path conventions
- Consistent implementation pattern with other client modules
eth-bytecode-db/verifier-alliance-database-tests/Cargo.toml (1)
1-19: LGTM! Package configuration is well-structured.The Cargo.toml configuration:
- Properly uses workspace-managed dependencies
- Includes appropriate test-specific dependencies
- Follows Rust package conventions
eth-bytecode-db/eth-bytecode-db-server/Cargo.toml (1)
Line range hint
30-44: LGTM! Test dependencies added appropriately.The new dev dependencies enhance the testing capabilities and are properly configured to use workspace-level version management.
eth-bytecode-db/eth-bytecode-db/Cargo.toml (1)
38-38: LGTM! Core dependency added appropriately.The addition of
verifier-alliance-databasealigns with the PR objective and maintains workspace-level version management.eth-bytecode-db/Cargo.toml (3)
12-12: LGTM! Workspace member added appropriately.The addition of
verifier-alliance-database-teststo workspace members is properly configured.
21-24: LGTM! Dependencies properly configured with local paths.The verifier alliance related dependencies are correctly configured using local paths within the workspace.
33-33: Verify version compatibility of updated dependencies.Please ensure compatibility between:
- Updated
blockscout-service-launcher(0.16.2)- New
async-backtrace(0.2.7)- New
test-log(0.2.16)- Updated
verification-common(rev: 5481500)Also applies to: 37-37, 70-70, 79-79
✅ Verification successful
Dependencies verified successfully
The updated dependencies are compatible:
- async-backtrace and test-log are test-only dependencies
- blockscout-service-launcher 0.16.2 and verification-common (rev: 5481500) have passing CI checks
- No version conflicts detected in the dependency tree
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential compatibility issues with the updated dependencies # Check if blockscout-service-launcher 0.16.2 has any known issues gh api graphql -f query=' { repository(owner: "blockscout", name: "blockscout-rs") { release(tagName: "blockscout-service-launcher-v0.16.2") { description url } } }' # Check if verification-common at revision 5481500 is stable gh api repos/blockscout/blockscout-rs/commits/5481500/check-runsLength of output: 6353
Script:
#!/bin/bash # Check dependency declarations and constraints fd Cargo.toml --exec cat {} \; # Look for usage of these dependencies rg -A 2 "use.*async_backtrace|use.*test_log|use.*blockscout_service_launcher"Length of output: 65709
eth-bytecode-db/verifier-alliance-database/Cargo.toml (2)
16-17: Breaking changes in core dependencies.The removal of
-v1suffix fromverification-commonandverifier-alliance-entitydependencies indicates a breaking change. This aligns with the PR title's!marker.Let's verify the impact of these dependency changes:
✅ Verification successful
Breaking changes have been properly propagated across the workspace
The removal of
-v1suffix from dependencies is consistent across the entire workspace, with no remaining references to the old versioned dependencies. All affected packages have been updated to use the new non-versioned variants through workspace-level version management.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other packages that might be affected by the breaking changes # Search for any remaining v1 references that might need updating rg -l "verification-common-v1|verifier-alliance-entity-v1" # Check Cargo.toml files in workspace for potentially affected dependencies fd Cargo.toml | xargs rg "verification-common|verifier-alliance-entity"Length of output: 1676
25-26: Verify test infrastructure changes.The addition of
verifier-alliance-database-testsas a separate package and the update toverifier-alliance-migrationsuggests a restructuring of the testing framework.Let's verify the test infrastructure setup:
✅ Verification successful
Test infrastructure changes are properly implemented.
The changes introduce a well-structured test framework with:
- Comprehensive test cases for various contract verification scenarios
- Database state validation utilities
- Proper migration system integration with schema versioning
The infrastructure is correctly set up and ready for use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new test infrastructure setup # Check if the new test package exists and has the expected structure fd -t f . "verifier-alliance-database-tests" # Look for migration tests that might need to be updated rg -l "verifier-alliance-migration-v1"Length of output: 203
Script:
#!/bin/bash # Find all Cargo.toml files and their content fd -t f "Cargo.toml$" -x cat {} # Search for test files rg -l "mod tests" -g "*.rs" # Look for migration-related files fd -t f -e rs . | rg -l "migration"Length of output: 66333
Script:
#!/bin/bash # Look for test files and configurations rg -t rust "test|fixture" "verifier-alliance-database-tests" -l # Check migration files rg -t rust "migration" "verifier-alliance-migration" -l # Look for test dependencies and configurations rg -l "verifier-alliance-(database-tests|migration)" -g "Cargo.toml"Length of output: 589
Script:
#!/bin/bash # Check test package contents cat eth-bytecode-db/verifier-alliance-database-tests/Cargo.toml find eth-bytecode-db/verifier-alliance-database-tests/src -type f -name "*.rs" -exec cat {} \; # Check migration setup cat eth-bytecode-db/eth-bytecode-db/verifier-alliance-migration/Cargo.toml find eth-bytecode-db/eth-bytecode-db/verifier-alliance-migration/src -type f -name "*.rs" -exec cat {} \;Length of output: 19364
eth-bytecode-db/verifier-alliance-database/tests/integration/contract_deployments.rs (1)
5-5: LGTM! Import path updated correctly.The change from versioned to non-versioned import aligns with the PR objective of integrating verifier alliance schema v1.
eth-bytecode-db/verifier-alliance-database/tests/integration/transformations.rs (2)
4-19: LGTM! Well-structured initialization function.The initialization function is well-organized with clear error messages and proper separation of concerns.
47-84: LGTM! Comprehensive verified contract data construction.The helper function properly handles all required fields and includes appropriate error messages for invalid inputs.
eth-bytecode-db/verifier-alliance-database/tests/integration/internal_compiled_contracts.rs (1)
37-39: LGTM! Proper test coverage for code artifacts.The test cases properly cover all artifact types with appropriate JSON structures.
Also applies to: 43-46
eth-bytecode-db/verifier-alliance-database/tests/integration/verified_contracts.rs (4)
1-1: LGTM! Import paths updated to use stable API version.The changes align with the PR objective of integrating verifier_alliance schema v1 by removing version suffixes from imports.
Also applies to: 6-14
198-207: Validate source maps and reference formats.The source maps and references follow expected formats:
- Source maps are strings
- Link references contain library information with length and start offset
- CBOR auxdata contains value and offset
- Immutable references contain length and start offset
Line range hint
112-145: Verify hex value lengths match Ethereum specifications.The hex values used in the test match the expected lengths:
- Immutable values: 32 bytes (0x32)
- Library addresses: 20 bytes (0x20)
- CBOR auxdata: 32 bytes (0x32)
- Constructor arguments: variable length (0x01020304)
Run this script to verify the hex values:
✅ Verification successful
All hex values follow Ethereum specifications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that hex values match Ethereum specifications # Test: Check for any non-standard length addresses rg -U --multiline '(?:decode_hex\(")(0x[a-fA-F0-9]{40})' -g '!{*.md,*.txt}' # Test: Check for any non-standard length hashes rg -U --multiline '(?:decode_hex\(")(0x[a-fA-F0-9]{64})' -g '!{*.md,*.txt}'Length of output: 3756
188-188: Verify compiler settings match supported EVM versions.The compiler settings use "paris" as the EVM version. Ensure this is the intended target version for the verifier alliance schema.
Run this script to check supported EVM versions:
✅ Verification successful
The "paris" EVM version is valid and supported by the verifier alliance schema.
The version is actively used in the codebase, including:
- Proxy verifier server services
- Verifier alliance database test fixtures
- Standard compiler settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for EVM version usage across the codebase # Test: Look for EVM version definitions or validations rg -A 5 'evmVersion|evm_version' -g '*.{rs,toml}'Length of output: 83246
Script:
#!/bin/bash # Search for "paris" EVM version references and version validation code echo "=== Searching for paris EVM version ===" rg -i "paris" -g '!*.{log,lock}' --type rust echo -e "\n=== Looking for EVM version validation code ===" rg "impl.*EvmVersion|enum.*EvmVersion" --type rust echo -e "\n=== Searching for documentation about supported versions ===" fd "README|CHANGELOG" --type f --exec rg -i "evm.?version"Length of output: 3630
eth-bytecode-db/eth-bytecode-db/src/verification/types.rs (2)
8-8: LGTM!The import statement is correctly placed and imports the necessary types for the new implementations.
138-146: LGTM!The
Fromimplementation provides a complete and correct mapping betweenCompiledContractLanguageandSourceTypevariants.eth-bytecode-db/verifier-alliance-database/src/types.rs (1)
166-173: New structRetrievedVerifiedContractlooks goodThe addition of the
RetrievedVerifiedContractstruct enhances metadata tracking with creation and update timestamps, as well as user information. The implementation appears correct and aligns with the intended functionality.eth-bytecode-db/verifier-alliance-database/src/lib.rs (1)
150-154: Review logic for handling multiple contract deploymentsThe function processes only the most recent
contract_deployment_modelafter sorting, ignoring others. If the intention is to retrieve all verified contracts associated with all deployments of an address, consider iterating over allcontract_deployment_models.To verify whether there are other deployments being ignored, run the following script:
Replace
<chain_id>and<contract_address_in_hex>with appropriate values.eth-bytecode-db/eth-bytecode-db/src/verification/handlers/mod.rs (1)
433-452:⚠️ Potential issueEnsure
runtime_codeis present before using itIn the
save_deployment_datafunction, when constructingInsertContractDeployment, theruntime_codefield is used without checking if it isSome. This can lead to a panic ifruntime_codeisNone. Please ensure thatruntime_codeis present before using it in both arms of the match statement.Apply this diff to fix the issue:
AllianceContract { creation_code: Some(creation_code), block_number: Some(block_number), transaction_hash: Some(transaction_hash), transaction_index: Some(transaction_index), deployer: Some(deployer), + runtime_code: Some(runtime_code), .. } => InsertContractDeployment::Regular { chain_id, address: deployment_data.contract_address.to_vec(), transaction_hash: transaction_hash.to_vec(), block_number: block_number .try_into() .context("parsing contract metadata: invalid block_number")?, transaction_index: transaction_index .try_into() .context("parsing contract metadata: invalid transaction_index")?, deployer: deployer.to_vec(), creation_code: creation_code.to_vec(), - runtime_code: deployment_data.runtime_code.to_vec(), + runtime_code: runtime_code.to_vec(), }, AllianceContract { creation_code: None, block_number: None, transaction_hash: None, transaction_index: None, deployer: None, + runtime_code: Some(runtime_code), .. } => InsertContractDeployment::Genesis { chain_id, address: deployment_data.contract_address.to_vec(), - runtime_code: deployment_data.runtime_code.to_vec(), + runtime_code: runtime_code.to_vec(), },Also applies to: 454-464
⛔ Skipped due to learnings
Learnt from: rimrakhimov PR: blockscout/blockscout-rs#1134 File: eth-bytecode-db/verifier-alliance-database/src/internal.rs:270-273 Timestamp: 2024-12-12T16:19:10.540Z Learning: In `eth-bytecode-db/verifier-alliance-database/src/types.rs`, the `RetrieveContractDeployment` struct guarantees that at least one of `transaction_hash` or `runtime_code` is always `Some`, enforced by its constructors. This invariant ensures that functions using this struct can safely assume one of these fields is present without additional checks.
eth-bytecode-db/eth-bytecode-db-server/tests/verifier_alliance_integration/main.rs
Outdated
Show resolved
Hide resolved
eth-bytecode-db/eth-bytecode-db-server/tests/verifier_alliance_integration/solidity_verify.rs
Show resolved
Hide resolved
eth-bytecode-db/eth-bytecode-db-server/tests/verifier_alliance_integration/solidity_verify.rs
Show resolved
Hide resolved
...ecode-db/eth-bytecode-db-server/tests/verifier_alliance_integration/solidity_batch_import.rs
Show resolved
Hide resolved
rimrakhimov
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
eth-bytecode-db/eth-bytecode-db/src/verification/types.rs (1)
327-376: Well-structured implementation with robust error handling.The conversion implementation is thorough and handles all edge cases appropriately. The error messages are clear and the use of anyhow for error handling is good practice.
Consider extracting the compiler and language mapping into a separate function to improve readability:
impl TryFrom<DatabaseReadySource> for verifier_alliance_database::CompiledContract { type Error = anyhow::Error; fn try_from(source: DatabaseReadySource) -> Result<Self, Self::Error> { - let (compiler, language) = match source.source_type { - SourceType::Solidity => ( - CompiledContractCompiler::Solc, - CompiledContractLanguage::Solidity, - ), - SourceType::Vyper => ( - CompiledContractCompiler::Vyper, - CompiledContractLanguage::Vyper, - ), - SourceType::Yul => ( - CompiledContractCompiler::Solc, - CompiledContractLanguage::Yul, - ), - }; + let (compiler, language) = get_compiler_and_language(source.source_type); validate_name_part(&source.file_name, "file_name")?; // ... rest of the implementation } } +fn get_compiler_and_language(source_type: SourceType) -> (CompiledContractCompiler, CompiledContractLanguage) { + match source_type { + SourceType::Solidity => ( + CompiledContractCompiler::Solc, + CompiledContractLanguage::Solidity, + ), + SourceType::Vyper => ( + CompiledContractCompiler::Vyper, + CompiledContractLanguage::Vyper, + ), + SourceType::Yul => ( + CompiledContractCompiler::Solc, + CompiledContractLanguage::Yul, + ), + } +}eth-bytecode-db/eth-bytecode-db-server/tests/verifier_alliance_integration/main.rs (4)
11-17: Make the test API key more explicit.Consider renaming the constant to make it clear this is a test-only API key.
-const API_KEY: &str = "some api key"; +const TEST_API_KEY: &str = "test_api_key_for_integration_tests";
19-25: Simplify database prefix generation.The current approach using backtrace for generating unique database names is complex and potentially fragile. Consider using a simpler approach with UUID or timestamp.
- let bytecode_database_prefix = format!( - "{}_{}", - std::backtrace::Backtrace::force_capture(), - test_case_name - ) - .replace(|c: char| !c.is_ascii_alphanumeric(), "_"); + let bytecode_database_prefix = format!( + "test_{}_{}", + test_case_name, + uuid::Uuid::new_v4() + );
27-49: Extract settings initialization into a separate function.The settings initialization block is complex and would be more maintainable as a separate function. Also, consider using the
?operator instead ofunwrap/expect.+async fn create_test_settings( + bytecode_database_url: String, + alliance_database_url: String, +) -> Result<(Settings, String), Box<dyn std::error::Error>> { + let verifier_url = Url::parse( + std::env::var("VERIFIER_URL") + .unwrap_or_else(|_| "https://http.sc-verifier-test.k8s-dev.blockscout.com/".to_string()) + .as_str(), + )?; + + let mut settings = Settings::default(bytecode_database_url, verifier_url); + let (server_settings, base) = test_server::get_test_server_settings(); + + settings.server = server_settings; + settings.metrics.enabled = false; + settings.tracing.enabled = false; + settings.jaeger.enabled = false; + + settings.verifier_alliance_database.enabled = true; + settings.verifier_alliance_database.url = alliance_database_url; + settings.authorized_keys = serde_json::from_value( + serde_json::json!({"blockscout": {"key": TEST_API_KEY}}) + )?; + + Ok((settings, base)) +}Then use it in the setup function:
- let (settings, base) = { - // ... existing settings initialization ... - }; + let (settings, base) = create_test_settings( + bytecode_database.db_url(), + alliance_database.db_url(), + ) + .await + .expect("Failed to create test settings");
53-56: Improve error handling in client initialization.The client initialization could benefit from better error handling and configuration validation.
- let client_config = - http_client::Config::new(base.to_string()).set_api_key(Some(API_KEY.to_string())); - let client = http_client::Client::new(client_config).await; + let client_config = http_client::Config::new(base.to_string()) + .set_api_key(Some(TEST_API_KEY.to_string())); + let client = http_client::Client::new(client_config) + .await + .expect("Failed to initialize client");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
eth-bytecode-db/eth-bytecode-db-server/tests/verifier_alliance_integration/main.rs(1 hunks)eth-bytecode-db/eth-bytecode-db/src/verification/types.rs(3 hunks)eth-bytecode-db/verifier-alliance-database/src/lib.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- eth-bytecode-db/verifier-alliance-database/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Unit, doc and integration tests
🔇 Additional comments (4)
eth-bytecode-db/eth-bytecode-db/src/verification/types.rs (3)
8-8: LGTM!The import statement correctly brings in the required types from the verifier alliance database module.
138-146: LGTM!The implementation provides a complete and correct mapping between CompiledContractLanguage and SourceType enums, following Rust's type conversion best practices.
378-383: LGTM - Validation implemented as suggested.The validation function has been implemented correctly, following the suggestion from the previous review to prevent ambiguous identifiers in the fully qualified name.
eth-bytecode-db/eth-bytecode-db-server/tests/verifier_alliance_integration/main.rs (1)
1-10: LGTM! Clean module organization and relevant imports.The module structure and imports are well-organized and appropriate for the testing infrastructure.
…ercase before parsing it into CompiledContractLanguage
…btain the latest deployment
eth-bytecode-db/eth-bytecode-db/src/verification/handlers/mod.rs
Outdated
Show resolved
Hide resolved
eth-bytecode-db/eth-bytecode-db/src/verification/handlers/mod.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
eth-bytecode-db/verifier-alliance-database/src/lib.rs (1)
139-142: 🛠️ Refactor suggestionAvoid unnecessary transaction for read-only operations
The function only performs read operations, so starting a transaction is unnecessary unless you need a consistent snapshot of the data.
Apply this diff to remove the transaction:
- let database_connection = database_connection - .begin() - .await - .context("begin transaction")?;And remove the rollback at the end:
- // At the end of the function - database_connection - .rollback() - .await - .context("rollback transaction")?;
🧹 Nitpick comments (10)
eth-bytecode-db/verifier-alliance-database/src/lib.rs (3)
161-163: Extract error messages as constantsLong error messages should be extracted as constants at the module level for better maintainability and reusability.
Add these constants at the module level:
const ERR_COMPILED_CONTRACT_NOT_FOUND: &str = "compiled contract does not exist in the database; verified_contracts.id={}, compiled_contracts.id={}"; const ERR_MISSING_CREATION_CODE: &str = "compiled contract does not have creation code"; const ERR_MISSING_RUNTIME_CODE: &str = "compiled contract does not have runtime code";Then update the error messages:
- .ok_or_else(|| anyhow!("compiled contract does not exist in the database; verified_contracts.id={}, compiled_contracts.id={}", verified_contract_model.id, verified_contract_model.compilation_id))?; + .ok_or_else(|| anyhow!(ERR_COMPILED_CONTRACT_NOT_FOUND, verified_contract_model.id, verified_contract_model.compilation_id))?; - .ok_or(anyhow!("compiled contract does not have creation code"))?; + .ok_or(anyhow!(ERR_MISSING_CREATION_CODE))?; - .ok_or(anyhow!("compiled contract does not have runtime code"))?; + .ok_or(anyhow!(ERR_MISSING_RUNTIME_CODE))?;Also applies to: 170-172, 179-181
188-192: Consider batching source retrievalsInstead of retrieving sources for each contract individually within the loop, consider batching the retrievals for better performance.
Here's a suggested approach:
// Before the loop let compilation_ids: Vec<i64> = verified_contract_models .iter() .map(|model| model.compilation_id) .collect(); let sources_by_compilation = internal::retrieve_sources_by_compilation_ids( &database_connection, compilation_ids, ).await?; // Inside the loop, use sources_by_compilation.get(&compiled_contract_model.id)Would you like me to help implement the batched retrieval function?
150-150: Consider using ORDER BY in the database queryInstead of sorting in memory, consider adding ORDER BY to the database query for better performance with large datasets.
The sorting could be moved to the database query in
retrieve_contract_deployments_by_chain_id_and_address:ORDER BY block_number ASC, updated_at ASCeth-bytecode-db/verifier-alliance-database/src/internal.rs (3)
289-311: Consider adding pagination to retrieval functions.The retrieval functions that return collections (
retrieve_contract_deployments_by_chain_id_and_addressandretrieve_verified_contracts_by_deployment_id) might return large result sets. Consider adding pagination parameters to prevent potential performance issues.Example implementation:
pub async fn retrieve_contract_deployments_by_chain_id_and_address<C: ConnectionTrait>( database_connection: &C, chain_id: u128, contract_address: Vec<u8>, + page: Option<u64>, + page_size: Option<u64>, ) -> Result<Vec<contract_deployments::Model>, Error> { - contract_deployments::Entity::find() + let mut query = contract_deployments::Entity::find() .filter(contract_deployments::Column::ChainId.eq(Decimal::from(chain_id))) - .filter(contract_deployments::Column::Address.eq(contract_address)) + .filter(contract_deployments::Column::Address.eq(contract_address)); + + if let (Some(page), Some(size)) = (page, page_size) { + query = query + .offset((page * size) as u64) + .limit(size as u64); + } + + query .all(database_connection) .await .context("select from \"contract_deployments\"") }
501-521: Consider usingok_or_elseinstead of unwrap for better error handling.While the comment explains that unwrap is safe due to database constraints, it's better to provide explicit error messages for debugging and maintenance.
Consider this safer approach:
- let creation_match = verified_contract - .creation_match - .then(|| { - extract_match_from_model( - verified_contract.creation_metadata_match.unwrap(), - verified_contract.creation_transformations.unwrap(), - verified_contract.creation_values.unwrap(), - ) - }) - .transpose()?; + let creation_match = verified_contract + .creation_match + .then(|| { + let metadata_match = verified_contract.creation_metadata_match + .ok_or_else(|| anyhow!("creation_metadata_match is None despite database constraint"))?; + let transformations = verified_contract.creation_transformations + .ok_or_else(|| anyhow!("creation_transformations is None despite database constraint"))?; + let values = verified_contract.creation_values + .ok_or_else(|| anyhow!("creation_values is None despite database constraint"))?; + extract_match_from_model(metadata_match, transformations, values) + }) + .transpose()?;
Line range hint
289-567: Add documentation for public functions.The newly added public functions lack documentation comments. Consider adding comprehensive documentation that includes:
- Function purpose
- Parameter descriptions
- Return value descriptions
- Example usage where appropriate
Example for one of the functions:
/// Retrieves all contract deployments for a specific chain ID and contract address. /// /// # Arguments /// /// * `database_connection` - The database connection to use /// * `chain_id` - The chain ID to filter by /// * `contract_address` - The contract address to filter by /// /// # Returns /// /// A vector of contract deployment models matching the criteria /// /// # Errors /// /// Returns an error if the database query fails pub async fn retrieve_contract_deployments_by_chain_id_and_address<C: ConnectionTrait>( // ... rest of the functioneth-bytecode-db/eth-bytecode-db/src/verification/handlers/mod.rs (4)
366-366: Returning(db_client, ContractDeployment)is valid, but consider returning a single struct to avoid carrying the database reference as a tuple.
393-395: Warn vs. Error.
If a contract is missing bothtransaction_hashandruntime_code, you might consider making this an error path instead of a warning, since the deployment cannot be meaningfully retrieved.
432-474: Large match expression could be extracted.
Consider refactoring the genesis/regular detection into a helper function for improved readability and testability.
529-531: Fix grammar in error message.
Replace the double negative “have not matched” with “did not match” or similar expression for clarity.- "Neither creation code nor runtime code have not matched" + "Neither creation code nor runtime code matched"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
eth-bytecode-db/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
eth-bytecode-db/Cargo.toml(5 hunks)eth-bytecode-db/eth-bytecode-db-server/Cargo.toml(1 hunks)eth-bytecode-db/eth-bytecode-db/src/verification/handlers/mod.rs(6 hunks)eth-bytecode-db/verifier-alliance-database-tests/Cargo.toml(1 hunks)eth-bytecode-db/verifier-alliance-database/src/internal.rs(4 hunks)eth-bytecode-db/verifier-alliance-database/src/lib.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- eth-bytecode-db/eth-bytecode-db-server/Cargo.toml
- eth-bytecode-db/verifier-alliance-database-tests/Cargo.toml
- eth-bytecode-db/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Linting / lint
- GitHub Check: Unit, doc and integration tests
🔇 Additional comments (29)
eth-bytecode-db/verifier-alliance-database/src/lib.rs (1)
14-15: LGTM! Export changes are well-structured.The new type exports follow the existing pattern and are necessary for the new functionality.
eth-bytecode-db/eth-bytecode-db/src/verification/handlers/mod.rs (28)
21-21: No issues found for new import.
23-23: No issues found for new import.
26-29: No issues found in the newly added imports.
324-324: Asynchronous call looks good.
326-335: Straightforward construction ofVerifiedContract.
375-375: No actionable code here.
377-380: Potential negative chain ID risk.
If chain IDs can be negative or exceed theu128range, this conversion could fail. Confirm that all chain IDs fit withinu128, or handle out-of-bound cases gracefully.
381-391: Conditional logic for retrieval is correct.
401-401: Retrieval from database is clear.
405-408: Error message is well-structured.
428-428: Return type is consistent with the rest of the code.
429-430: Chain ID parsing is consistent with earlier logic.
431-431: Beginning of match expression is clear.
477-479: Insertion call is straightforward.
484-485: Parameters are descriptive.
486-490: Artifact parsing approach is consistent.
491-498: Creation and runtime code artifacts parsing is correct.
499-508: Code matching logic is clear.
509-510: No issues with runtime code matching logic.Also applies to: 512-512, 514-514
516-523: Valid handling for matching both creation and runtime codes.
524-525: Correct handling of creation-only match.
526-528: Correct handling of runtime-only match.
535-542: Artifact deserialization logic is clear.
581-581: Concurrent insertion strategy is appropriate.
633-633: Clear invocation ofcheck_code_matches.
635-635: Straightforward creation ofverified_contract.
636-642: Fields assigned accurately.
643-645: Insertion call into alliance database is consistent.
rimrakhimov
deleted the
rimrakhimov/eth-bytecode-db/verifier-alliance-integrate-schema-v1
branch
Closes #1108
Integrates updated
verifier-alliance-migration,verifier-alliance-entityandverifier-alliance-databasecrates to integrate v1 alliance schema into eth-bytecode-db serviceChanges left to the next PRs:
check_match_statusesfunction (validating that inserted verification data is not worse than existing ones) was removed in this PR, and will be returned later intoverifier-alliance-databasecrate.Summary by CodeRabbit
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
New Features
Improvements
Changes
Removed
Testing