Add Balancer V3 support for Plasma #179
Conversation
AryanGodara
changed the title
Cargo.toml
Outdated
| tracing = "0.1" | ||
| web3 = "0.19" | ||
|
|
||
| contracts = { git = "https://github.com/cowprotocol/services.git", tag = "v2.335.0", package = "contracts" } |
There was a problem hiding this comment.
This one should probably not be merged.
This requires cowprotocol/services#3908
PR to get merged, so that we have a new tag to point to, to import the balancer contracts. For now I'm making it point to a branch on my open PR
- Fix V2/V3 protocol version handling in swap logic - Add chain-specific defaults (Plasma → V3-only) - Add Plasma config example - Update contracts dependency for Plasma addresses Signed-off-by: Aryan Godara <aryangodara03@gmail.com>
…rders For buy orders (EXACT_OUT), the on-chain query result represents the input amount needed, not the output. Previously, the code incorrectly assigned (result, quote.return_amount_raw) for buy orders, which caused the fill validation to fail. Now both sell and buy orders correctly use quote.swap_amount_raw as the 'given' amount and the query result as the 'calculated' amount. Signed-off-by: Aryan Godara <aryangodara03@gmail.com>
Apply consistent formatting across multiple files including import statement grouping and line breaks. Signed-off-by: Aryan Godara <aryangodara03@gmail.com>
AryanGodara
force-pushed
the
aryan/add-plasma
branch
from
df1da99 to
7088e56
Compare
- Update contracts dependency from git rev to v2.336.0 tag to match other CoW dependencies - Apply consistent formatting across multiple files including import statement grouping and line breaks Signed-off-by: Aryan Godara <aryangodara03@gmail.com>
| ] } | ||
| serde = "1" | ||
| serde_json = "1" | ||
| serde_with = "3" |
There was a problem hiding this comment.
Getting a build failure from the app-data crate:
error[E0432]: unresolved import `serde_with::serde`
--> app-data/src/app_data_hash.rs:26:17
|
26 | serde_with::serde::{Deserialize, Serialize},
| ^^^^^ could not find `serde` in `serde_with`
Looks like app_data_hash.rs imports serde_with::serde::{Deserialize, Serialize}, but serde_with v3 doesn't have that re-export anymore (it was removed in the v2 → v3 upgrade). Not sure how this is working on main - maybe I'm missing something in my setup?
There was a problem hiding this comment.
This is also causing both the CI workflows to fail : (
There was a problem hiding this comment.
Why is the PR updating the dependencies in the first place? Unless a PR needs a dependency bump because it makes use of a new lib or features let's separate maintenance dependency bumps from new code.
There was a problem hiding this comment.
It was mainly linting which changed for cargo.toml (Maybe i'm using a different default formatter for toml files).
And i needed to change 1 dependency, to get the imports to work, as I referenced in above comment, I was temporarily making it point to a specific branch to fetch the balancer contracts on plasma from the services repo
This one should probably not be merged.
This requires cowprotocol/services#3908
PR to get merged, so that we have a new tag to point to, to import the balancer contracts. For now I'm making it point to a branch on my open PR
AryanGodara
requested review from
MartinquaXD and
jmg-duarte
and removed request for
fafk
Signed-off-by: Aryan Godara <aryangodara03@gmail.com>
MartinquaXD
left a comment
MartinquaXD
left a comment
There was a problem hiding this comment.
The fix itself makes sense to me. Probably needs to have some tests updated after the dependency issue gets resolved.
Cargo.toml
Outdated
| prometheus = "0.13" | ||
| prometheus-metric-storage = "0.5.0" | ||
| reqwest = "0.11" | ||
| reqwest = { version = "0.11", default-features = false, features = [ |
There was a problem hiding this comment.
What version of cargo fmt are you running?
In CI we use cargo +nightly fmt --all since we make use of formatting rules that are nightly only at the moment. But even if I run cargo fmt --all I can't reproduce the formatting.
That being said breaking crates with many features into multiple lines makes sense to me but ideally we'd make that reproducible using cargofmt rules.
There was a problem hiding this comment.
These are the versions I'm using :-
❯ cargo fmt --version
rustfmt 1.8.0-stable (ed61e7d7e2 2025-11-07)
❯ cargo --version
cargo 1.91.1 (ea2d97820 2025-10-10)
❯ rustc --version
rustc 1.91.1 (ed61e7d7e 2025-11-07)As for the cargo.toml formatting, it's the auto format on save feature. I think my IDE uses prettier extension for toml files (by default), and cargofmt doesn't really target the toml file, so we might need another cli linter installed for that to work 🤔
MartinquaXD
left a comment
MartinquaXD
left a comment
There was a problem hiding this comment.
Last nit: Could you please reset the changes to Cargo.lock and Cargo.toml altogether? The diff on GH doesn't show any relevant changes yet the files show up. Usually it's nice to remove any unnecessary diffs from the PR to make it as straight forward to review as possible.
Signed-off-by: Aryan Godara <aryangodara03@gmail.com>
Signed-off-by: Aryan Godara <aryangodara03@gmail.com>
It was the newline at the end, which was generating the diff. Fixed it. Now the PR only changes the relevant code. Will keep in mind from future 🫡 |
Previously, buy orders incorrectly assigned
(result, quote.return_amount_raw)which swapped the values. The on-chain query result should always bereturn_amount(the calculated value), whilequote.swap_amount_rawshould always beswap_amount(the given/exact value) for both order sides.(I also ran the linter
cargo fmtandcargo clippyso there are some formatting changes as well, but otherwise there were only 2 bug / business logic that I changed, both for balancer)