Don't take ownership of key in SignedEnvelope::new (#2510)

[laurentsenta]

Fixes issues #2510

• don't take ownership of key in SignedEnvelope::new
• clarify if we want / should use a ref through all the code instead of making a clone
• don't take ownership of key in PeerContrustor::new

[mxinden]

clarify if we want / should use a ref through all the code instead of making a clone

I am not quite sure I follow. Could you expand on this @laurentsenta?

[mxinden]

To reproduce CI testfailures locally you can run cd core/ && cargo test.

[laurentsenta]

@mxinden
regarding:

clarify if we want / should use a ref through all the code instead of making a clone

According to this doc, the keypair is the one used to authenticate every connection. But it looks like we clone this key when we want to use it.

Would that make sense to make the relationship explicit by using a reference in the PeerRecord too, instead of a clone?
So similar to borrowing the key in SignedEnvelop, should we also borrow the key when we instantiate a PeerRecord?

[mxinden]

clarify if we want / should use a ref through all the code instead of making a clone

According to this doc, the keypair is the one used to authenticate every connection. But it looks like we clone this key when we want to use it.

Would that make sense to make the relationship explicit by using a reference in the PeerRecord too, instead of a clone? So similar to borrowing the key in SignedEnvelop, should we also borrow the key when we instantiate a PeerRecord?

Good callout @laurentsenta. Off the top of my head there is no reason why PeerRecord and SignedRecord should not take a reference only. Maybe @thomaseizinger has thoughts here?

[thomaseizinger]

clarify if we want / should use a ref through all the code instead of making a clone

According to this doc, the keypair is the one used to authenticate every connection. But it looks like we clone this key when we want to use it.

Would that make sense to make the relationship explicit by using a reference in the PeerRecord too, instead of a clone? So similar to borrowing the key in SignedEnvelop, should we also borrow the key when we instantiate a PeerRecord?

Good callout @laurentsenta. Off the top of my head there is no reason why PeerRecord and SignedRecord should not take a reference only. Maybe @thomaseizinger has thoughts here?

Yes, skimming the code it makes sense :)

[laurentsenta]

Thanks for the feedbacks! I pushed another commit with the tweak,
The PR is code complete, cargo check & tests are passing on my machine

[laurentsenta]

Thanks for the review, @thomaseizinger! could you merge, or should I ping Max? I don't have write access on the repo yet.

[mxinden]

Missing a changelog entry in core/CHANGELOG.md. Otherwise looks good to me.

[laurentsenta]

@mxinden Fixed, thanks for the check!

[mxinden]

Thanks for the help @laurentsenta.

[mxinden]

First patch merged @laurentsenta 🎉