refactor: move out offer/find_content/find_nodes functions from manager.rs

[KolbyML]

What was wrong?

our OverlayService code in manager.rs is massive and feels unorganized and is a little hard to go through. I was talking to @morph-dev a month ago and he said he would prefer large refactors like this should be in a separate PR from PR's trying to make logic changes.

I was implementing ethereum/portal-network-specs#383 I wanted to refactor the calls for find_content like I did when I refactored the ping functions out of manager.rs when I implemented ping extensions.

I am now doing it for the 3 other content messages I didn't refactor out, since manager.rs is super big and it is hard to look through when I am trying to search through the functions for a specific message domain e.x.

• ping
• offer
• find content
• find node

How was it fixed?

Move the functions which belong to the 3 domains left to refactor the same way I did it for ping

• offer
• find content
• find node

[Copilot]

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

crates/portalnet/src/utp/controller.rs:3

• Verify that alloy::primitives::Bytes fully replaces bytes::Bytes functionality across all usages to avoid unintentional behavior changes.

use alloy::primitives::Bytes;

crates/portalnet/src/overlay/service/find_nodes.rs:192

• [nitpick] Consider refactoring this condition for clarity by separating the local node filtering from the duplicate check.

if !query_info.untrusted_enrs.iter().any(|enr| enr.node_id() == enr_ref.node_id() && enr.node_id() != local_node_id)

[carver]

This is way too large to do a real review on. So it's definitely a good thing it's in its own PR! I randomly selected ~10 functions to double-check that the logic didn't change in the refactor, and it all looked good to me.

Is there anything worth pointing out for review that wasn't just a straight move?

At some point, it would be nice to find a way to actually split the responsibilities without sharing the struct everywhere. This PR is a meaningful improvement, but it still leaves the logic sprawling too much to fully reason about. But I don't have better suggestion for architecture at the moment. So I'm happy with taking this step forward. 👌🏻

[carver]

Typo in comment. Plus, it feels strange to name the variable enrs, though I understand the type constrains it for now. It would be possible to make it generic on a type that implements ssz encoding, I suppose.

Suggested change

	/// Limits a to a maximum packet size, including the discv5 header overhead.
	pub fn pop_while_ssz_bytes_len_gt(enrs: &mut Vec<SszEnr>, max_size: usize) {
	while enrs.ssz_bytes_len() > max_size {
	enrs.pop();
	/// Limits to a maximum packet size, including the discv5 header overhead.
	pub fn pop_while_ssz_bytes_len_gt(elements: &mut Vec<SszEnr>, max_size: usize) {
	while elements.ssz_bytes_len() > max_size {
	elements.pop();

No strong feelings on any of this except the docstring. :)

[KolbyML]

/// Limits the elements count to a maximum packet size, including the discv5 header overhead.
pub fn pop_while_ssz_bytes_len_gt<SSZObject: Encode>(
    elements: &mut Vec<SSZObject>,
    max_size: usize,
) {
    while elements.ssz_bytes_len() > max_size {
        elements.pop();
    }
}

I ended up just making the function generic as if we are changing the variable name I thought we might as well fully commit to where your suggestion was headed, and expanded on your comment suggestion a bit, as I was still a little confused reading it even after removing the a

[KolbyML]

Is there anything worth pointing out for review that wasn't just a straight move?

It was straight forward pretty much just copy pasting stuff

At some point, it would be nice to find a way to actually split the responsibilities without sharing the struct everywhere. This PR is a meaningful improvement, but it still leaves the logic sprawling too much to fully reason about. But I don't have better suggestion for architecture at the moment. So I'm happy with taking this step forward. 👌🏻

I agree. I think this is a good first step as all these functions are better sorted now on what they are actually for. I think this PR will make whatever refactor we do end up doing in the future easier.

[morph-dev]

I had a quick look at it and I like the direction. Thank you for doing it!

I assumed no functionality changes I didn't spend time actually looking at the code, just the function names.

One thing that I would suggest if it easy enough (I didn't check it myself) is to also move tests into relevant files.