magic-wormhole.rs icon indicating copy to clipboard operation
magic-wormhole.rs copied to clipboard

Send client version with bind message

Open wuan opened this issue 2 years ago • 9 comments

This PR sets the client_version field in the bind message (see magic-wormhole/magic-wormhole/src/wormhole_rendezvous.py:182). So far this is only being used by the Python and Wormhole-William (Go) clients.

It will use the following mapping:

implementation rust
version <package-version>

wuan avatar Dec 02 '22 20:12 wuan

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 :tada:

Comparison is base (46eceb0) 43.73% compared to head (00842fb) 43.74%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
+ Coverage   43.73%   43.74%   +0.01%     
==========================================
  Files          18       18              
  Lines        2552     2558       +6     
==========================================
+ Hits         1116     1119       +3     
- Misses       1436     1439       +3     
Impacted Files Coverage Δ
src/core.rs 78.91% <100.00%> (+0.38%) :arrow_up:
src/core/server_messages.rs 68.57% <100.00%> (+0.92%) :arrow_up:

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Dec 02 '22 21:12 codecov[bot]

This field does not seem to be specified in the protocols. We should fix that

piegamesde avatar Dec 02 '22 21:12 piegamesde

Using lifetimes here has the effect of avoiding about two allocations in total and is total overkill. But if you really want to have a borrowed version of the struct, then use Cow instead please.

piegamesde avatar Dec 02 '22 22:12 piegamesde

I'm new to Rust and need to find out how to avoid lifetimes then. Should I prefer to use String instead of &str to achieve that?

wuan avatar Dec 02 '22 22:12 wuan

Just clone liberally. And if you find yourself cloning too much, wrap in a reference counter to make it cheaper.

One of the key lessons most beginners eventually learn is that Rust gives you the possibility to be efficient, but it's not always necessary/productive to make use of that. (Incidentally, I think this is also where a lot of the "fighting the borrow checker" meme comes from)

piegamesde avatar Dec 02 '22 22:12 piegamesde

The solution is now hopefully clean enough.

wuan avatar Jan 04 '23 16:01 wuan

Yes, it is.

Unless this PR is urgent, I'd like to discuss the client-version field in the protocol before merging this. Depending on what the values mean, I'd personally prefer CLIENT_NAME = "wormhole-rs"; over "rust". On the other hand, there may exist multiple clients using this library, should the value reflect that or not?

piegamesde avatar Jan 05 '23 17:01 piegamesde

On the other hand, there may exist multiple clients using this library, should the value reflect that or not?

That would be a future update to enable overriding this information by the caller, like in https://github.com/psanford/wormhole-william/blob/master/rendezvous/options.go.

wuan avatar Mar 15 '23 09:03 wuan

https://github.com/magic-wormhole/magic-wormhole-protocols/issues/35

piegamesde avatar Mar 15 '23 13:03 piegamesde