-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use alloy instead of web3 whenever possible #308
base: xiangyi/helios_indexer_only
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the code to use alloy’s primitives instead of web3 for keccak256 hashing, except for web3::signing::recover. Key changes include:
- Replacing web3::signing::keccak256 calls with alloy::primitives::keccak256 across multiple modules.
- Updating Cargo.toml dependencies to favor alloy and related packages.
- Refactoring contract instantiation in the Ethereum client code to use alloy-based types.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
integration-tests/src/commands.rs | Substituted web3 keccak256 call with alloy version, though without dereference operator. |
integration-tests/src/actions/sign.rs | Updated keccak256 call with alloy and added dereference operator. |
integration-tests/src/actions/mod.rs | Replaced keccak256 usages consistently with alloy calls using dereference. |
integration-tests/Cargo.toml | Added alloy dependency. |
chain-signatures/node/src/rpc.rs | Refactored Ethereum client setup to use alloy contract instance. |
chain-signatures/node/Cargo.toml | Removed web3 dependency and added alloy-signer-local. |
chain-signatures/crypto/src/kdf.rs | Updated hashing and address conversion using alloy. |
chain-signatures/crypto/Cargo.toml | Replaced web3 with alloy workspace dependency. |
Cargo.toml | Added alloy dependency to workspace with necessary features. |
Comments suppressed due to low confidence (1)
integration-tests/src/commands.rs:41
- For consistency with other changes where alloy::primitives::keccak256 is dereferenced, consider using '*alloy::primitives::keccak256(PAYLOAD)' to ensure the correct type is assigned.
let payload_hashed = alloy::primitives::keccak256(PAYLOAD);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I think alloy will be a better dependency for us long-term.
LGTM
chain-signatures/node/src/rpc.rs
Outdated
use alloy::{ | ||
contract::{ContractInstance, Interface}, | ||
dyn_abi::DynSolValue, | ||
network::EthereumWallet, | ||
primitives::U256, | ||
providers::ProviderBuilder, | ||
transports::http::{Client as ReqwestClient, Http}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I don't see many other cases of more-than-one levels of import group nesting. Not sure if anybody cares, I think we are already quite inconsistent with imports, which is fine by me. But you might want to check if you are using rust analyzer settings in your editor that generates this. I think if you set imports.granularity.group
to module
, instead of crate, it will be more in line with existing code.
https://rust-analyzer.github.io/book/features.html?highlight=import#import-granularity
chain-signatures/node/src/rpc.rs
Outdated
let abi_value = json.get("abi").expect("Failed to get ABI from artifact"); | ||
let abi = serde_json::from_str(&abi_value.to_string()).unwrap(); | ||
|
||
// Create a new `ContractInstance` of the `Counter` contract from the abi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Counter contract? Is this maybe a copy-paste leftover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah. I'll remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@@ -14,4 +14,4 @@ sha3.workspace = true | |||
getrandom = { version = "0.2.12", features = ["custom"] } | |||
|
|||
[dev-dependencies] | |||
web3.workspace = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be hard to get rid of web3 completely? It's a heavy crate, and our Rust analyzer is already slow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just that I can't find the equivalent of web3::signing::recover
and don't want to spend time on writing code with alloy that does the same thing...
5e7baf2
to
2b391c7
Compare
This PR replaces the use of web3 crate with alloy in rpc.rs. Now we will be making respond() call on eth with alloy.
Also replaced web3 with alloy in most places except
web3::signing::recover
as I did not find an equal function in alloy yet.