-
Notifications
You must be signed in to change notification settings - Fork 151
Migrate last ethcontract error handling to alloy #4024
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
Conversation
|
/gemini review |
Summary of ChangesHello @jmg-duarte, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request finalizes the transition of contract error handling from the legacy Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request does a great job of migrating the remaining ethcontract error handling to alloy, removing obsolete code and simplifying the logic. The changes in balancer_v2 are correct. I've found one potential issue in the collect_pool_results function you modified. It seems to have a pre-existing bug in how it handles paused pools, which could lead to unexpected errors instead of filtering. I've left a detailed comment with a suggested fix. Otherwise, the changes look solid.
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.
Code Review
This pull request effectively completes the migration from ethcontract to alloy for error handling. The changes are well-contained and correctly replace the old error classification logic with the new alloy-based mechanisms. The removal of the crates/contracts/src/errors.rs file and its related code cleans up the codebase nicely. The updates in the Balancer and Uniswap v2 pool fetching logic, as well as the Tenderly API, are correct and align with the goal of using alloy's error types. I have one suggestion to improve readability in the Balancer pool fetching logic by re-introducing a helper function for error checking, which would make the code cleaner and more consistent with its previous structure.
squadgazzz
left a comment
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
# Description Removes the ethcontract-generate dependency from contracts, along with #4024 we'll be able to fully remove the ethcontract dependency from contracts. Note that the artifacts JSON changes are outputs from the *new* vendoring — i.e. the only change was the newline. # Changes - [ ] Replaces the ethcontract-generate dependency with reqwest and a bit of serde_json work ## How to test * Checkout the previous contracts * Run `RUST_BACKTRACE=1 cargo r -p contracts -F bin --bin vendor` * Validate the diff
Description
While working on the Balancer migration, I noticed that we were filtering out errors with the old logic, even though we've migrated all calls to alloy (this either means we've been missing errors or that the library doesn't error at all, so we didn't notice 😛) — it's the downside of using anyhow/erasing our error types, inspection breaks
After making the proper changes, I was able to remove the old contract error handling for good.
Changes
How to test
Existing tests