`Framing` refactor: `framing.rs` api #1033

jbesraa · 2024-07-04T12:07:14Z

framing-sv2 Crate refactor p3, previous prs #982 #976
Partially resolves #903
In this pr I started working on the exposed API from framing.rs by handling the errors in a nicer way and also be more consistent with the naming

Fi3 · 2024-07-04T13:33:17Z

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

+#[derive(Debug)]
+pub enum Frame<T, B>
+where
+    T: Serialize + GetSize,


Best practice is to constrain generics only where you need it.

In the impl block where you have function that need the generics to be constrained

Having naked generics makes it really hard to read the enum

I don't get it. Why is harder?

Maybe you want to use InnerType and Buffer instead of T and B ?

previously it was only Frame<T,B>{ HandShake(HandShakeFrame), Sv2(Sv2Frame<T,B>) without any info about T and B

I dont feel very strongly about it, but I would appreciate reviewing the full PR (:

I went trough the code everything look very good to me.

what is the outcome here?

@Fi3 do you still feel need we shouldn't be constraining generics here? or looking at the rest of the PR change your mind?

should we drop this commit from the PR?

constrain generics where not needed is bad, so my opinion is the same. If you are concerned about readability you can use Frame<InnerType,Buffer> instead of Frame<T,B>

Fi3 · 2024-07-10T14:21:53Z

roles/jd-client/src/lib/upstream_sv2/upstream.rs

        // Gets the message payload
-        let payload = incoming.payload();
+        let payload = match incoming.payload() {


This seems like a place where clippy would complain to use if let Some . But if clippy is happy I'm as well. Maybe since you are at it you could just remove the above comment.

roles/pool/src/lib/mining_pool/mod.rs

plebhash · 2024-07-15T22:05:09Z

@jbesraa it seems we're breaking sv2-ffi

https://github.com/stratum-mining/stratum/actions/runs/9889295180/job/27315027024?pr=1033#step:8:437

jbesraa · 2024-07-16T14:04:29Z

oops missed this, it should be fixed now @plebhash

`Frame` is defined with `T` and `B` generics but the constraints are only introduced in the impl level which makes it harder to read the enum and understand whats those generics are about. As those generics are a key part of the `Frame` enum, it makes more sense to introduce the constraints in the enum level.

..instead of panicking

Remove `Option` from its return type.

`Sv2Frame` can handle serialized and non-serliazed data, both scenarios were previously in the same struct wrapped by Option, where if one is Some, the other is None but never both None or Some.

plebhash · 2024-08-20T22:47:28Z

@jbesraa FYI I solved some conflicts here, which is why I'm co-authoring the commits

plebhash · 2024-08-20T23:08:42Z

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

@@ -44,11 +44,9 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> From<Sv2Frame<T, B>>

 /// Abstraction for a SV2 Frame.


we should expand this comment to make it clear what each enum variant is

maybe something like:

/// Both `Payload` and `Raw` variants carry the `Header` as metadata /// `Payload` means the `Sv2Frame` carries a payload, but it has not yet been serialized /// `Raw` means the `Sv2Frame` has been fully serialized, where the `serialized` field contains both `header` + `payload`

updating the comments is especially important given that these are used on the Rust Docs, so they need to follow these changes accordingly

plebhash · 2024-08-20T23:11:52Z

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

@@ -57,23 +55,23 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
    /// When called on a non serialized frame, it is not so cheap (because it serializes it).


we should update these comments so that they are coherent with the new behavior of this function

updating the comments is especially important given that these are used on the Rust Docs, so they need to follow these changes accordingly

plebhash · 2024-08-20T23:12:18Z

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

@@ -83,16 +81,18 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
    /// already serialized payload. If the frame has not yet been


we should update these comments so that they are coherent with the new behavior of this function

updating the comments is especially important given that these are used on the Rust Docs, so they need to follow these changes accordingly

plebhash · 2024-08-20T23:13:17Z

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

@@ -91,8 +91,8 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
    }

    /// `Sv2Frame` always returns `Some(self.header)`.


we should update these comments so that they are coherent with the new behavior of this function

updating the comments is especially important given that these are used on the Rust Docs, so they need to follow these changes accordingly

plebhash · 2024-08-20T23:16:20Z

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

@@ -113,10 +113,9 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
    pub fn from_bytes_unchecked(mut bytes: B) -> Self {


Github doesn't allow me to add this comment on the lines above. Ideally this comment should be placed on line 98, but placing it here should be sufficient

the comments on from_bytes are outdated with regards to the new enum-based Sv2Frame

we should update these comments so that they are coherent with the new behavior of this function

updating the comments is especially important given that these are used on the Rust Docs, so they need to follow these changes accordingly

plebhash · 2024-08-20T23:17:07Z

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

@@ -150,13 +149,9 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
    /// otherwise, returns the length of `self.payload`.


we should update these comments so that they are coherent with the new behavior of this function

updating the comments is especially important given that these are used on the Rust Docs, so they need to follow these changes accordingly

plebhash

strong ACK on converting Sv2Frame from a struct into an enum

the previous APIs around the struct were very confusing for the user, and also had some panics on edge cases, which IMHO is not the best way to design an API

so this is making things much better!

however, we need to also update all comments accordingly, otherwise we will be generating outdated Rust Docs

I'm also seeing some CI things blowing up after I solved a conflict

plebhash · 2024-08-20T23:57:52Z

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

@@ -83,16 +81,18 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
    /// already serialized payload. If the frame has not yet been
    /// serialized, this function should never be used (it will panic).
    pub fn payload(&mut self) -> Option<&mut [u8]> {


shouldn't we rename this method?

with the new enum we are sort of establishing a convention where the Payload variant signals that Sv2Frame has not yet been serialized

so calling a method that is named payload for a Sv2Frame::Payload and getting a None is extremely counter intuitive!

I feel we would create a more sane user experience if this method was called raw or serialized

jbesraa · 2024-08-21T04:02:46Z

@jbesraa FYI I solved some conflicts here, which is why I'm co-authoring the commits

Thanks for doing that. Please next time just write a comment and I'll rebase. It's gonna be tricky for me now to understand what you did or what were the changes in dev that caused conflicts.

plebhash · 2024-08-21T23:10:54Z

@jbesraa FYI I solved some conflicts here, which is why I'm co-authoring the commits

Thanks for doing that. Please next time just write a comment and I'll rebase. It's gonna be tricky for me now to understand what you did or what were the changes in dev that caused conflicts.

it was some small conflicts on roles/mining-proxy/src/lib/downstream_mining.rs, which came from merging #1021

basically I just had to adjust some calls to payload and header so they followed the changes introduced on this PR

but I understand how that was potentially counter-productive on your side, and I will avoid doing this again in the future. apologies again!

plebhash · 2024-09-03T16:52:10Z

let's revert this to draft for now, we should aim to finish docs and have a refined and wide collective understanding of protocols crates, which will be addressed by the documentation issues listed on #845

rrybarczyk · 2024-09-04T13:56:01Z

Converted to draft until all documentation issues in #845 are addressed, all refactor issues are complete, and we have team discussions/consensus on path forwards to protocols refactoring.

jbesraa force-pushed the 2024-07-02-framing-refactor-p3 branch 2 times, most recently from e87cba7 to 9474b3b Compare July 4, 2024 12:09

Fi3 reviewed Jul 4, 2024

View reviewed changes

jbesraa force-pushed the 2024-07-02-framing-refactor-p3 branch 6 times, most recently from f30003a to d185f35 Compare July 8, 2024 11:44

jbesraa marked this pull request as ready for review July 8, 2024 11:45

jbesraa changed the title ~~Framing refactor: fix functions signature in framing.rs~~ Framing refactor: framing.rs api Jul 8, 2024

Fi3 reviewed Jul 10, 2024

View reviewed changes

roles/pool/src/lib/mining_pool/mod.rs Show resolved Hide resolved

jbesraa force-pushed the 2024-07-02-framing-refactor-p3 branch from d185f35 to aaa542b Compare July 11, 2024 09:51

jbesraa mentioned this pull request Jul 11, 2024

Framing refactor: test Sv2Frame and HandShakeFrame #1049

Draft

jbesraa force-pushed the 2024-07-02-framing-refactor-p3 branch from aaa542b to 3c322e4 Compare July 16, 2024 13:54

plebhash self-assigned this Jul 16, 2024

This was referenced Jul 17, 2024

Refactor ping pong example #977

Draft

Refactor ping pong encrypted example #1007

Draft

rrybarczyk requested a review from plebhash July 17, 2024 13:58

plebhash mentioned this pull request Jul 20, 2024

Rust Docs + refactor: protocols crates #845

Open

67 tasks

plebhash linked an issue Aug 20, 2024 that may be closed by this pull request

refactor: framing_sv2 crate #903

Open

jbesraa added 4 commits August 20, 2024 19:44

Change payload fn signature to return Option

e613a46

..instead of panicking

Change Sv2Frame::get_header to header and ..

d0aae41

Remove `Option` from its return type.

Change Sv2Frame from struct to enum

d1d9247

`Sv2Frame` can handle serialized and non-serliazed data, both scenarios were previously in the same struct wrapped by Option, where if one is Some, the other is None but never both None or Some.

plebhash force-pushed the 2024-07-02-framing-refactor-p3 branch from 3c322e4 to d1d9247 Compare August 20, 2024 22:46

plebhash reviewed Aug 20, 2024

View reviewed changes

plebhash requested changes Aug 20, 2024

View reviewed changes

plebhash reviewed Aug 20, 2024

View reviewed changes

rrybarczyk self-requested a review August 21, 2024 18:09

rrybarczyk added protocols Lowest level protocol logic refactor Implies refactoring code labels Aug 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Framing` refactor: `framing.rs` api #1033

`Framing` refactor: `framing.rs` api #1033

jbesraa commented Jul 4, 2024 •

edited

Loading

Fi3 Jul 4, 2024

Fi3 Jul 4, 2024

jbesraa Jul 10, 2024

Fi3 Jul 10, 2024

Fi3 Jul 10, 2024

jbesraa Jul 10, 2024

jbesraa Jul 10, 2024

Fi3 Jul 10, 2024

plebhash Aug 20, 2024 •

edited

Loading

Fi3 Aug 21, 2024

Fi3 Jul 10, 2024

plebhash commented Jul 15, 2024 •

edited

Loading

jbesraa commented Jul 16, 2024

plebhash commented Aug 20, 2024

plebhash Aug 20, 2024 •

edited

Loading

plebhash Aug 20, 2024

plebhash Aug 20, 2024

plebhash Aug 20, 2024

plebhash Aug 20, 2024

plebhash Aug 20, 2024

plebhash left a comment •

edited

Loading

plebhash Aug 20, 2024 •

edited

Loading

jbesraa commented Aug 21, 2024

plebhash commented Aug 21, 2024

plebhash commented Sep 3, 2024

rrybarczyk commented Sep 4, 2024

		@@ -44,11 +44,9 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> From<Sv2Frame<T, B>>

		/// Abstraction for a SV2 Frame.

		@@ -57,23 +55,23 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
		/// When called on a non serialized frame, it is not so cheap (because it serializes it).

		@@ -83,16 +81,18 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
		/// already serialized payload. If the frame has not yet been

		@@ -91,8 +91,8 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
		}

		/// `Sv2Frame` always returns `Some(self.header)`.

		@@ -113,10 +113,9 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
		pub fn from_bytes_unchecked(mut bytes: B) -> Self {

		@@ -150,13 +149,9 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
		/// otherwise, returns the length of `self.payload`.

Framing refactor: framing.rs api #1033

Are you sure you want to change the base?

Framing refactor: framing.rs api #1033

Conversation

jbesraa commented Jul 4, 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

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

plebhash Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plebhash commented Jul 15, 2024 • edited Loading

jbesraa commented Jul 16, 2024

plebhash commented Aug 20, 2024

plebhash Aug 20, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plebhash left a comment • edited Loading

Choose a reason for hiding this comment

plebhash Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

jbesraa commented Aug 21, 2024

plebhash commented Aug 21, 2024

plebhash commented Sep 3, 2024

rrybarczyk commented Sep 4, 2024

`Framing` refactor: `framing.rs` api #1033

`Framing` refactor: `framing.rs` api #1033

jbesraa commented Jul 4, 2024 •

edited

Loading

plebhash Aug 20, 2024 •

edited

Loading

plebhash commented Jul 15, 2024 •

edited

Loading

plebhash Aug 20, 2024 •

edited

Loading

plebhash left a comment •

edited

Loading

plebhash Aug 20, 2024 •

edited

Loading