Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces conversion functionality from ethcontract::Account to corresponding alloy types to enable interoperability between the two libraries. The implementation addresses differences in account handling between local accounts (which only need an address) and KMS/PrivateKey accounts (which require actual signers).
- Adds
AlloyAccountenum wrapper to handle different account types uniformly - Implements async conversion trait
TryIntoAlloyAsyncforethcontract::Account - Updates dependencies to include necessary alloy signer features and switches ethcontract to development branch
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/ethrpc/src/alloy/conversions.rs | Adds AlloyAccount enum and conversion implementation from ethcontract Account types |
| crates/ethrpc/Cargo.toml | Enables required alloy signer features for local and AWS KMS signing |
| Cargo.toml | Updates ethcontract dependencies to development branch with AWS library updates |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
squadgazzz
force-pushed
the
alloy-account-convert
branch
2 times, most recently
from
18c92f5 to
c4443b2
Compare
squadgazzz
force-pushed
the
alloy-account-convert
branch
from
92dc286 to
2111790
Compare
# Conflicts: # crates/ethrpc/src/alloy/conversions.rs
# Conflicts: # crates/ethrpc/Cargo.toml # crates/ethrpc/src/alloy/conversions.rs
# Description When experimenting with #3599 I was able to avoid some dependency conflicts after running `cargo generate-lockfile`. This PR updates the lockfile to keep it up-to-date and it should be easier later to work with dependency updates.
# Conflicts: # Cargo.lock
| derivative = "2.2.0" | ||
| derive_more = { version = "1.0.0", features = ["full"] } | ||
| ethcontract = { version = "0.25.8", default-features = false, features = ["aws-kms"] } | ||
| ethcontract = { git = "https://github.com/cowprotocol/ethcontract-rs", rev = "c9ec7c090ea649f1cd2a999caae71aa571251b48", default-features = false, features = ["aws-kms"] } |
There was a problem hiding this comment.
Are the commits here still needed? Do you expect many more changes needed in ethcontract-rs to aid the alloy migration?
There was a problem hiding this comment.
0.25.9 wasnt released yet
There was a problem hiding this comment.
I can't release anything anymore due to an issue with the expired deployment key which I suppose belongs to the gnosis team.
There was a problem hiding this comment.
If doing new releases turns out to be a problem let's stick with pinning commits. After all we want to migrate away from that library anyway. 👌
Description
Adds a conversion from
ethcontract::Accountto the corresponding alloy types. A wrapper is introduced because local accounts don’t share a common type with KMS or PrivateKey in alloy. The usage also differs when building/signing transactions: for a local account it’s enough to just set the from field (an address), while with KMS and PrivateKey there isn’t a direct way to use a signer that’s different from the provider’s configured one(more details alloy-rs/alloy#2829).ethcontract::Account::Lockedremains unused in this repo and it seems like there is no direct way to convert it intoalloy::signers::local::LocalSigner, so this branch isunimplemented.This PR also depends on cowprotocol/ethcontract-rs#981, which explains incompatibilities.
After ethcontract was updated in cowprotocol/ethcontract-rs#981, driver tests started failing with stack overflow. As @jmg-duarte's research showed, that the issue lies in how updated third-party libraries optimizations work, which is not related to any infinite recursion. The problem doesn't exist when building with the
--releaseflag, so instead, the stack size was increased.How to test
In the upcoming PRs.