Draft: Create a wrap for eth::U256 in order to ensure correct serde #2086 #2416

m-lord-renkse · 2024-02-20T10:31:56Z

Description

Remove the eth::U256 type and create a new type U256 in crates/number. The idea is to enforce specific (de)serialization for for the U256 type as per #2086.

Changes

The new type as been converted into primitive_types::U256 when necessary when needed for the business logic. Ideal solution would be to propagate the new type not only as an API type, but also as an internal type for the business logic operations (but it requires many more changes, it could be potentially done in a followup issue).

How to test

#2086

github-actions · 2024-02-20T10:32:11Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

crates/driver/src/infra/solver/dto/solution.rs

crates/driver/src/domain/eth/gas.rs

m-lord-renkse · 2024-02-20T10:59:40Z

I have read the CLA Document and I hereby sign the CLA

MartinquaXD

Maybe I misunderstood the original issue but I thought we only want to have a new type that handles the (de)serialization without having to remember to use macros.

If we want to completely replace primitve_types::U256 with our new type I think we should look into implementing std::ops::Deref for the new type. That way we probably wouldn't have to reimplement a bunch of functions for the new type that we are currently using on the existing type.

Alternatively if we want to actually completely replace primitive_types::U256 with the new type it might also make sense to add a new crate that re-exports all the primtive_types but replaces the regular U256 with the new U256 type. But if we already go that far could also consider forking the primitive_types crate or even patching the behavior upstream.

crates/driver/src/boundary/mempool.rs

crates/driver/src/domain/competition/auction.rs

crates/driver/src/infra/config/file/mod.rs

crates/driver/src/infra/solver/dto/solution.rs

crates/driver/src/util/serialize/mod.rs

crates/number/src/serialization.rs

m-lord-renkse · 2024-02-21T11:39:13Z

crates/driver/src/boundary/mempool.rs

