Skip to content
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

Add try_spawn function for Anvil and Geth bindings #226

Merged
merged 6 commits into from
Mar 7, 2024

Conversation

moricho
Copy link
Contributor

@moricho moricho commented Feb 21, 2024

Motivation

Closes: #136

Solution

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a great start, we can now simplify the existing spawn functions by calling try_spawn.expect

Comment on lines 72 to 74
/// Spawning the anvil process failed.
#[error("couldnt start anvil")]
Spawn,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should also include the wrapped io error

Copy link
Contributor Author

@moricho moricho Feb 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed at: bcdf7f8

@@ -313,6 +346,107 @@ impl Anvil {
chain_id: self.chain_id.or(chain_id),
}
}

/// Consumes the builder and spawns `anvil`. If spawning fails, returns an error.
#[track_caller]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[track_caller]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed at: 263d850

@moricho
Copy link
Contributor Author

moricho commented Feb 25, 2024

@mattsse
Thanks for the review!
I've tried to categorize some of the errors related to Geth. However, I'm unsure if I have left anything out or if the categorization is accurate. If you have any suggestions or advice, I would appreciate your input.
https://github.com/alloy-rs/alloy/pull/226/files#diff-edd3384dd7db8fd08c3dfd953104f384359ab2f8211cdae1504eebd93ffbf38fR174-R210

@moricho moricho requested a review from mattsse March 6, 2024 18:46
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the delay,
error variants look fine,
last thing left to do is to change the fn spawn to

fn spawn(&self) -> Instance {
   self.try_spawn().unwrap()
}

@moricho
Copy link
Contributor Author

moricho commented Mar 7, 2024

Added the above change in 037a15f

@moricho moricho requested a review from mattsse March 7, 2024 12:24
@mattsse mattsse merged commit 9430040 into alloy-rs:main Mar 7, 2024
14 checks passed
@moricho moricho deleted the try-spawn branch March 7, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Add fallible spawn function for Anvil and Geth bindings
2 participants