-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat: Alloy migration #2
Conversation
use std::{fmt, str::FromStr}; | ||
|
||
/// A block number or tag. | ||
#[derive(Copy, Clone, Debug, Default, PartialEq, Eq, Hash)] |
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.
This eventually should come from RPC types.
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.
Flagging this also exists here: https://github.com/paradigmxyz/reth/blob/529635f8d4d627d3652807e3f0377b3c256de4fc/crates/primitives/src/block.rs#L548.
}; | ||
|
||
/// Helper type to parse numeric strings, `u64` and `U256` | ||
#[derive(Deserialize, Debug, Clone)] |
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.
These were moved from ethers_core
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.
How do we do this in Reth? @mattsse
|
||
/// Common Ethereum unit types. | ||
#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)] | ||
pub enum Units { |
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.
Also moved from ethers_core
. This feels like it should live somewhere else (maybe alloy), as a primitive.
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.
@prestwich @DaniPopes I am OK w/ moving this to core/
but no strong opinions
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.
this adds a bunch of rpc junk that should live somewhere else, alloy-rpc-types I think.
so perhaps we do that first?
but also fine with doing some duplication here in the meantime
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.
Good w/ me. The follow-up should remove RawAbi. I don't mind the BlockNumber type being here, let's open ticket for removing it when we have it in the RPC types.
use std::{fmt, str::FromStr}; | ||
|
||
/// A block number or tag. | ||
#[derive(Copy, Clone, Debug, Default, PartialEq, Eq, Hash)] |
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.
Flagging this also exists here: https://github.com/paradigmxyz/reth/blob/529635f8d4d627d3652807e3f0377b3c256de4fc/crates/primitives/src/block.rs#L548.
}; | ||
use alloy_json_abi::JsonAbi as Abi; | ||
use alloy_primitives::{Address, Bytes}; | ||
use ethers_core::abi::RawAbi; |
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.
This should be removed and adjusted to the right Alloy type, am OK w/ doing in follow up.
}; | ||
|
||
/// Helper type to parse numeric strings, `u64` and `U256` | ||
#[derive(Deserialize, Debug, Clone)] |
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.
How do we do this in Reth? @mattsse
|
||
/// Common Ethereum unit types. | ||
#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)] | ||
pub enum Units { |
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.
@prestwich @DaniPopes I am OK w/ moving this to core/
but no strong opinions
assert_eq!(item.abi().unwrap(), serde_json::from_str(DAO_ABI).unwrap()); | ||
assert_eq!( | ||
item.abi().unwrap(), | ||
serde_json::from_str(DEPOSIT_CONTRACT_ABI).unwrap() |
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.
did anything not work with the DAO ABI?
Migrates the repo to alloy. The only remaining ethers types is
Chain
, which is still being worked on as a separate package.