Skip to content

feat(iroh-base)!: reduce external types in the iroh-base API for keys#3529

Merged
Frando merged 8 commits intomainfrom
feat-base-external-deps
Oct 14, 2025
Merged

feat(iroh-base)!: reduce external types in the iroh-base API for keys#3529
Frando merged 8 commits intomainfrom
feat-base-external-deps

Conversation

@dignifiedquire
Copy link
Copy Markdown
Contributor

@dignifiedquire dignifiedquire commented Oct 13, 2025

Description

In preparation of stabilizing our APIs, this removes ed25519_dalek types from the public API of iroh-base, except for a few conversions, which are doc(hidden) to clearly mark them as not stable.

Ref #3177

Breaking Changes

  • added iroh_base::Signature which replaces ed25519_dalek::Signature in the public API of iroh_base
  • iroh_base error types have changed
  • removed Into and From conversions for PublicKey - ed25519_dalek::VerifyingKey
  • removed Into and From conversions for SecretKey - ed25519_dalek::SigningKey

@dignifiedquire dignifiedquire self-assigned this Oct 13, 2025
@dignifiedquire dignifiedquire added this to the v0.94 milestone Oct 13, 2025
@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 13, 2025

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

Last updated: 2025-10-14T10:20:29Z

@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 13, 2025

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: 6231a68

@Frando
Copy link
Copy Markdown
Member

Frando commented Oct 13, 2025

Not sure about #[doc(hidden)] on From impls. It is quite error-prone: Users may write key.into() just because you're used to it and - it works. But if we deem these impls fine to break in semver-compat releases, then people run into it nevertheless. So I'd suggest to remove the From impls and have private into_verifying_key etc. fns. And users would then have to go over the to_bytes/from_bytes route, right? Which is what we'd recommend anyway if we mark the From impls doc_hidden.

Edit: And if we need the conversions in downstream crates, maybe make them pub fns with doc_hidden and a doc comment that explains that these may break in semver-compat releases?

@n0bot n0bot bot added this to iroh Oct 13, 2025
@github-project-automation github-project-automation bot moved this to 🏗 In progress in iroh Oct 13, 2025
@dignifiedquire
Copy link
Copy Markdown
Contributor Author

Not sure about #[doc(hidden)] on From impls. It is quite error-prone: Users may write key.into() just because you're used to it and - it works. But if we deem these impls fine to break in semver-compat releases, then people run into it nevertheless. So I'd suggest to remove the From impls and have private into_verifying_key etc. fns. And users would then have to go over the to_bytes/from_bytes route, right? Which is what we'd recommend anyway if we mark the From impls doc_hidden.

Edit: And if we need the conversions in downstream crates, maybe make them pub fns with doc_hidden and a doc comment that explains that these may break in semver-compat releases?

yeah we can do that as alternative

@dignifiedquire
Copy link
Copy Markdown
Contributor Author

@Frando changed based on your suggestions

let peer_id = VerifyingKey::from_public_key_der(&certs[0])
.map_err(|_| RemoteNodeIdSnafu.build())?
.into();
let peer_id = NodeId::from_bytes(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will be validating the key twice, maybe add NodeId::from_public_key_der(&[u8]) -> Result<Self>? But can also be a separate PR, I think this is not changed from before.

We also really should cache the remote's node id on the Connection struct I think, it seems expensive to do this whenever it's accessed. But also separate PR ofc.

iroh/src/key.rs Outdated

pub(super) fn public_ed_box(key: &ed25519_dalek::VerifyingKey) -> crypto_box::PublicKey {
pub(super) fn public_ed_box(key: &PublicKey) -> crypto_box::PublicKey {
let key = ed25519_dalek::VerifyingKey::from_bytes(key.as_bytes()).expect("valid key");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

se could use key.as_verifying_key() here if we keep it pub. I think the only other usage atm is in iroh/src/tls/verifier.rs. So Either let's use as_verifying_key here and there, or make it not pub and use from_bytes/as_bytes in both places. But I think using the pub+doc_hidden fn here is fine.

Copy link
Copy Markdown
Member

@Frando Frando left a comment

Choose a reason for hiding this comment

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

nice! some comments but no blockers.

@dignifiedquire dignifiedquire force-pushed the feat-base-external-deps branch from 435e474 to 8a0c16c Compare October 14, 2025 08:54
@dignifiedquire dignifiedquire changed the title feat(iroh-base)!: reduce external types in the iroh-base API feat(iroh-base)!: reduce external types in the iroh-base API for keys Oct 14, 2025
@dignifiedquire dignifiedquire added this pull request to the merge queue Oct 14, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 14, 2025
@dignifiedquire dignifiedquire added this pull request to the merge queue Oct 14, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 14, 2025
@dignifiedquire dignifiedquire added this pull request to the merge queue Oct 14, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 14, 2025
@dignifiedquire dignifiedquire added this pull request to the merge queue Oct 14, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 14, 2025
@Frando Frando added this pull request to the merge queue Oct 14, 2025
Merged via the queue into main with commit b45ae27 Oct 14, 2025
28 of 29 checks passed
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in iroh Oct 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

2 participants