Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send client version with bind message #176

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wuan
Copy link
Contributor

@wuan wuan commented Dec 2, 2022

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 wuan force-pushed the send_client_version branch 3 times, most recently from 17aa206 to 1d9e1b0 Compare December 2, 2022 21:07
@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

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

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%) ⬆️
src/core/server_messages.rs 68.57% <100.00%> (+0.92%) ⬆️

... 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.

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

@piegamesde
Copy link
Member

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

@piegamesde
Copy link
Member

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
Copy link
Contributor Author

wuan commented Dec 2, 2022

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
Copy link
Member

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 wuan force-pushed the send_client_version branch 2 times, most recently from 51401dc to 8dbbc13 Compare December 3, 2022 01:21
@wuan wuan force-pushed the send_client_version branch 2 times, most recently from 459ba64 to 0c54e2d Compare January 4, 2023 16:41
@wuan
Copy link
Contributor Author

wuan commented Jan 4, 2023

The solution is now hopefully clean enough.

@piegamesde
Copy link
Member

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
Copy link
Contributor Author

wuan commented Mar 15, 2023

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
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants