refactor: create separate binary crates and move TrinConfig to trin crate #1614

KolbyML · 2024-12-26T10:22:22Z

What was wrong?

#1609 trin reporting versioning is broken. I need versioning working for ping extensions.

This is one PR, but I am going to make another PR fixing versioning.

One issue was that, TrinConfig shouldn't be in Ethportal-api and Trin's binary shouldn't be defined in the global cargo file

This causes a recursive import in ethportal-peertests.

Anyways I am making this PR for multiple reasons, I just haven't felt enough pain to clean things up until now

How was it fixed?

move src/bin/*'s into separate crates
move TrinConfig into Trin's binaries crate. It really shouldn't be in ethportal-api

ogenev

Having the trin cli config in ethportal-api is something that bugged me for a long time, great!

ogenev · 2025-01-06T09:54:33Z

bin/poll-latest/Cargo.toml

+tokio.workspace = true
+tracing.workspace = true
+trin-utils.workspace = true
+url.workspace = true


nit: No new line at the end of file.

ogenev · 2025-01-06T09:54:58Z

bin/purge-invalid-history-content/Cargo.toml

+portalnet.workspace = true
+tracing.workspace = true
+trin-storage.workspace = true
+trin-utils.workspace = true


nit: No new line at the end of file

ogenev · 2025-01-06T10:02:44Z

ethportal-api/Cargo.toml

@@ -2,7 +2,6 @@
 name = "ethportal-api"
 version = "0.4.0"
 description = "Definitions for various Ethereum Portal Network JSONRPC APIs"
-build = "build.rs"


Why the removal of this? In vergen docs it is required: https://docs.rs/vergen/latest/vergen/#usage

I think we should discuss this in this PR #1615

I did some testing and if you are building it doesn't seem required. I also checked other projects which used vergen such as Reth and they removed build = "build.rs" as well.

build = "build.rs" isn't friendly with publishing crates to crates.io and it doesn't seem needed for our usecase.

Fair enough 👍 .

ogenev · 2025-01-06T10:09:14Z

ethportal-api/src/types/cli.rs

Why are we leaving this file in ethportal-api and we are not moving all helper methods to trin/src/cli.rs ?

portal-bridge is using these helper functions too, maybe we can move them to trin-utils instead? What do you think?

Hmm, yeah, if more than one crate uses them, trin-utils seems a reasonable candidate.

ogenev · 2025-01-06T10:22:04Z

trin-storage/src/config.rs

 use r2d2::Pool;
 use r2d2_sqlite::SqliteConnectionManager;

 use crate::{error::ContentStoreError, utils::setup_sql, DistanceFunction};

 const BYTES_IN_MB_U64: u64 = 1000 * 1000;

+/// The storage capacity configurtion.
+#[derive(Debug, Clone, PartialEq, Eq)]


What is the reason for this change?

It can't be stored in the trin binary crate as trin-storage depends on it and I can't think of a reason it should be in ethportal-api

ogenev · 2025-01-06T10:36:22Z

bin/trin/src/cli.rs

+}
+
+impl TrinConfig {
+    pub fn portalnet_config(&self, private_key: B256) -> PortalnetConfig {


Not sure if I like this pattern, I prefer implementing the From trait for PortalnetConfig and RpcConfig.

impl From<TrinConfig> for PortalnetConfig { ... } impl From<TrinConfig> for RpcConfig { ... }

I can do From for RpcConfig, but I can't for impl From<TrinConfig> for PortalnetConfig { as it requires the private_key: B256 argument

OK, we can then change the function name to to_portalnet_config because we are not consuming self and will follow the naming conventions.

ogenev · 2025-01-06T10:37:08Z

rpc/src/config.rs

+use alloy::transports::http::reqwest::Url;
+use ethportal_api::types::{cli::Web3TransportType, network::Subnetwork};
+
+pub struct RpcConfig {


nit: a docstring is always nice

KolbyML · 2025-01-06T11:52:36Z

@ogenev ready for another review, I want to review this PR first #1615

ogenev

Left a comment for moving the helper functions to trin-utils.

Anyways, the PR looks good 👍 .

…rate

njgheorghita · 2025-01-07T16:02:39Z

Cargo.toml

 [workspace]
 members = [
+    "bin/historical-batch", 
+    "bin/poll-latest", 
+    "bin/purge-invalid-history-content", 


hmmm. now it seems like the overhead for including these single-task binaries is higher than before. given that we don't really use these in production, I'd be in favor of removing all of them, except for one, to leave a framework for how to do it in future scenarios. Though, this can be done in a follow-up pr np

ok which one do you want to keep

For tooling we could make x,y,z subcommands in the trin cli, which would make there versions be tied. I think the purge script makes sense as a Trin sub command specifically

Idk... honestly does it make sense to keep any of them around? None of them are used / have been in use recently afaik. And maybe we should migrate these kinds of tasks to the "subcommand" pattern, which might enforce a bit more of a thorough design/plan rather than these single-use scripts, which are a bit hacky....

Ok I will remove them all then, as I don't have any specific attachment to them either

njgheorghita · 2025-01-07T16:11:43Z

bin/trin/src/cli.rs

-use crate::{
-    types::{bootnodes::Bootnodes, network::Subnetwork},
-    version::FULL_VERSION,
+use ethportal_api::{


I feel like this refactor should have removed some dependencies from ethportal-api? At least clap ... maybe url?

njgheorghita · 2025-01-07T16:18:47Z

Cargo.toml

 [workspace]
 members = [
+    "bin/historical-batch", 
+    "bin/poll-latest", 
+    "bin/purge-invalid-history-content", 


Idk... honestly does it make sense to keep any of them around? None of them are used / have been in use recently afaik. And maybe we should migrate these kinds of tasks to the "subcommand" pattern, which might enforce a bit more of a thorough design/plan rather than these single-use scripts, which are a bit hacky....

njgheorghita

You'll also need to remove the refs to the deleted scripts in the dockerfiles

KolbyML · 2025-01-07T16:33:26Z

You'll also need to remove the refs to the deleted scripts in the dockerfiles

good catch

KolbyML force-pushed the restructure-trin branch 2 times, most recently from 540182a to bcbee34 Compare December 27, 2024 09:28

KolbyML marked this pull request as ready for review December 27, 2024 09:31

KolbyML requested review from carver, njgheorghita, ogenev and morph-dev December 27, 2024 09:32

KolbyML force-pushed the restructure-trin branch 3 times, most recently from e16cdf9 to 3dbc59a Compare December 27, 2024 11:58

KolbyML mentioned this pull request Dec 27, 2024

feat: implement ping extensions #1616

Open

KolbyML force-pushed the restructure-trin branch from 3dbc59a to a7a3588 Compare December 28, 2024 09:41

ogenev reviewed Jan 6, 2025

View reviewed changes

KolbyML force-pushed the restructure-trin branch from 2b8035f to 23b3e4b Compare January 6, 2025 11:51

ogenev approved these changes Jan 7, 2025

View reviewed changes

KolbyML added 4 commits January 7, 2025 08:57

refactor: create separate binary crates and move TrinConfig to trin c…

8da4e03

…rate

fix: address PR concerns

1674320

fix: resolve PR concerns

583edcc

fix: ci

9af91d9

KolbyML force-pushed the restructure-trin branch from 5a74998 to 9af91d9 Compare January 7, 2025 15:57

njgheorghita reviewed Jan 7, 2025

View reviewed changes

fix: clean up rebase

05bd46a

njgheorghita approved these changes Jan 7, 2025

View reviewed changes

fix: resolve PR concerns

c2e8202

njgheorghita reviewed Jan 7, 2025

View reviewed changes

fix: clean up docker files

cf510f2

fix: lower ci parallelism now that we have less crates

836b11c

KolbyML merged commit c5de5a5 into ethereum:master Jan 7, 2025
9 checks passed

KolbyML mentioned this pull request Jan 13, 2025

fix: update submodule and missing workspace member #1633

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: create separate binary crates and move TrinConfig to trin crate #1614

refactor: create separate binary crates and move TrinConfig to trin crate #1614

KolbyML commented Dec 26, 2024 •

edited

Loading

ogenev left a comment

ogenev Jan 6, 2025

ogenev Jan 6, 2025

ogenev Jan 6, 2025

KolbyML Jan 6, 2025

ogenev Jan 7, 2025

ogenev Jan 6, 2025

KolbyML Jan 6, 2025

ogenev Jan 7, 2025

ogenev Jan 6, 2025

KolbyML Jan 6, 2025

ogenev Jan 6, 2025

KolbyML Jan 6, 2025

ogenev Jan 7, 2025

ogenev Jan 6, 2025

KolbyML commented Jan 6, 2025

ogenev left a comment

njgheorghita Jan 7, 2025 •

edited

Loading

KolbyML Jan 7, 2025

KolbyML Jan 7, 2025

njgheorghita Jan 7, 2025

KolbyML Jan 7, 2025

njgheorghita Jan 7, 2025

KolbyML Jan 7, 2025

njgheorghita Jan 7, 2025

njgheorghita left a comment

KolbyML commented Jan 7, 2025

refactor: create separate binary crates and move TrinConfig to trin crate #1614

refactor: create separate binary crates and move TrinConfig to trin crate #1614

Conversation

KolbyML commented Dec 26, 2024 • edited Loading

What was wrong?

How was it fixed?

ogenev 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KolbyML commented Jan 6, 2025

ogenev left a comment

Choose a reason for hiding this comment

njgheorghita Jan 7, 2025 • 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

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

njgheorghita left a comment

Choose a reason for hiding this comment

KolbyML commented Jan 7, 2025

KolbyML commented Dec 26, 2024 •

edited

Loading

njgheorghita Jan 7, 2025 •

edited

Loading