-
Notifications
You must be signed in to change notification settings - Fork 39
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
Entry point api #25
Entry point api #25
Conversation
"$OUT_DIR/UserOperation.sol/UserOperationLib.json" | ||
); | ||
|
||
impl From<UserOp> for entry_point_api::UserOperation { |
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 is dup to the pr #24 . Just want to put it here to compile well. Should be removed later.
} | ||
|
||
#[derive(Debug, Deserialize, PartialEq)] | ||
pub struct JsonRpcError { |
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 is actually the same as https://github.com/gakonst/ethers-rs/blob/master/ethers-providers/src/transports/common.rs#L14-L22. But sadly, ethers-rs doesn't expose this type.
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.
Do you know if there is any specific reason why that type is not exposed? Maybe we should update ethers-rs to expose 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.
Besides JsonRpcError, there are a lot of middleware error types in ethers-rs. What kind of error you get depends on the middleware and it is dynamic dispatch. The ethers-rs team told me it is going to change in version 2. I didn't explore how difficult to fix it in the current version.
FYI, the current solution is not handling all the situations well. I haven't met any network errors (or middleware error) yet. And we need to improve this in the future.
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.
Ok, got it, thanks for the explanation
type Err = anyhow::Error; | ||
fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
let re = Regex::new( | ||
r###"code: (\d+), message: ([^,]*), data: (None|Some\(String\("([^)]*)"\))"###, |
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.
https://github.com/gakonst/ethers-rs/blob/master/ethers-providers/src/transports/common.rs#L26
Revert back to JsonRpcError based on the Display
.
ops_per_aggregator: Vec<U>, | ||
beneficiary: Address, | ||
) -> Result<(), EntryPointErr> { | ||
todo!() |
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.
I haven't found good test cases for this. I would make it work in another pr.
} | ||
|
||
#[derive(Debug, Deserialize, PartialEq)] | ||
pub struct JsonRpcError { |
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.
Do you know if there is any specific reason why that type is not exposed? Maybe we should update ethers-rs to expose it.
This looks good; good job! Left some comments |
@zsluedem It's ready for merge unless you plan to make any additional changes. |
@Vid201 Thanks |
Introduce a wrapped entry point api that handle the error in our own way.