feat(ticket): Ensure a ticket always has at least one address #892
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This makes it impossible to have a ticket with an empty address list. It makes the fields of the ticket private and also makes creating the ticket fallible. Both are probably good things for the public API and the latter e.g. allows us to add further checks like ensure the hash really exists.
src/provider/mod.rs
Outdated
| } | ||
|
|
||
| impl Ticket { | ||
| fn new(hash: Hash, peer: PeerId, addrs: Vec<SocketAddr>, token: AuthToken) -> Result<Self> { | ||
| ensure!(!addrs.is_empty(), "Invalid address list in ticket"); |
There was a problem hiding this comment.
Maybe more sth like, missing addresses?
dignifiedquire
previously approved these changes
Mar 28, 2023
flub
commented
Mar 28, 2023
Comment on lines
+545
to
+568
| tokio::spawn(async move { | ||
| let (path, tmp_path) = if let Some(path) = path { | ||
| let absolute = path.canonicalize()?; | ||
| println!("Adding {} as {}...", path.display(), absolute.display()); | ||
| (absolute, None) | ||
| } else { | ||
| // Store STDIN content into a temporary file | ||
| let (file, path) = tempfile::NamedTempFile::new()?.into_parts(); | ||
| let mut file = tokio::fs::File::from_std(file); | ||
| let path_buf = path.to_path_buf(); | ||
| // Copy from stdin to the file, until EOF | ||
| tokio::io::copy(&mut tokio::io::stdin(), &mut file).await?; | ||
| println!("Adding from stdin..."); | ||
| // return the TempPath to keep it alive | ||
| (path_buf, Some(path)) | ||
| }; | ||
| // tell the provider to add the data | ||
| let stream = controller.server_streaming(ProvideRequest { path }).await?; | ||
| let (hash, entries) = aggregate_add_response(stream).await?; | ||
| print_add_response(hash, entries); | ||
| let ticket = provider.ticket(hash)?; | ||
| println!("All-in-one ticket: {ticket}"); | ||
| anyhow::Ok(tmp_path) | ||
| }) |
There was a problem hiding this comment.
this block is only indented, otherwise unchanged
This has the advantage that the provider itself can not construct the ticket directly and thus is forced to uphold the invariant that the address list must not be empty.
|
PTAL: I additionally moved the Ticket to it's own module. This ensures that the provider can not construct a ticket without upholding the invariant. Also the provider module is pretty huge by now, so splitting it up does no harm. |
dignifiedquire
approved these changes
Mar 29, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes it impossible to have a ticket with an empty address list.
It makes the fields of the ticket private and also makes creating the
ticket fallible. Both are probably good things for the public API and
the latter e.g. allows us to add further checks like ensure the hash
really exists.