refactor: changes done while porting iroh-willow #45

Frando · 2024-08-12T14:57:03Z

This PR contains changes I made to willow-rs while porting iroh-willow to use willow-rs.

The changes are mostly small and not all related to each other. They were mostly done by doing jump to definition from iroh-willow when touching willow-rs parts that didn't fit in well yet.

I will add review comments in the diff to explain the motivation.

My idea was to submit this here, let you review it, and if anything warrants further discussion, I will separate it out into its own PR, to keep only uncontroversial things in here that can easily be merged.

If you prefer to split this up right away, let me know.

Frando · 2024-08-12T14:58:03Z

data-model/src/entry.rs

@@ -13,7 +13,7 @@ pub type Timestamp = u64;

 /// The metadata associated with each Payload.
 /// [Definition](https://willowprotocol.org/specs/data-model/index.html#Entry).
-#[derive(Debug, PartialEq, Eq, Clone)]
+#[derive(Debug, PartialEq, Eq, Clone, Ord, PartialOrd)]


I think the derived Ord should work as expected, as the derive will sort fields in definition order, which is the same order I deemed logical in my impl before.

I mostly need this to stick entries into a BTreeSet or such, to have predictable ordering.

I feel like the impl of PartialOrd and Ord should have a doc comment stating that the order is not according to the spec but simply an arbitrary choice.

Added a comment that details the sort order, and swapped the field ordering on the struct so that payload_digest comes before payload_length, so that the derived Ord matches the "is newer" relation from the willow spec (for entries where namespace, subspace and path are equal).

Frando · 2024-08-12T14:59:52Z

data-model/src/grouping/area_of_interest.rs