@@ -51,7 +52,7 @@ pub enum Kind {
    /// The MEVBlocker private mempool.
    MEVBlocker {
        url: reqwest::Url,
-        max_additional_tip: eth::U256,
+        max_additional_tip: primitive_types::U256,


I did this to keep the same behavior.

What same behaviour? Isn't eth::U256 just a re-export of primitive type?

squadgazzz · 2024-02-21T12:08:59Z

Would it be possible to split this activity into separate PRs moving from crate to crate(or any other separation kind)? Maybe, to also add tracking tasks to the original issue.

MartinquaXD

The driver and solvers crate export a U256 type as part of the eth module (e.g. here). Might be a good idea to export the new number::U256 instead of the current primitive_types::U256 instead to keep people from using the old type.

Overall I'm still not sure what the best solution is for this. :/
Maybe @fleupold, @sunce86 and @squadgazzz have an opinion as well?

crates/number/src/serialization.rs

crates/autopilot/src/driver_model.rs

squadgazzz · 2024-02-22T15:11:28Z

Might be a good idea to export the new number::U256 instead of the current primitive_types::U256 instead to keep people from using the old type.

That looks convenient.

m-lord-renkse · 2024-02-22T15:21:52Z

The driver and solvers crate export a U256 type as part of the eth module (e.g. here). Might be a good idea to export the new number::U256 instead of the current primitive_types::U256 instead to keep people from using the old type.

Overall I'm still not sure what the best solution is for this. :/ Maybe @fleupold, @sunce86 and @squadgazzz have an opinion as well?

It is a good idea, but I think the best is to do it in a followup issue, not to have a lot of more changes in this one, wdyt?

MartinquaXD · 2024-02-22T15:36:22Z

It is a good idea, but I think the best is to do it in a followup issue, not to have a lot of more changes in this one, wdyt?

Makes sense. I keep thinking that the new type is equivalent to the old one so it shouldn't introduce more changes. But actually we'd have to dereference in a lot of places then.

MartinquaXD

Looks alright to me. Let's give it a try.

squadgazzz

LG. Do we have a follow-up issue? Could you link it here, please?

fleupold

I don't want to be negative about this PR (I realize a lot of work went into making all these changes), but what are we really gaining here? Maybe we were a bit too optimistic about the original issue in the first place?

We have a new type with a bunch of boilerplate conversion but now have to remember instead of using #[serde_as(as = "HexOrDecimalU256")] to use number::256 (if most of other callsites continue using eth::U256 this may be as challenging). I assume we cannot get a clippy warning for this?

I guess the real win will only come when we replace the eth::U256 export with the new type (making confusion less likely)?

fleupold · 2024-02-23T18:28:38Z

crates/driver/src/boundary/mempool.rs

@@ -51,7 +52,7 @@ pub enum Kind {
    /// The MEVBlocker private mempool.
    MEVBlocker {
        url: reqwest::Url,
-        max_additional_tip: eth::U256,
+        max_additional_tip: primitive_types::U256,


What same behaviour? Isn't eth::U256 just a re-export of primitive type?

MartinquaXD · 2024-02-23T20:57:36Z

I assume we cannot get a clippy warning for this?

Unfortunately not. There is an open issue for warning on trait implementation on specific types but and the open draft PR doesn't seem like it would get merged any time soon.
The options I see are:

fork primitive types and patch it
try to upstream that patch. They are aware of the issue but refuse to change the default behavior. I asked if they would be fine with a feature flag that switches to the saner logic and they are open to this option. So depending on how tricky that patch is (probably requires a patch to a macro) this might be the best way out.
completely replace U256 type but I think it will not be possible to make it work seamlessly with the regular primitive_types during computations.
push the clippy PR over the finish line

m-lord-renkse · 2024-02-26T08:06:50Z

Unfortunately not. There is an open issue for warning on trait implementation on specific types but and the open draft PR doesn't seem like it would get merged any time soon. The options I see are:

fork primitive types and patch it

try to upstream that patch. They are aware of the issue but refuse to change the default behavior. I asked if they would be fine with a feature flag that switches to the saner logic and they are open to this option. So depending on how tricky that patch is (probably requires a patch to a macro) this might be the best way out.

completely replace U256 type but I think it will not be possible to make it work seamlessly with the regular primitive_types during computations.

push the clippy PR over the finish line

I think 1 may be complicated to maintain. We would need to rebase the fork every time we want to bump to the latest version of primitive_types.

The option 3 is not complicated, it should be a smaller PR than this one I believe. I could take care of that as a follow up issue (I'll create the issue once confirmed).

sunce86 · 2024-02-26T12:56:08Z

The driver and solvers crate export a U256 type as part of the eth module (e.g. here). Might be a good idea to export the new number::U256 instead of the current primitive_types::U256 instead to keep people from using the old type.

Overall I'm still not sure what the best solution is for this. :/ Maybe @fleupold, @sunce86 and @squadgazzz have an opinion as well?

Long term, I would prefer to forbid using primitive_types::U256 in the domain, and instead have a wrapper around it for every object, because every U256 represents something meaningful to the domain. Not sure if it should be number::U256, because we don't need serde in domain.

sunce86

Mostly nits.

sunce86 · 2024-02-26T12:49:09Z

crates/autopilot/src/infra/solvers/dto/solve.rs

@@ -6,7 +6,6 @@ use {
    },
    chrono::{DateTime, Utc},
    itertools::Itertools,
-    number::serialization::HexOrDecimalU256,


nit: can we also make the line below this one primitive_types::H160, and explicitly use primitive_types::U256 where needed, so if we accidentally add a reference to this type next time, it's more obvious.

sunce86 · 2024-02-26T13:03:18Z

crates/e2e/tests/e2e/ethflow.rs

@@ -135,7 +135,7 @@ async fn eth_flow_tx_zero_fee(web3: Web3) {
            .get_order(&ethflow_order.uid(onchain.contracts()).await)
            .await
            .unwrap();
-        order.metadata.executed_surplus_fee > U256::zero()
+        order.metadata.executed_surplus_fee > U256::zero().into()


Suggested change

order.metadata.executed_surplus_fee > U256::zero().into()

order.metadata.executed_surplus_fee > number::U256::zero()

sunce86 · 2024-02-26T13:04:26Z

crates/e2e/tests/e2e/limit_orders.rs

@@ -376,10 +376,10 @@ async fn mixed_limit_and_market_orders_test(web3: Web3) {

    let order_b = OrderCreation {
        sell_token: token_b.address(),
-        sell_amount: to_wei(5),
-        fee_amount: to_wei(1),
+        sell_amount: to_wei(5).into(),


nit: what happens if we change to_wei() to return number::U256?

sunce86 · 2024-02-26T13:12:49Z

crates/number/src/nonzero.rs

+    fn try_from(value: crate::U256) -> Result<Self, Self::Error> {
+        ZeroU256::from(value).try_into()
+    }
+}


This should be removed as not used.

sunce86 · 2024-02-26T13:19:31Z

crates/number/src/serialization.rs

+impl U256 {
+    pub fn zero() -> Self {
+        Self(primitive_types::U256::zero())
+    }


nit: Instead, how about implementing an existing trait for this:

impl num::Zero for U256 { fn zero() -> Self { Self(primitive_types::U256::zero()) } fn is_zero(&self) -> bool { self.0.is_zero() } } impl std::ops::Add for U256 { type Output = Self; fn add(self, rhs: Self) -> Self::Output { Self(self.0 + rhs.0) } }

github-actions · 2024-03-05T00:06:17Z

This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed.

m-lord-renkse requested a review from a team as a code owner February 20, 2024 10:31

m-lord-renkse force-pushed the implement-wrap-ethU256 branch from d9e0556 to 2ce009d Compare February 20, 2024 10:38

m-lord-renkse commented Feb 20, 2024

View reviewed changes

crates/driver/src/infra/solver/dto/solution.rs Outdated Show resolved Hide resolved

m-lord-renkse force-pushed the implement-wrap-ethU256 branch 2 times, most recently from 7103de8 to d9f8c2c Compare February 20, 2024 10:48

m-lord-renkse commented Feb 20, 2024

View reviewed changes

crates/driver/src/domain/eth/gas.rs Outdated Show resolved Hide resolved

MartinquaXD reviewed Feb 20, 2024

View reviewed changes

m-lord-renkse force-pushed the implement-wrap-ethU256 branch from d9f8c2c to 2ac995e Compare February 21, 2024 11:35

github-actions bot added a commit that referenced this pull request Feb 21, 2024

@Mateo-mro has signed the CLA from Pull Request #2416

d0a622f

m-lord-renkse commented Feb 21, 2024

View reviewed changes

m-lord-renkse force-pushed the implement-wrap-ethU256 branch 2 times, most recently from 27d1049 to 4b639d5 Compare February 21, 2024 12:05

m-lord-renkse force-pushed the implement-wrap-ethU256 branch 2 times, most recently from cd66af2 to 28c8265 Compare February 21, 2024 12:58

MartinquaXD reviewed Feb 22, 2024

View reviewed changes

crates/number/src/serialization.rs Outdated Show resolved Hide resolved

crates/number/src/serialization.rs Outdated Show resolved Hide resolved

crates/autopilot/src/driver_model.rs Outdated Show resolved Hide resolved

m-lord-renkse force-pushed the implement-wrap-ethU256 branch 2 times, most recently from 501c058 to ceaefb0 Compare February 22, 2024 15:10

m-lord-renkse force-pushed the implement-wrap-ethU256 branch from ceaefb0 to d41de45 Compare February 22, 2024 15:19

m-lord-renkse force-pushed the implement-wrap-ethU256 branch from d41de45 to f8c0f85 Compare February 22, 2024 15:23

MartinquaXD approved these changes Feb 23, 2024

View reviewed changes

m-lord-renkse force-pushed the implement-wrap-ethU256 branch from f8c0f85 to 1015cc6 Compare February 23, 2024 13:30

squadgazzz approved these changes Feb 23, 2024

View reviewed changes

fleupold reviewed Feb 23, 2024

View reviewed changes

Create a wrap for eth::U256 in order to ensure correct serde

c2f4b0e

m-lord-renkse force-pushed the implement-wrap-ethU256 branch from 1015cc6 to c2f4b0e Compare February 26, 2024 08:10

sunce86 approved these changes Feb 26, 2024

View reviewed changes

github-actions bot added the stale label Mar 5, 2024

github-actions bot closed this Mar 13, 2024

m-lord-renkse deleted the implement-wrap-ethU256 branch September 19, 2024 12:48

github-actions bot mentioned this pull request Dec 3, 2024

Pull requests report (3/12/2024 09:08) cowprotocol/pm#87

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: Create a wrap for eth::U256 in order to ensure correct serde #2086 #2416

Draft: Create a wrap for eth::U256 in order to ensure correct serde #2086 #2416

m-lord-renkse commented Feb 20, 2024

github-actions bot commented Feb 20, 2024 •

edited

Loading

m-lord-renkse commented Feb 20, 2024

MartinquaXD left a comment

m-lord-renkse Feb 21, 2024

fleupold Feb 23, 2024

squadgazzz commented Feb 21, 2024 •

edited

Loading

MartinquaXD left a comment

squadgazzz commented Feb 22, 2024

m-lord-renkse commented Feb 22, 2024

MartinquaXD commented Feb 22, 2024

MartinquaXD left a comment

squadgazzz left a comment

fleupold left a comment

fleupold Feb 23, 2024

MartinquaXD commented Feb 23, 2024 •

edited

Loading

m-lord-renkse commented Feb 26, 2024

sunce86 commented Feb 26, 2024

sunce86 left a comment

sunce86 Feb 26, 2024

sunce86 Feb 26, 2024

sunce86 Feb 26, 2024

sunce86 Feb 26, 2024

sunce86 Feb 26, 2024

github-actions bot commented Mar 5, 2024

	order.metadata.executed_surplus_fee > U256::zero().into()
	order.metadata.executed_surplus_fee > number::U256::zero()

Draft: Create a wrap for eth::U256 in order to ensure correct serde #2086 #2416

Draft: Create a wrap for eth::U256 in order to ensure correct serde #2086 #2416

Conversation

m-lord-renkse commented Feb 20, 2024

Description

Changes

How to test

github-actions bot commented Feb 20, 2024 • edited Loading

m-lord-renkse commented Feb 20, 2024

MartinquaXD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squadgazzz commented Feb 21, 2024 • edited Loading

MartinquaXD left a comment

Choose a reason for hiding this comment

squadgazzz commented Feb 22, 2024

m-lord-renkse commented Feb 22, 2024

MartinquaXD commented Feb 22, 2024

MartinquaXD left a comment

Choose a reason for hiding this comment

squadgazzz left a comment

Choose a reason for hiding this comment

fleupold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinquaXD commented Feb 23, 2024 • edited Loading

m-lord-renkse commented Feb 26, 2024

sunce86 commented Feb 26, 2024

sunce86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 5, 2024

github-actions bot commented Feb 20, 2024 •

edited

Loading

squadgazzz commented Feb 21, 2024 •

edited

Loading

MartinquaXD commented Feb 23, 2024 •

edited

Loading