Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 73.68% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title 'Migrate from Ethers to Alloy' directly and concisely describes the main change: replacing the deprecated ethers-rs library with alloy throughout the codebase. |
| Description check | ✅ Passed | The PR description follows the template structure with both usage and development sections completed, includes the issue reference (#886), and has all checklist items marked as done. |
| Linked Issues check | ✅ Passed | The PR successfully addresses issue #886 by migrating all ethers-rs dependencies to alloy, including updates to workspace dependencies, error types, contract bindings, provider implementations, and test utilities. |
| Out of Scope Changes check | ✅ Passed | All code changes are directly related to the ethers-to-alloy migration: dependency updates, type replacements (H256→B256, Provider implementations), error handling, and test adjustments align with the stated objective. |
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/common/background_anvil.rs (1)
72-75: Avoid panicking on invalid private keysLine [73] calls
PrivateKeySigner::from_str(private_key).unwrap(), so a malformed key (e.g., user typo) will panic the test harness instead of returning a structuredTestError::AlloyError. Please propagate the error viamap_errso the caller gets the expected RPC failure surface.Apply this diff:
- let provider_signer = - PrivateKeySigner::from_str(private_key).unwrap().with_chain_id(chain_id.into()); + let provider_signer = PrivateKeySigner::from_str(private_key) + .map_err(|e| TestError::AlloyError(format!("Invalid private key: {e}")))? + .with_chain_id(chain_id.into());
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.toml(2 hunks)crates/starknet-devnet-core/Cargo.toml(1 hunks)crates/starknet-devnet-core/src/error.rs(1 hunks)crates/starknet-devnet-core/src/messaging/ethereum.rs(14 hunks)crates/starknet-devnet-core/src/messaging/mod.rs(5 hunks)crates/starknet-devnet-core/src/starknet/add_l1_handler_transaction.rs(2 hunks)crates/starknet-devnet-core/src/starknet/mod.rs(2 hunks)tests/integration/Cargo.toml(1 hunks)tests/integration/common/background_anvil.rs(6 hunks)tests/integration/common/errors.rs(1 hunks)tests/integration/common/utils.rs(2 hunks)tests/integration/test_messaging.rs(16 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-16T07:35:24.725Z
Learnt from: FabijanC
Repo: 0xSpaceShard/starknet-devnet PR: 799
File: crates/starknet-devnet-core/src/starknet/mod.rs:240-246
Timestamp: 2025-06-16T07:35:24.725Z
Learning: In the Starknet devnet codebase, `get_class_hash_at` has different behaviors at different layers: at the internal state level (blockifier), it returns `Ok(ClassHash(Felt::ZERO))` for undeployed addresses, while at the RPC layer, this gets converted to `Err(Error::ContractNotFound)`. When checking if a contract is deployed at the internal state level, use `is_ok_and(|h| h.0 == Felt::ZERO)` to detect undeployed addresses.
Applied to files:
crates/starknet-devnet-core/src/starknet/mod.rscrates/starknet-devnet-core/src/messaging/mod.rstests/integration/test_messaging.rs
📚 Learning: 2025-09-18T13:11:58.957Z
Learnt from: Abeeujah
Repo: 0xSpaceShard/starknet-devnet PR: 848
File: tests/integration/test_blocks_generation.rs:65-78
Timestamp: 2025-09-18T13:11:58.957Z
Learning: The function assert_tx_succeeded_pre_confirmed in tests/integration/common/utils.rs was not converted to return Result<(), anyhow::Error> and still returns unit type (), unlike assert_tx_succeeded_accepted which was converted to return Result.
Applied to files:
tests/integration/common/utils.rstests/integration/common/errors.rstests/integration/test_messaging.rs
📚 Learning: 2025-09-18T13:15:00.238Z
Learnt from: Abeeujah
Repo: 0xSpaceShard/starknet-devnet PR: 848
File: tests/integration/test_blocks_generation.rs:141-162
Timestamp: 2025-09-18T13:15:00.238Z
Learning: In tests/integration/common/utils.rs, the function assert_tx_succeeded_pre_confirmed currently contains only a TODO comment and doesn't perform any actual assertions, making it effectively unimplemented despite having its signature updated to return Result<(), anyhow::Error>.
Applied to files:
tests/integration/common/errors.rstests/integration/test_messaging.rs
📚 Learning: 2025-09-18T13:15:00.238Z
Learnt from: Abeeujah
Repo: 0xSpaceShard/starknet-devnet PR: 848
File: tests/integration/test_blocks_generation.rs:141-162
Timestamp: 2025-09-18T13:15:00.238Z
Learning: The function assert_tx_succeeded_pre_confirmed in tests/integration/common/utils.rs is currently unimplemented - it only contains a TODO message printed via eprintln! and has all the actual assertion logic commented out. The function still returns unit type () rather than Result<(), anyhow::Error> and essentially acts as a no-op stub.
Applied to files:
tests/integration/test_messaging.rscrates/starknet-devnet-core/src/messaging/ethereum.rs
🔇 Additional comments (18)
crates/starknet-devnet-core/src/starknet/add_l1_handler_transaction.rs (2)
1-1: LGTM! Import updated correctly for alloy migration.The import of
B256fromalloy::primitivescorrectly replaces the previousH256import as part of the ethers-rs to alloy migration.
31-38: LGTM! B256 conversion is correct and consistent across the codebase.The conversion from L1 transaction hash to
B256usingB256::new(*l1_tx_hash.as_bytes())is the correct pattern for the alloy migration. Thel1_to_l2_tx_hashesHashMap is properly defined withB256as the key type, and the same conversion pattern is used consistently elsewhere in the codebase.crates/starknet-devnet-core/Cargo.toml (1)
18-18: LGTM! Clean dependency migration.The replacement of
etherswithalloyis straightforward and aligns with the workspace-level dependency updates.crates/starknet-devnet-core/src/error.rs (1)
205-206: LGTM! Error variant correctly updated.The rename from
EthersErrortoAlloyErroris consistent with the library migration and properly updates both the variant name and error message.tests/integration/common/utils.rs (2)
8-8: LGTM! Import updated to Alloy.The import path correctly reflects the migration from ethers to alloy primitives.
465-467: LGTM! Conversion method correctly updated.The change from
U256::from_big_endian(&f.to_bytes_be())toU256::from_be_bytes(f.to_bytes_be())correctly reflects the API difference between ethers and alloy. Both methods interpret big-endian byte arrays identically, so the behavior is preserved.tests/integration/Cargo.toml (1)
19-19: LGTM! Test dependency correctly updated.The replacement of
etherswithalloyin the integration tests aligns with the broader migration.tests/integration/common/errors.rs (1)
21-22: LGTM! Test error variant correctly updated.The rename from
EthersErrortoAlloyErrorin the test error enum is consistent with the core library changes.Cargo.toml (2)
74-74: LGTM! Minor version bump.The upgrade of
urlfrom 2.4 to 2.5 is a minor version increment and should be backward compatible.
118-119: LGTM! New dependencies for migration.The addition of
alloy(1.1.0) andeyre(0.6) at the workspace level properly supports the migration from ethers-rs.crates/starknet-devnet-core/src/starknet/mod.rs (2)
4-4: LGTM! Hash type import updated.The import correctly switches from ethers'
H256to alloy'sB256for representing 32-byte hashes.
1564-1564: LGTM! Hash construction updated for alloy.The change from
H256(*l1_tx_hash.as_bytes())toB256::new(*l1_tx_hash.as_bytes())correctly reflects the API difference between ethers and alloy for constructing 32-byte hashes. The behavior is semantically equivalent.crates/starknet-devnet-core/src/messaging/mod.rs (6)
35-35: LGTM! Hash type import updated.The import correctly switches from ethers'
H256to alloy'sB256.
50-50: LGTM! Documentation updated.The doc comment correctly reflects the change from ethers to alloy.
59-59: LGTM! Public field type updated.The HashMap key type correctly changes from
H256toB256for thel2_to_l1_messages_hashesfield.
63-63: LGTM! Public field type updated.The HashMap key type correctly changes from
H256toB256for thel1_to_l2_tx_hashesfield.
165-165: LGTM! Hash construction updated.The change from
H256(...)toB256::new(...)correctly reflects alloy's API for constructing 32-byte hashes.
211-211: LGTM! Hash construction updated.The change from
H256(...)toB256::new(...)correctly reflects alloy's API for constructing 32-byte hashes.
Usage related changes
Maybe bit faster performance
Development related changes
ethers-rs library has been depreciated in favour of alloy. This PR migrates from one to the other. Resolves #886.
Checklist:
./scripts/format.sh./scripts/clippy_check.sh./scripts/check_unused_deps.sh./scripts/check_spelling.sh./website/README.mdSummary by CodeRabbit