@@ -16,11 +16,16 @@ pub struct AreaOfInterest<const MCL: usize, const MCC: usize, const MPL: usize,
 impl<const MCL: usize, const MCC: usize, const MPL: usize, S: SubspaceId>
    AreaOfInterest<MCL, MCC, MPL, S>
 {
+    /// Creates a new [`AreaOfInterest`].
+    pub fn new(area: Area<MCL, MCC, MPL, S>, max_count: u64, max_size: u64) -> Self {


It's just easier to type than initializing with let aoi = AreaOfInterest { area, max_count, max_size } and leads to less line breaks.

Frando · 2024-08-12T15:00:16Z

data-model/src/grouping/area_of_interest.rs

    /// Return the intersection of this [`AreaOfInterest`] with another.
    /// [Definition](https://willowprotocol.org/specs/grouping-entries/index.html#aoi_intersection).
    pub fn intersection(
        &self,
-        other: AreaOfInterest<MCL, MCC, MPL, S>,
+        other: &AreaOfInterest<MCL, MCC, MPL, S>,


Doesn't consume other so should take by ref to not need cloning at call site

Frando · 2024-08-12T15:00:26Z

data-model/src/grouping/range.rs

@@ -6,7 +6,7 @@ use arbitrary::{Arbitrary, Error as ArbitraryError};

 use crate::path::Path;

-#[derive(Debug, Hash, PartialEq, Eq)]
+#[derive(Debug, Hash, PartialEq, Eq, Clone, Copy)]


Those can easily be copy

Frando · 2024-08-12T15:00:55Z

data-model/src/grouping/range.rs

@@ -164,6 +152,11 @@ impl<T> Range<T>
 where
    T: Ord + Clone,
 {
+    /// Construct a range.
+    pub fn new(start: T, end: RangeEnd<T>) -> Self {


Again, a constructor for when you have a RangeEnd available (eg from another range) saves line breaks at callsite

Frando · 2024-08-12T15:01:16Z

meadowcap/src/communal_capability.rs

@@ -157,8 +157,8 @@ where
    /// The kind of access this capability grants.
    ///
    /// [Definition](https://willowprotocol.org/specs/meadowcap/index.html#communal_cap_mode)
-    pub fn access_mode(&self) -> &AccessMode {
-        &self.access_mode
+    pub fn access_mode(&self) -> AccessMode {


Copy types should be returned by value

Frando · 2024-08-12T15:01:50Z

meadowcap/src/mc_capability.rs

        namespace_key: NamespacePublicKey,
-        namespace_secret: NamespaceSecret,
+        namespace_secret: &NamespaceSecret,


No need for this to be async.

NamespaceSecret isn't consumed, so take by ref to not have to clone at call site

Frando · 2024-08-12T15:02:05Z

meadowcap/src/mc_capability.rs

@@ -251,7 +251,7 @@ where
    /// Return a new [`McAuthorisationToken`], or an error if the resulting signature was not for the capability's receiver.
    pub fn authorisation_token<UserSecret, PD>(
        &self,
-        entry: Entry<MCL, MCC, MPL, NamespacePublicKey, UserPublicKey, PD>,
+        entry: &Entry<MCL, MCC, MPL, NamespacePublicKey, UserPublicKey, PD>,


Entry isn't consumed so take ref to avoid clone at call site

Frando · 2024-08-12T15:02:22Z

meadowcap/src/mc_subspace_capability.rs

@@ -69,9 +69,9 @@ where
    UserSignature: Encodable + Clone,
 {
    /// Generate a new [`McSubspaceCapability`] for a given user, or return an error if the given namespace secret is incorrect.
-    pub fn new<const MCL: usize, const MCC: usize, const MPL: usize, NamespaceSecret>(
+    pub fn new<NamespaceSecret>(


the const generics weren't used

Frando · 2024-08-12T22:37:04Z

data-model/src/entry.rs

-    pub Entry<MCL, MCC, MPL, N, S, PD>,
-    pub AT,
+    Entry<MCL, MCC, MPL, N, S, PD>,
+    AT,


I think the fields should not be pub: If this type is intended to symbolize an entry+token pair where the authorisation was checked (which the new function indicates, and which is also how the term is used in the willow spec) then I think the fields should be private, otherwise AuthorisedEntry(some_entry, some_token) is a too-easy-to-get-wrong constructor for an unchecked AuthorisedEntry.

AljoschaMeyer · 2024-08-13T22:17:40Z

Thank you. I agree with ~~almost~~ all of these, ~~will add comments where I'm not 100% convinced~~.

…gth to newer relation from spec

Frando · 2024-08-19T07:18:48Z

@AljoschaMeyer wrote here:

Re Ord on Entry again: the sideloading protocol defines a total order on entries to use (entries "being prior" to others): https://willowprotocol.org/specs/sideloading/index.html#sideload_protocol

The implementation of Ord should implement that order, and refer to it in the doc comment. Sorry, I forgot about that order earlier.

Right! I changed towards a manual impl of Ord that uses the definition of ordering from the sideloading protocol.

sgwilym

Thank you for this roundup!

refactor: changes done while porting iroh-willow

d89cbf6

Frando commented Aug 12, 2024

View reviewed changes

fix: fuzz targets for recent change

75369f3

Frando marked this pull request as ready for review August 12, 2024 15:06

refactor: make fields of AuthorisedEntry private

254c153

Frando commented Aug 12, 2024

View reviewed changes

feat: derive Hash for Entry

65bfc1f

This was referenced Aug 13, 2024

refactor(iroh-willow): use willow-rs n0-computer/iroh#2616

Merged

[wip] feat: willow implementation n0-computer/iroh#2231

Draft

Frando added 3 commits August 14, 2024 14:15

fix: document entry sort order and order by payload digest before len…

ebb9efd

…gth to newer relation from spec

fix: properly implement Ord

100b4c0

chore: fmt

04eb287

sgwilym approved these changes Aug 19, 2024

View reviewed changes

sgwilym merged commit b0a5eeb into earthstar-project:main Aug 19, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: changes done while porting iroh-willow #45

refactor: changes done while porting iroh-willow #45

Frando commented Aug 12, 2024

Frando Aug 12, 2024

AljoschaMeyer Aug 13, 2024

Frando Aug 14, 2024

Frando Aug 12, 2024

Frando Aug 12, 2024

Frando Aug 12, 2024

Frando Aug 12, 2024

Frando Aug 12, 2024

Frando Aug 12, 2024

Frando Aug 12, 2024

Frando Aug 12, 2024

Frando Aug 12, 2024 •

edited

Loading

AljoschaMeyer commented Aug 13, 2024 •

edited

Loading

Frando commented Aug 19, 2024

sgwilym left a comment

refactor: changes done while porting iroh-willow #45

refactor: changes done while porting iroh-willow #45

Conversation

Frando commented Aug 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frando Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

AljoschaMeyer commented Aug 13, 2024 • edited Loading

Frando commented Aug 19, 2024

sgwilym left a comment

Choose a reason for hiding this comment

Frando Aug 12, 2024 •

edited

Loading

AljoschaMeyer commented Aug 13, 2024 •

edited

Loading