PrivateInterest, PrivateAreaContext + relative encodings to Area #26
No reviewers
Labels
No labels
breaking
bug
contribution welcome
duplicate
enhancement
good first issue
help wanted
invalid
question
upstream
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
worm-blossom/willow_rs!26
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "private-area-context"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implements
PrivateInterestandPrivateAreaContextand implementsRelativeEncodable<PrivateAreaContext>forArea. Includes fuzz tests, and a great newsubspace_includes_subspacefunction.Wrought.
@ -271,1 +276,3 @@self.0.can_be_encoded_relative_to(rel.into())self.0.can_be_encoded_relative_to(<&willow_data_model::groupings::Area<4096,Sorry, missed this one in earlier reviews: please use the proper constants here and elsewhere (just do a search for 4096 in
willow25).@ -122,1 +124,4 @@}/// Returns `true` if the outer `Option<S>` includes the inner `Option<S>`, where `None` denotes interest in all subspaces of the namespace.fn subspace_includes_subspace<S: PartialEq>(outer: Option<&S>, inner: Option<&S>) -> bool {Huh, doesn't this need to be
pub(crate) fn bla?I remember this function being quite handy outside of the crate, but happy to scope it to the crate if you disagree.
Right now it is private, or am I missing something?
@ -0,0 +86,4 @@/// Returns `true` is this [`PrivateInterest`] is [comparable](https://willowprotocol.org/specs/pio/index.html#pi_comparable) to `other`.pub fn is_comparable(&self, other: &Self) -> bool {self.is_more_specific(other) || other.is_more_specific(other)Typo, second one should not compare
othertoother.The most clearly readable version is
self.is_more_specific(other) || self.is_less_specific(other)in my opinion.@ -0,0 +90,4 @@}/// Returns true if `self` and `other` are [awkward](https://willowprotocol.org/specs/pio/index.html#pi_awkward), meaning they are neither [comparable](https://willowprotocol.org/specs/pio/index.html#pi_comparable) nor [disjoint](https://willowprotocol.org/specs/pio/index.html#pi_disjoint).pub fn awkward(&self, other: &Self) -> bool {I prefer
are_awkward, to indicate that this is a predicate (compareis_comparable,is_more_specific, etc). Or maybeis_awkward? Or possibly evenis_awkward_with? All of these read better than justawkwardin actual code that calls this, in any case.@ -0,0 +98,4 @@impl<const MCL: usize, const MCC: usize, const MPL: usize, N: PartialEq, S: PartialEq + Clone>PrivateInterest<MCL, MCC, MPL, N, S>{/// Returns `true` if the given [`Entry`] is included by `self`.The word "included" should be a link to
https://willowprotocol.org/specs/pio/index.html#pi_include_entry@ -0,0 +112,4 @@impl<const MCL: usize, const MCC: usize, const MPL: usize, N, S: PartialEq + Clone>PrivateInterest<MCL, MCC, MPL, N, S>{/// Returns `true` if the given [`Area`] in included by `self`.The word "included" should be a link to
https://willowprotocol.org/specs/pio/index.html#pi_include_area@ -0,0 +114,4 @@{/// Returns `true` if the given [`Area`] in included by `self`.pub fn includes_area(&self, area: &Area<MCL, MCC, MPL, S>) -> bool {//self.subspace_id.includes_area_subspace(area.subspace())delete the comment
@ -0,0 +119,4 @@&& self.path.is_prefix_of(area.path())}/// Returns `true` if the given [`Area`] in related to `self`.Comment should define "related" (this isn't in the spec, I think).
@ -0,0 +143,4 @@impl<const MCL: usize, const MCC: usize, const MPL: usize, N: Clone, S: Clone>PrivateInterest<MCL, MCC, MPL, N, S>{/// Clones self, but replaces the subspace id with `any`.Noneinstead ofany.(or make
anylink to the spec, but I prefer the self-contained rust-only version of the doc comment)@ -0,0 +162,4 @@N: Arbitrary<'a>,S: Arbitrary<'a>,{fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result<Self> {Am I missing a reason why this cannot simply be derived?
@ -0,0 +188,4 @@pub struct PrivateAreaContext<const MCL: usize, const MCC: usize, const MPL: usize, N, S> {/// The [`PrivateInterest`] to be kept private.private: PrivateInterest<MCL, MCC, MPL, N, S>,spurious newline
@ -0,0 +189,4 @@/// The [`PrivateInterest`] to be kept private.private: PrivateInterest<MCL, MCC, MPL, N, S>,/// The almost containing Area relative to which we encode."almost containing" should be a link to the spec, "Area" be a link to the struct.
Also "almost-containing"?
@ -0,0 +197,4 @@#[derive(Debug, Clone, Copy)]pub struct AreaNotAlmostIncludedError;impl core::fmt::Display for AreaNotAlmostIncludedError {Import the trait instead of using fully qualified names here.
🐭 Okay, sorry, I was imprecise here. Import everything and do not use fully qualified names anywhere =S
@ -0,0 +199,4 @@impl core::fmt::Display for AreaNotAlmostIncludedError {fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {core::fmt::Display::fmt("ComponentsNotRelatedError", f)Slightly misleading fmt implementation =P
@ -0,0 +208,4 @@impl<const MCL: usize, const MCC: usize, const MPL: usize, N, S>PrivateAreaContext<MCL, MCC, MPL, N, S>{/// Returns a new [`PrivateInterest`] with the given `private` and `rel` attributes. Will fail if the given [`PrivateInterest`]'s path is not a prefix of `rel`'s path.Copy-pasted doc comment =(
@ -0,0 +253,4 @@N: Arbitrary<'a>,S: Arbitrary<'a>,{fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result<Self> {Can this be derived?
@ -0,0 +234,4 @@impl<const MCL: usize, const MCC: usize, const MPL: usize, N, S: PartialEq>PrivateAreaContext<MCL, MCC, MPL, N, S>{/// Returns whether an [`Area`] _almost includes_ another area, that is, if the other [`Area`] would be included by this if [`Area] if it had the same [`SubspaceId`].This sentence no correct grammar.
🐭
Returns whether an [
Area] almost includes another area, that is if the other [Area] would be included by this **if [Area] if it had** the same [SubspaceId`].@ -0,0 +348,4 @@}(Some(rel_end), Some(val_end)) => {let end_from_start = *val_end - *rel.rel().times().start() <= *rel_end - *val_end;// println!("end_from_start: {:?}", end_from_start);remove comment
@ -0,0 +362,4 @@}};consumer.consume(either::Either::Left(header)).await?;Import
Leftinstead of using fully qualified paths.@ -0,0 +376,4 @@_ => {}}/*Remove comment.
@ -0,0 +390,4 @@cu64_encode(end_diff.into(), 2, consumer).await?;}// We can use new_unchecked because we know the paths from the context prefix the area's path.Add a
debug_assert!for that, please. Theunsafefeels a lot less scary then, given our thorough fuzzing.@ -0,0 +397,4 @@rel.rel().path().clone(),);self.path()Unsafe blocks should be of minimal size; move the relative encoding out of the unsafe block.
But then
private_path_contextis out of scope.@ -0,0 +407,4 @@/// Returns `false` when `self` is not [almost included by](https://willowprotocol.org/specs/encodings/index.html#almost_include) the relative [`PrivateAreaContext`].fn can_be_encoded_relative_to(&self, rel: &PrivateAreaContext<MCL, MCC, MPL, N, S>) -> bool {!(!rel.almost_includes_area(self) || !rel.private.almost_includes_area(self))!(!a || !b)is the same asa && b@ -0,0 +494,4 @@Some(end)};// We can use new_unchecked because we know the paths from the context prefix the area's path.debug assertion here again
@ -0,0 +500,4 @@rel.private().path().clone(),rel.rel().path().clone(),);Close the
unsafeblock here (and open a new, small unsafe block later).But then
private_path_contextis no longer in scope when it is needed later.@ -0,0 +509,4 @@return Err(DecodeError::Other(Blame::TheirFault));}// We can do this we just rejected any case where time_start is strictly greater than end.let times = TimeRange::new_closed_unchecked(time_start.into(), end.into());debug assertion =)
(yes, this is overkill, but
unsafeis really scary okay?)Is it so scary that it's going to somehow make it past the same case returning two lines above?
Yeah, fair.
@ -46,2 +46,4 @@pub use super::test_parameters::*;}/// Checks if a bit of a given byte is flagged at a particular index (with 0 being the most significant bit)Needs a dot at the end =S
🐭
@ -0,0 +122,4 @@&& self.path.is_prefix_of(area.path())}/// Returns `true` if the given [`Area`] in related to `self`, that is:🐭 Sorry, had a typo there.
in [included]->is [included].