magic-wormhole / magic-wormhole.rs

Rust implementation of Magic Wormhole, with new features and enhancements
European Union Public License 1.2
648 stars 72 forks source link

Send client version with bind message #176

Open wuan opened 1 year ago

wuan commented 1 year ago

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>
codecov[bot] commented 1 year ago

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 ```diff @@ 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](https://codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/176?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole) | Coverage Δ | | |---|---|---| | [src/core.rs](https://codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/176?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole#diff-c3JjL2NvcmUucnM=) | `78.91% <100.00%> (+0.38%)` | :arrow_up: | | [src/core/server\_messages.rs](https://codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/176?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole#diff-c3JjL2NvcmUvc2VydmVyX21lc3NhZ2VzLnJz) | `68.57% <100.00%> (+0.92%)` | :arrow_up: | ... and [3 files with indirect coverage changes](https://codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/176/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole) Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole)

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

piegamesde commented 1 year ago

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

piegamesde commented 1 year ago

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.

wuan commented 1 year ago

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?

piegamesde commented 1 year ago

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)

wuan commented 1 year ago

The solution is now hopefully clean enough.

piegamesde commented 1 year ago

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?

wuan commented 1 year ago

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.

piegamesde commented 1 year ago

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