Derive Clone for HandshakeComplete

[mycognosist]

Simply derives Clone for the HandshakeComplete struct and addresses clippy lints related to needless borrows.

@adria0, would you be able to merge this? I don't have write permissions for the handshake repo.

[mycognosist]

I've added three commits which should address all clippy warnings, as well as a test warning.

[Dhole]

The changes look good to me! Nevertheless I see from the github actions that it's not building, do you know what's happening? The error is in lib.rs but you didn't touch that file in this PR.

[mycognosist]

@Dhole

Thanks for taking a look! The build is failing due to an "ambiguous glob re-exports" error:

error: ambiguous glob re-exports
  --> src/lib.rs:16:9
   |
16 | pub use boxstream::*;
   |         ^^^^^^^^^^^^ the name `Result` in the type namespace is first re-exported here
17 | pub use handshake::*;
   |         ------------ but the name `Result` in the type namespace is also re-exported here
   |
   = note: `-D ambiguous-glob-reexports` implied by `-D warnings`

error: ambiguous glob re-exports
  --> src/lib.rs:16:9
   |
16 | pub use boxstream::*;
   |         ^^^^^^^^^^^^ the name `Error` in the type namespace is first re-exported here
17 | pub use handshake::*;
   |         ------------ but the name `Error` in the type namespace is also re-exported here

error: could not compile `kuska-handshake` (lib) due to 2 previous errors

I haven't touched that file and I'm not able to replicate the error locally when running the build command (probably because I'm running an older version of the compiler and that specific lint was only introduced earlier this year):

RUSTFLAGS="-D warnings" cargo build --verbose --all-features --all-targets

[mycognosist]

I'm taking a closer look at thiserror to see if I can create a top-level error type which unifies the separate boxstream and handshake errors (in a transparent way that doesn't cause any breakages to downstream libraries and applications).

Solving the Result issue is easier: just define the result in src/lib.rs and use that in the boxstream and handshake modules.

[Dhole]

I'm taking a closer look at thiserror to see if I can create a top-level error type which unifies the separate boxstream and handshake errors (in a transparent way that doesn't cause any breakages to downstream libraries and applications).

Solving the Result issue is easier: just define the result in src/lib.rs and use that in the boxstream and handshake modules.

I was able to reproduce the build failure with Rust 1.73.0

Perhaps another solution is the following:

• export boxstream::Error and boxstream::Result as BoxstreamError, BoxstreamResult
• export handhsake::Error and handshake::Result as HandshakeError, HandshakeResult
• Create a new error in lib.rs called Error with From and From, which is an enum with two cases, one for BoxstreamError and one for HandshakeError
• Define in lib.rs pub type Result<T> = std::result::Result<T, Error>

Although this may still create downstream problems if an application is matching on the Error cases, which now will be different for the top level Error.

I guess your idea is to merge both errors into a single one? That should not break any downstream usage, but it feels a bit wrong to flatten out the error cases of the two modules 🤔

[mycognosist]

The solution you outlined is exactly what I was thinking (using the #[error(transparent)] attribute on each of the two enum variants so that the Display impl of each underlying error would be used).

I've been playing around with a few options this morning but the solution outlined above ends up breaking https://github.com/Kuska-ssb/ssb.

There's another solution which seems to work for me; simply replace the glob re-exports with explicit (named) re-exports and alias the error and result types:

pub use boxstream::{
    BoxStreamRecv, BoxStreamSend, Error as BoxstreamError, Header, Result as BoxstreamResult,
};
pub use handshake::{
    Error as HandshakeError, Handshake, Result as HandshakeResult, SendServerAccept,
};

That solution requires minimal changes to the handshake crate, solves the clippy warnings and does not cause any breakage in https://github.com/Kuska-ssb/ssb. Seems like a good outcome?

[ed255]

There's another solution which seems to work for me; simply replace the glob re-exports with explicit (named) re-exports and alias the error and result types:

pub use boxstream::{
    BoxStreamRecv, BoxStreamSend, Error as BoxstreamError, Header, Result as BoxstreamResult,
};
pub use handshake::{
    Error as HandshakeError, Handshake, Result as HandshakeResult, SendServerAccept,
};

That solution requires minimal changes to the handshake crate, solves the clippy warnings and does not cause any breakage in https://github.com/Kuska-ssb/ssb. Seems like a good outcome?

This solution looks great to me! And I think it's very clean :)

[Dhole]

On the last run, the examples didn't build with this error:

  --> examples/handshake.rs:12:5
   |
12 |     SharedSecret,
   |     ^^^^^^^^^^^^ no `SharedSecret` in the root

I guess SharedSecret needs to be exported too.

BTW, the message from @ed255 was myself from my other github account 😅

[mycognosist]

Sorry for all the noise with these failed builds! The commit I just pushed should pass 🤞🏻

BTW, the message from @ed255 was myself from my other github account 😅

I was a bit confused at first but then I saw your bio and understood :)

[Dhole]

LGTM! Thanks for all the extra work you did to make all the github actions pass 😄