Unify networks and add Base support #3091

m-lord-renkse · 2024-10-29T15:30:54Z

Description

Create a crate called network which unifies all the networks in the whole codebase. The network crate is in fact an enum, which should be always consumed in a non-exhaustive way, therefore, when adding a new network, it will error in all the place it requires action.

This reduced duplicated code, and makes it easier to add a new network. With this change, only network should contain reference to the real chain ID.

Added Base support.

Changes

Create a new crate network
Delete all duplicated code
Make the new type Network as an enun, and replace it in the code base in a non-exhaustive way, so it screams error when adding a new network
Add support for Base

How to test

Unit tests
Regression tests

m-lord-renkse · 2024-10-29T15:35:52Z

crates/autopilot/src/domain/settlement/mod.rs

-const MAINNET_BLOCK_TIME: u64 = 13_000; // ms
-const GNOSIS_BLOCK_TIME: u64 = 5_000; // ms
-const SEPOLIA_BLOCK_TIME: u64 = 13_000; // ms
-const ARBITRUM_ONE_BLOCK_TIME: u64 = 100; // ms


This was incorrect, it should've been 250. Any special reason for it @sunce86 ?

No, I just thought it's 100ms 😄

m-lord-renkse · 2024-10-29T15:36:32Z

crates/autopilot/src/infra/blockchain/id.rs

@@ -1,43 +0,0 @@
-use primitive_types::U256;


Goodbye duplication

m-lord-renkse · 2024-10-29T15:38:39Z

crates/network/Cargo.toml

@@ -0,0 +1,15 @@
+[package]


I am not married to the idea of making it a crate since the code is not that big.

Advantages:

more modular

less code in share

faster compilation time

Disadvantages:

more crates, difficult to navigate

I like the idea of encapsulating all the necessary changes to add a new network in a single place. I think there are probably more types basic enough that we could have shared code for them (although this could also lead to another shared crate with too much stuff in there if we are not careful).

Maybe it makes sense to leave this as a separate network crate for now but be open to the idea to turn it into a primitive_types crate later on to also move Address, ContractAddress, TokenAddress, etc. into it.

I love the idea to have a crate with common domain stuff like Address, TokenAddress, Network, etc. I proposed it in the past too. In the near future, we can rename the crate and stuff moving stuff there and removing duplicates.

m-lord-renkse · 2024-10-29T15:40:27Z

crates/network/src/lib.rs

+/// Represents each available network
+#[derive(Clone, Copy, Debug, PartialEq)]
+pub enum Network {
+    Mainnet = 1,


Now the chain IDs are unified in one place

m-lord-renkse · 2024-10-29T15:41:10Z

crates/network/src/lib.rs

+                Network::try_from(value).map_err(de::Error::custom)
+            }
+
+            fn visit_str<E>(self, value: &str) -> Result<Self::Value, E>


It is not needed to add Deserialization from str, but since I was on it, I thought it could be handy in case we configure chain ID as "1". I am open to remove it too.

m-lord-renkse · 2024-10-29T15:42:03Z

crates/shared/src/account_balances.rs

