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

Add nameplates fn #184

Merged
merged 1 commit into from
Mar 8, 2023
Merged

Conversation

JustusFT
Copy link
Contributor

Ref: #183

Copy link
Member

@piegamesde piegamesde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking okay so far. One thing I'm unsure about is any public API: as proposed here, you'd need access to the RendezvousServer in order to make the call. As far as I can tell, it is only exposed when using Wormhole::connect_custom.

I don't know what the intended use cases are*, so maybe this is all the necessary API.

* autocompletion is out

src/core/rendezvous.rs Outdated Show resolved Hide resolved
src/core/rendezvous.rs Outdated Show resolved Hide resolved
@JustusFT
Copy link
Contributor Author

JustusFT commented Mar 1, 2023

I updated the linked issue with more information about the use case.
Currently I want the application to exit if the receiver put in a code with an unclaimed nameplate.
Would it be better to include the nameplate validation inside connect_with_code so we don't need to call connect_custom? This would be similar to how wormhole-william does it

@piegamesde
Copy link
Member

That would be okay to do, however it should be opt-in, as in most scenarios you actually don't want such a check.

@JustusFT JustusFT requested a review from piegamesde March 3, 2023 05:37
src/core.rs Outdated Show resolved Hide resolved
src/core/rendezvous.rs Outdated Show resolved Hide resolved
src/core/test.rs Outdated Show resolved Hide resolved
@JustusFT JustusFT requested a review from piegamesde March 7, 2023 03:23
Copy link
Member

@piegamesde piegamesde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the commit history pretty (squashing everything together is okay too) and then we are good to go.

Edit: CI got fixed, please also rebase for that

- Adds `list_nameplates` which returns a list of currently claimed
  nameplates
- Adds a new argument (`expect_claimed_nameplate`) to
  `connect_with_code`. When true, the function will return an error
  if the nameplate is not claimed
@JustusFT JustusFT force-pushed the add-nameplates-fn branch from 7c13ce8 to 3daaded Compare March 8, 2023 01:08
@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Patch coverage: 85.71% and project coverage change: +0.92 🎉

Comparison is base (32369ac) 43.15% compared to head (3daaded) 44.08%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #184      +/-   ##
==========================================
+ Coverage   43.15%   44.08%   +0.92%     
==========================================
  Files          18       18              
  Lines        2542     2552      +10     
==========================================
+ Hits         1097     1125      +28     
+ Misses       1445     1427      -18     
Impacted Files Coverage Δ
cli/src/main.rs 0.00% <0.00%> (ø)
src/core/rendezvous.rs 74.52% <87.50%> (+0.50%) ⬆️
src/core.rs 78.52% <100.00%> (+3.21%) ⬆️
src/transit/transport.rs 86.59% <0.00%> (+2.06%) ⬆️
src/transit.rs 76.96% <0.00%> (+2.45%) ⬆️
src/core/server_messages.rs 67.64% <0.00%> (+8.82%) ⬆️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JustusFT
Copy link
Contributor Author

JustusFT commented Mar 8, 2023

@piegamesde I squashed and rebased 👍

@piegamesde piegamesde merged commit 46eceb0 into magic-wormhole:master Mar 8, 2023
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.

2 participants