-
Notifications
You must be signed in to change notification settings - Fork 151
Migrate balancer v2 sources to alloy #4023
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
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 represents a significant internal refactoring effort, transitioning the Balancer V2 liquidity source implementation to leverage the 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
|
|
/gemini review |
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 is a significant and well-executed refactoring to migrate Balancer V2 sources from ethcontract types to alloy types. The changes span across multiple crates, including driver, shared, solver, and solvers, consistently replacing types like H256, U256, and BlockId with their alloy equivalents. The removal of BalancerFactoryKind from command-line arguments and its associated logic is a good simplification. The test suite has been diligently updated to reflect these type changes. Overall, the migration appears to be thorough and correct, improving type consistency across the codebase.
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 migrates the Balancer V2 liquidity sources from ethcontract types to alloy types. This is a significant refactoring that touches many files across driver, shared, solver, and solvers crates. The changes primarily involve replacing ethcontract::{H256, U256} with their alloy counterparts (B256, U256), removing the IntoAlloy/IntoLegacy conversion traits, and updating contract calls and tests to use alloy idioms. Additionally, the configuration of Balancer factories has been refactored, removing the BalancerFactoryKind enum in favor of direct address configuration.
Overall, the migration is well-executed and improves code clarity by using alloy's more idiomatic constants and methods. The changes are consistent across the codebase.
I have one comment regarding a change that seems out of scope for this PR.
I am having trouble creating individual review comments. Click here to see my feedback.
crates/shared/src/sources/mod.rs (79-98)
The removal of PoolAggregator and its PoolFetching implementation seems unrelated to the PR's goal of migrating Balancer V2 sources to Alloy. This logic is for Uniswap V2 pools. Was this removal intentional? If so, it might be better to move it to a separate PR to keep this one focused. If it was not intentional, it should be reverted.
|
/gemini review |
| #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq, ValueEnum)] | ||
| #[clap(rename_all = "verbatim")] | ||
| pub enum BalancerFactoryKind { | ||
| Weighted, | ||
| WeightedV3, | ||
| WeightedV4, | ||
| Weighted2Token, | ||
| StableV2, | ||
| LiquidityBootstrapping, | ||
| NoProtocolFeeLiquidityBootstrapping, | ||
| ComposableStable, | ||
| ComposableStableV3, | ||
| ComposableStableV4, | ||
| ComposableStableV5, | ||
| ComposableStableV6, |
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 was unused
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 unused, on the infra repo too
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 successfully migrates the Balancer V2 sources module from ethcontract to alloy primitives, which is a great step forward in modernizing the codebase. The changes simplify the code and improve type safety across various components like pool fetching, pool implementations, and swap math. The refactoring is well-executed and the code is much cleaner. The identified minor issues, such as a typo in a comment and duplicated test assertions, can be addressed for further improvement.
crates/shared/src/sources/balancer_v2/pool_fetching/pool_storage.rs
Outdated
Show resolved
Hide resolved
m-sz
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.
Small typo, otherwise LGTM!
crates/shared/src/sources/balancer_v2/pool_fetching/pool_storage.rs
Outdated
Show resolved
Hide resolved
…ge.rs Co-authored-by: Marcin Szymczak <[email protected]>
Description
Migrates Balancer V2 sources module from
ethcontracttoalloyprimitives, continuing the project-wide modernizationeffort. This simplifies the codebase and improves type safety across pool fetching, pool implementations, and swap math
calculations.
Changes
ethcontractcontract bindings withalloytypes in pool fetchingrecent_block_cache.rssources/mod.rsHow to test
Existing tests