@@ -67,7 +67,6 @@ pub trait BalanceFetching: Send + Sync {

 /// Contracts required for balance simulation.
 pub struct Contracts {
-    pub chain_id: u64,


It was unused.

m-lord-renkse · 2024-10-29T15:42:25Z

crates/shared/src/bad_token/token_owner_finder.rs

-        match chain_id {
-            1 => &[Self::Liquidity, Self::Blockscout, Self::Ethplorer],
-            100 => &[Self::Liquidity, Self::Blockscout],
-            _ => &[Self::Liquidity],


I removed the non-exhaustive case, otherwise when adding a new network we may overlook this.

m-lord-renkse · 2024-10-29T15:44:06Z

crates/shared/src/signature_validator.rs

@@ -60,7 +60,6 @@ pub fn check_erc1271_result(result: Bytes<[u8; 4]>) -> Result<(), SignatureValid

 /// Contracts required for signature verification simulation.
 pub struct Contracts {
-    pub chain_id: u64,


It was unused.

m-lord-renkse · 2024-10-29T15:44:40Z

crates/solvers/src/infra/config/baseline.rs

@@ -18,8 +18,7 @@ use {
 struct Config {
    /// Optional chain ID. This is used to automatically determine the address
    /// of the WETH contract.
-    #[serde_as(as = "Option<serialize::ChainId>")]


No need to do serde_as when the type implements Deserialization

crates/network/src/lib.rs

MartinquaXD · 2024-10-30T08:01:30Z

crates/network/src/lib.rs

+    /// Returns the block time in milliseconds
+    pub fn block_time_in_ms(&self) -> u64 {
+        match self {
+            Self::Mainnet => 13_000,


Suggested change

Self::Mainnet => 13_000,

Self::Mainnet => 12_000,

This was from legacy code. I corrected it. Maybe @sunce86 had a reason for it? 🤔

No, not too important from the aspect of settlement code where it was used as 13_000

crates/network/src/lib.rs

crates/shared/src/bad_token/token_owner_finder.rs

MartinquaXD · 2024-10-30T08:12:00Z

crates/network/Cargo.toml

@@ -0,0 +1,15 @@
+[package]


I like the idea of encapsulating all the necessary changes to add a new network in a single place. I think there are probably more types basic enough that we could have shared code for them (although this could also lead to another shared crate with too much stuff in there if we are not careful).

Maybe it makes sense to leave this as a separate network crate for now but be open to the idea to turn it into a primitive_types crate later on to also move Address, ContractAddress, TokenAddress, etc. into it.

crates/network/LICENSE-APACHE

crates/solvers/src/infra/contracts.rs

squadgazzz

👍

sunce86

There is a eth::ChainId in driver domain::eth, can you check if you can remove that one as well?

I think crate chain should not represent union of all functionalities related to chains, across all other crates, but rather just an enum and constructor => common code used in all crates, that rarely changes (only when new chain is added) and can be extensively reused as is.

sunce86 · 2024-10-30T15:32:24Z

crates/autopilot/src/domain/settlement/mod.rs

-        infra::blockchain::Id::Sepolia => TARGET_AGE / SEPOLIA_BLOCK_TIME,
-        infra::blockchain::Id::ArbitrumOne => TARGET_AGE / ARBITRUM_ONE_BLOCK_TIME,
-    }
+    chain.blocks_in(TARGET_AGE).round() as u64


nit: blocks_in is only used in autopilot. Just a personal preference to put ONLY COMMON functionalities in chain crate.

yeah, I thought of that, but blocks in X time seemed to me pretty generic, regardless of it is used only by autopilot or not 🤔 it is not domain code, it is generic code. That's why I kept the 1 hour settlement time where it is.

what's more, the code you are commenting on is NOT in the chain crate, but in the autopilot, so I don't know if you confused it 🤔

Yeah, not a big deal for this function in particular. Just wanted to align what we put in chain, as I can forsee that blocks_in will be removed in a few months and that would lead to updating chain crate which we might want to avoid (if we want it to change only when the new chain is added).

crates/autopilot/src/run.rs

sunce86 · 2024-10-30T15:35:45Z

crates/chain/src/lib.rs

+#[repr(u64)]
+pub enum Chain {
+    Mainnet = 1,
+    Goerli = 5,


Why resurrecting Goerli?

Backwards compatibility. Many of the matches account for it. I am up for deleting all its occurrences.

Me to. But I'm also fine adding a separate issue to tackle it in a separate PR.

crates/chain/src/lib.rs

m-lord-renkse · 2024-10-30T15:58:06Z

I think crate chain should not represent union of all functionalities related to chains, across all other crates, but rather just an enum and constructor => common code used in all crates, that rarely changes (only when new chain is added) and can be extensively reused as is.

Yeah, that is exactly what I intended with this PR. By making the enum and consuming it in a non-exhaustive way in the domain logic, we guarantee that adding a new network is seamless, what's more, we avoid a lot of code duplication.

There is a eth::ChainId in driver domain::eth, can you check if you can remove that one as well?

let me check it!

sunce86

Nice

m-lord-renkse force-pushed the unify-networks branch 2 times, most recently from b229b4f to 87bb1f6 Compare October 29, 2024 15:35

m-lord-renkse commented Oct 29, 2024

View reviewed changes

crates/autopilot/src/infra/blockchain/id.rs

@@ -1,43 +0,0 @@

use primitive_types::U256;

Copy link

Contributor Author

m-lord-renkse Oct 29, 2024

Choose a reason for hiding this comment

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

Goodbye duplication

m-lord-renkse commented Oct 29, 2024

View reviewed changes

m-lord-renkse force-pushed the unify-networks branch from 87bb1f6 to 026347b Compare October 29, 2024 15:39

m-lord-renkse commented Oct 29, 2024

View reviewed changes

m-lord-renkse force-pushed the unify-networks branch from 026347b to 6b9f83a Compare October 29, 2024 15:43

m-lord-renkse commented Oct 29, 2024

View reviewed changes

m-lord-renkse force-pushed the unify-networks branch from 6b9f83a to 03d4104 Compare October 29, 2024 15:46

Unify networks and add Base support

7959140

m-lord-renkse force-pushed the unify-networks branch from 03d4104 to 7959140 Compare October 29, 2024 15:48

m-lord-renkse marked this pull request as ready for review October 29, 2024 15:52

m-lord-renkse requested a review from a team as a code owner October 29, 2024 15:52

MartinquaXD reviewed Oct 30, 2024

View reviewed changes

m-lord-renkse added 2 commits October 30, 2024 10:00

rework comments

5583cf9

rework comments

ccdcea6

squadgazzz reviewed Oct 30, 2024

View reviewed changes

crates/solvers/src/infra/contracts.rs Outdated Show resolved Hide resolved

m-lord-renkse added 2 commits October 30, 2024 10:29

adapt test to latest change

f852b45

rework comments

51fa5cc

m-lord-renkse requested a review from MartinquaXD October 30, 2024 09:58

MartinquaXD approved these changes Oct 30, 2024

View reviewed changes

m-lord-renkse force-pushed the unify-networks branch 4 times, most recently from c342c23 to 643242f Compare October 30, 2024 14:21

m-lord-renkse force-pushed the unify-networks branch 3 times, most recently from ef30de3 to 2e49f97 Compare October 30, 2024 14:34

rename

9b2d13e

m-lord-renkse force-pushed the unify-networks branch from 2e49f97 to 9b2d13e Compare October 30, 2024 14:38

m-lord-renkse requested a review from squadgazzz October 30, 2024 14:38

squadgazzz approved these changes Oct 30, 2024

View reviewed changes

sunce86 reviewed Oct 30, 2024

View reviewed changes

rework comments

489fc70

m-lord-renkse requested a review from sunce86 October 31, 2024 09:11

m-lord-renkse and others added 2 commits October 31, 2024 10:11

Merge branch 'main' into unify-networks

6a57edb

nit

a6f8c61

sunce86 approved these changes Oct 31, 2024

View reviewed changes

m-lord-renkse merged commit d417d0b into main Oct 31, 2024
12 checks passed

m-lord-renkse deleted the unify-networks branch October 31, 2024 09:41

github-actions bot locked and limited conversation to collaborators Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify networks and add Base support #3091

Unify networks and add Base support #3091

m-lord-renkse commented Oct 29, 2024

m-lord-renkse Oct 29, 2024

sunce86 Oct 30, 2024

m-lord-renkse Oct 29, 2024

m-lord-renkse Oct 29, 2024

MartinquaXD Oct 30, 2024

m-lord-renkse Oct 30, 2024

m-lord-renkse Oct 29, 2024

m-lord-renkse Oct 29, 2024 •

edited

Loading

m-lord-renkse Oct 29, 2024

m-lord-renkse Oct 29, 2024 •

edited

Loading

m-lord-renkse Oct 29, 2024

m-lord-renkse Oct 29, 2024

MartinquaXD Oct 30, 2024

m-lord-renkse Oct 30, 2024 •

edited

Loading

sunce86 Oct 30, 2024

MartinquaXD Oct 30, 2024

squadgazzz left a comment

sunce86 left a comment •

edited

Loading

sunce86 Oct 30, 2024

m-lord-renkse Oct 30, 2024

m-lord-renkse Oct 30, 2024

sunce86 Oct 30, 2024 •

edited

Loading

sunce86 Oct 30, 2024

m-lord-renkse Oct 30, 2024 •

edited

Loading

sunce86 Oct 30, 2024

m-lord-renkse commented Oct 30, 2024 •

edited

Loading

sunce86 left a comment

Unify networks and add Base support #3091

Unify networks and add Base support #3091

Conversation

m-lord-renkse commented Oct 29, 2024

Description

Changes

How to test

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

Choose a reason for hiding this comment

m-lord-renkse Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-lord-renkse Oct 29, 2024 • edited Loading

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

m-lord-renkse Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squadgazzz left a comment

Choose a reason for hiding this comment

sunce86 left a comment • edited Loading

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

sunce86 Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-lord-renkse Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-lord-renkse commented Oct 30, 2024 • edited Loading

sunce86 left a comment

Choose a reason for hiding this comment

m-lord-renkse Oct 29, 2024 •

edited

Loading

m-lord-renkse Oct 29, 2024 •

edited

Loading

m-lord-renkse Oct 30, 2024 •

edited

Loading

sunce86 left a comment •

edited

Loading

sunce86 Oct 30, 2024 •

edited

Loading

m-lord-renkse Oct 30, 2024 •

edited

Loading

m-lord-renkse commented Oct 30, 2024 •

edited

Loading