refactor: framing_sv2 crate

[plebhash]

This task is an outcome of #845

While documenting the framing_sv2 crate, a lot of things stood out as needing to refactor.

The documentation effort was an immediate action, while a refactoring (which implies breaking API changes) is not so urgent priority, so for now we should leave this in the backlog for an appropriate moment in the future.

Below are the different items that I believe deserve to be rewritten on framing_sv2 crate.

rename framing2.rs

the character 2 in filename framing2.rs seems to make some kind of allusion to SV2, but this isn't following any pattern or convention in naming files.

so IMO this file should be renamed to framing.rs.

Frame trait removal

The Frame trait is unnecessary. It is never really used anywhere in the code as a Trait bound. The only usage of this trait is in the impl Frame blocks for Sv2Frame and HandShakeFrame. I even did a quick experiment where I commented out the Frame trait declaration and with small adaptations, I easily got the code to compile and run without errors.

The Frame trait makes some impositions into the APIs of Sv2Frame and HandShakeFrame structs by trying to unify them logically (they're both kinds of "frame"). But unfortunately, the resulting APIs are unintuitive, more complex, and dirtier than they need to be.

For example, the Sv2Frame::payload method is emitting a panic! when the API is used incorrectly. Ideally, the API should be built so that there's no footguns on when this function is used.

stratum/protocols/v2/framing-sv2/src/framing2.rs

Lines 132 to 145 in 24b8b1d

	// self can be either serialized (it cointain an AsMut<[u8]> with the serialized data or
	// deserialized it contain the rust type that represant the Sv2 message. If the type is
	// deserialized self.paylos.is_some() is true. To get the serialized payload the inner type
	// should be serialized and this function should never be used, cause is intended as a fast
	// function that return a reference to an already serialized payload. For that the function
	// panic.
	fn payload(&'a mut self) -> &'a mut [u8] {
	if let Some(serialized) = self.serialized.as_mut() {
	&mut serialized.as_mut()[Header::SIZE..]
	} else {
	// panic here is the expected behaviour
	panic!()
	}
	}

Since the Frame trait is never used anywhere else, there's only penalty for its existence (confusing APIs), for no profit.

I propose we remove Frame trait and refactor the APIs of Sv2Frame and HandShakeFrame so they become more concise and intuitive.

code organization

impl blocks should be written immediately after the declaration of the respective struct or enum.

We should avoid mixing impl blocks for different entities together. If they need to co-exist in the same file, they should be grouped logically in different regions of the file, so the reader can easily find methods right next to the declaration of the struct or enum they belong to.

sv2_frame::get_header

note from @Fi3:

Note for the future, it could make sense to return a type that encode the above infos: enum Ok, Bigger, Smaller. This would be an API change so for now I would live it like it is

error handling and edge cases

• strict discipline over error handling (no unwrap, expect)
• strict discipline over edge cases (no unimplemented!, todo!, panic!, unreachable!, etc)

[average-gary]

code organization

Is this enforceable somehow?
If this is desirable, the CONTRIBUTING.md should be updated as well. (#1030)

[Fi3]

I would allow unreachable and expect

[Fi3]

A not on the payload panic. The API can and must be refactored in a way that avoid it just return a Result. When that is not possible (for reason x) IMO is better to panic and fail fast, rather the let the user use something in a wrong way.

[GitGab19]

New issue opened by @Fi3 could be related to this: #1147

[plebhash]

as discussed here #858 (comment)

the usage of generic B and the type alias Slice is a bit confusing... both are aiming to achieve very similar goals but in a redundant way

• Sv2Frame<T, B> has generic B to allow the user to decide which kind of buffer they want (which could be anything, as long as it satisfies the trait contraints).
• HandShakeFrame uses the Slice alias to choose between Vec<u8> vs buffer_sv2::Slice, and this choice is enacted via the with_buffer_pool

we should replace B with Slice everywhere and restrict user choices around Buffer kinds to only with_buffer_pool feature flag

as a result, both enum Frame<T> and struct Sv2Frame<T> should be generic only over T

[rrybarczyk]

In framing.rs, unit tests are handing without a mod tests block.

[jbesraa]

In framing.rs, unit tests are handing without a mod tests block.

This was addressed here 05f9b4c