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: common base library #1780

Merged
merged 13 commits into from
Nov 8, 2023
Merged

refactor: common base library #1780

merged 13 commits into from
Nov 8, 2023

Conversation

Frando
Copy link
Member

@Frando Frando commented Nov 8, 2023

Description

This adds a new crate iroh_base. The following types were moved into the new crate:

  • Hash, HashAndFormat and BlobFormat from iroh_bytes::util
  • base32 encoding/decoding util functions. They lived at various places before.
  • RpcError and RpcResult from iroh_bytes::util

Notes & open questions

  • Naming: Shall it be iroh_common or iroh_base? I do not mind. If anyhow has a preference, please raise. I can change easily.
  • Anything else that should be moved over? Anything that I moved that should not be moved?

Change checklist

  • Self-review.
  • Documentation updates if relevant.
  • Tests if relevant.

@dignifiedquire
Copy link
Contributor

please make sure to reexport the types from iroh/src/lib.rs so only iroh needs to be a dependency

@Frando
Copy link
Member Author

Frando commented Nov 8, 2023

please make sure to reexport the types from iroh/src/lib.rs so only iroh needs to be a dependency

Added a pub use iroh_base as base. Or should we export the individual types directly?

@dignifiedquire
Copy link
Contributor

Added a pub use iroh_base as base. Or should we export the individual types directly?

I think individually makes probably more sense, to avoid moving types in there/out there a breaking change down the line

@dignifiedquire
Copy link
Contributor

I am generally fine with the name, I don't love it, but I don't have any alternatives 😅

@Frando
Copy link
Member Author

Frando commented Nov 8, 2023

I am generally fine with the name, I don't love it, but I don't have any alternatives 😅

Well, iroh-common was the most discussed contender so far. I don't really have a preference between the two I think.

@Frando
Copy link
Member Author

Frando commented Nov 8, 2023

Added a pub use iroh_base as base. Or should we export the individual types directly?

I think individually makes probably more sense, to avoid moving types in there/out there a breaking change down the line

I realized that the iroh_base::hash::* types are already reexported in iroh_bytes, which is reexported from iroh, so they are available as iroh::bytes::{Hash, HashAndFormat, BlobFormat}, which is fine I'd say?

The types from iroh_base::rpc are re-exported as iroh::rpc_protocol::{RpcResult, RpcError}, which is also fine.

Leaves the base32 which I reexported from iroh directly now.

@Frando Frando changed the title (WIP) refactor: common base library refactor: common base library Nov 8, 2023
@Frando
Copy link
Member Author

Frando commented Nov 8, 2023

iroh_base currently has a dependency on bao_tree, but only for the Hash type, which is a reexport from iroh_blake3. Should we add a dep to iroh_blake3 instead? Downside is that the versions would have to be kept in sync vigilantly.

@dignifiedquire
Copy link
Contributor

not sure, @rklaehn thoughts?

@Frando
Copy link
Member Author

Frando commented Nov 8, 2023

iroh_sync now fails to compile because rustc claims that bytes::Bytes doesn't implement serde::Serialize and Deserialize. I have no idea why that is the case. The serde feature is enabled for bytes in iroh-sync/Cargo.toml.

Anyone has an idea!?

@@ -31,7 +31,7 @@ rand_core = "0.6.4"
serde = { version = "1.0.164", features = ["derive"] }
strum = { version = "0.25", features = ["derive"] }
url = "2.4"
bytes = { version = "1.4", feature = ["serde"] }
bytes = { version = "1.4", features = ["serde"] }
Copy link
Member Author

Choose a reason for hiding this comment

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

😆

@Frando Frando added this pull request to the merge queue Nov 8, 2023
Merged via the queue into main with commit de58d71 Nov 8, 2023
15 checks passed
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.

None yet

3 participants