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

refactor(iroh-willow): use willow-rs #2616

Merged
merged 30 commits into from
Aug 13, 2024
Merged

refactor(iroh-willow): use willow-rs #2616

merged 30 commits into from
Aug 13, 2024

Conversation

Frando
Copy link
Member

@Frando Frando commented Aug 12, 2024

Description

Adopts willow-rs in iroh-willow:

  • Use the types from willow-data-model. Our implementations are replaced by type aliases onto the willow-data-model types with the generics filled in.
  • Use meadowcap. Our implementations are replaced by type aliases onto the meadowcap types with the generics filled in.

The PR is large and not well readable because I adapted the module structure to be closer to willow-rs as well in the same go (a bit non-ideal, but I was already having to go over most imports once, so did it in one go). I will merge this as soon as all tests are green, and then refine in the main willow PR (#2231).

Breaking Changes

Many public types from iroh-willow are now type aliases instead of structs.

Notes & open questions

Based on #2231 and currently uses willow-rs git dependency with two PRs merged in: earthstar-project/willow-rs#45 and earthstar-project/willow-rs#46

Generics

Type aliases work well in many regards, but they don't really hide all the generics well, they appear in compiler error messages and rust-analyzer doc hovers etc and make all that quite unreadable. We talked about this a bit, and will have to improve the story at least for the public client API, and potentially also for ourselves working in the internals.

  • We can newtype wrap everything, then the generics are gone. But the cost is that we'd have to reimplement and forward all methods.
  • Maybe there's ways to reduce the generics in iroh-willow itself, by using a zerosized struct that implements a trait with associated types instead.

Encodings

This PR does not yet include spec-compliant encodings for WGPS messages, it still uses postcard with serde as an interim solution. To make this work with the willow-rs types which don't implement serde, I added wrapper types that takes the willow-encoding and maps them into serde. The proper encodings will still have to be written, same for the integration of ufotofu encoding with our in-memory pipe.

Key types

We currently have UserId and UserPublicKey types (and same for namespace): Id is a wrapper around [u8; 32], wheres PublicKey is the ed25519_dalek::VerifyingKey. Converting from Id to PublicKey is fallible (because not all 32 bytes are valid ed pubkeys) and non-free (it involves numeric operations). Before this PR, we used PublicKey in meadowcap and Id in data-model. This is no longer possible because the generics have to map on the same time. We now use Id everywhere and convert to PublicKey on demand.

This is non-ideal because the conversion will happen far more often than needed. However willow currently assumes a non-fallible successor() function when a UserId (UserPublicKey) is used as a subspace id.

What we likely want is an in-memory LRU cache for the converted versions. Or a type where the numeric pubkey is optional, but that would either need mutable access to the key for operations, or interior mutability.

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.

Copy link

github-actions bot commented Aug 12, 2024

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2616/docs/iroh/

Last updated: 2024-08-13T14:01:09Z

@Frando Frando force-pushed the willow-use-willow-rs branch 3 times, most recently from de8aa4a to 88bcdce Compare August 12, 2024 22:53
@Frando Frando force-pushed the willow-use-willow-rs branch from 88bcdce to f43e0b4 Compare August 12, 2024 22:53
@Frando Frando changed the title [wip] refactor(iroh-willow): use willow-rs refactor(iroh-willow): use willow-rs Aug 13, 2024
@Frando Frando marked this pull request as ready for review August 13, 2024 14:00
@Frando Frando merged commit 723f21c into willow Aug 13, 2024
27 checks passed
@Frando Frando mentioned this pull request Aug 13, 2024
51 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants