-
Notifications
You must be signed in to change notification settings - Fork 49
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
Update to multihash 0.19 #140
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting this started!
The CI currently fails due to the code not being formatted correctly. Just run a cargo fmt
and you should be good.
Cargo.toml
Outdated
scale-codec = ["parity-scale-codec", "multihash/scale-codec"] | ||
serde-codec = ["alloc", "serde", "multihash/serde-codec", "serde_bytes"] | ||
|
||
[dependencies] | ||
multihash = { version = "0.18.0", default-features = false } | ||
multihash = { version = "0.19.0", default-features = false } | ||
multihash-codetable = { version = "0.1.0", default-features = false, features = ["digest", "sha2"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the upgrade to of Multihash, we should take the chance of reducing the dependency tree. From this PR, it looks like multihash-codetable
is only used in the tests/examples. Therefore I propose making it a dev dependency and not re-exporting it.
I know it's a breaking change, but the code table was only re-exported as the full library was. With separate crates we now have more flexibility here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to :)
Sorry about forgetting to fmt. Will move codetable to dev-depend and run fmt. |
Actually, |
Above passes |
I think having it optional is already great and good enough for this PR. I'm thinking about having a follow up PR which removes the |
Looking at why CI failed... |
Got it. I missed the warning due to the tests output. My bad, fixed this time around. multihash-derive isn't removed from dev-dependencies because it's still required by the MultihashDigest macro offered by codetable. |
Cargo.toml
Outdated
default = ["std", "multihash/default", "multihash-codetable/default"] | ||
std = ["multihash/std", "multihash-codetable/std", "unsigned-varint/std", "alloc", "multibase/std", "serde/std"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will add multihash-codetable
by default. So just remove them from here.
I needed to check that myself and figured out that it's the same for the other optional dependencies as well. I guess we should fix that properly somehow. But for now at least not adding multihash-codetable
is a good first step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yep. The solution is ?/
, which only sets it if present.
Doesn't correct it for the other optional crates.
Now uses |
The CI failure is not caused by your code, it's a regression in rust-multihash. I've opened multiformats/rust-multihash#336 and I'm having a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the new multihash v0.19.1 release the CI passes. Thanks for your contribution!
Unfortunately, I can't push to the branch but here is a patch to make Subject: [PATCH] Make `multihash-codetable` a dev-dependency
---
Index: Cargo.toml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/Cargo.toml b/Cargo.toml
--- a/Cargo.toml (revision 011f0eecc3a4e57818ed420009d1c8a13dec4421)
+++ b/Cargo.toml (revision 37a453d6d57a58886e3ab5bb0fb42c8a431eb501)
@@ -11,16 +11,15 @@
rust-version = "1.60"
[features]
-default = ["std", "multihash/default", "multihash-codetable?/default"]
-std = ["multihash/std", "multihash-codetable?/std", "unsigned-varint/std", "alloc", "multibase/std", "serde/std"]
+default = ["std", "multihash/default"]
+std = ["multihash/std", "unsigned-varint/std", "alloc", "multibase/std", "serde/std"]
alloc = ["multibase", "multihash/alloc", "core2/alloc", "serde/alloc"]
-arb = ["quickcheck", "rand", "multihash/arb", "multihash-codetable", "arbitrary"]
+arb = ["quickcheck", "rand", "multihash/arb", "arbitrary"]
scale-codec = ["parity-scale-codec", "multihash/scale-codec"]
serde-codec = ["alloc", "serde", "multihash/serde-codec", "serde_bytes"]
[dependencies]
multihash = { version = "0.19.0", default-features = false }
-multihash-codetable = { version = "0.1.0", default-features = false, features = ["digest", "sha2"], optional = true }
unsigned-varint = { version = "0.7.0", default-features = false }
multibase = { version = "0.9.1", optional = true, default-features = false }
@@ -36,3 +35,4 @@
[dev-dependencies]
multihash-derive = { version = "0.9.0", default-features = false }
serde_json = "1.0.59"
+multihash-codetable = { version = "0.1.0", features = ["digest", "sha2"] }
Index: src/arb.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/arb.rs b/src/arb.rs
--- a/src/arb.rs (revision 011f0eecc3a4e57818ed420009d1c8a13dec4421)
+++ b/src/arb.rs (revision 37a453d6d57a58886e3ab5bb0fb42c8a431eb501)
@@ -3,7 +3,6 @@
use std::convert::TryFrom;
use multihash::Multihash;
-use multihash_codetable::{Code, MultihashDigest};
use quickcheck::Gen;
use rand::{
distributions::{weighted::WeightedIndex, Distribution},
@@ -13,6 +12,7 @@
use arbitrary::{size_hint, Unstructured};
use rand::SeedableRng;
+use crate::cid::SHA2_256;
use crate::{CidGeneric, Version};
impl quickcheck::Arbitrary for Version {
@@ -25,12 +25,12 @@
impl<const S: usize> quickcheck::Arbitrary for CidGeneric<S> {
fn arbitrary(g: &mut Gen) -> Self {
if S >= 32 && Version::arbitrary(g) == Version::V0 {
- let data: Vec<u8> = Vec::arbitrary(g);
- let hash = Code::Sha2_256
- .digest(&data)
- .resize()
- .expect("digest too large");
- CidGeneric::new_v0(hash).expect("sha2_256 is a valid hash for cid v0")
+ let data = std::array::from_fn::<_, 32, _>(|_| u8::arbitrary(g));
+
+ CidGeneric::new_v0(
+ Multihash::wrap(SHA2_256, &data).expect("S is guaranteed to be > 32"),
+ )
+ .expect("sha2_256 is a valid hash for cid v0")
} else {
// In real world lower IPLD Codec codes more likely to happen, hence distribute them
// with bias towards smaller values.
@@ -58,7 +58,7 @@
impl<'a, const S: usize> arbitrary::Arbitrary<'a> for CidGeneric<S> {
fn arbitrary(u: &mut Unstructured<'a>) -> arbitrary::Result<Self> {
if S >= 32 && u.ratio(1, 10)? {
- let mh = Multihash::wrap(Code::Sha2_256.into(), u.bytes(32)?).unwrap();
+ let mh = Multihash::wrap(SHA256, u.bytes(32)?).unwrap();
return Ok(CidGeneric::new_v0(mh).expect("32 bytes is correct for v0"));
}
Index: src/cid.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/cid.rs b/src/cid.rs
--- a/src/cid.rs (revision 011f0eecc3a4e57818ed420009d1c8a13dec4421)
+++ b/src/cid.rs (revision 37a453d6d57a58886e3ab5bb0fb42c8a431eb501)
@@ -56,7 +56,7 @@
/// DAG-PB multicodec code
const DAG_PB: u64 = 0x70;
/// The SHA_256 multicodec code
-const SHA2_256: u64 = 0x12;
+pub(crate) const SHA2_256: u64 = 0x12;
/// Representation of a CID.
///
|
@thomaseizinger could you please open a new PR? The tracking issue is #141. |
Done: #144. |
Time for a new release? |
As it's a breaking release, I'm not in a rush, but it sounds like you are (and you did do the original PR pushing it forward after all :) Though it would be great to get #142 in, especially the |
I'm not in a rush, just trying to get the ecosystem's clock ticking at a healthy rate :) This causes a few duplicated dependencies for me IIRC, and when I noticed, it was brief enough to make the PR. Please, feel free to wait a couple weeks if desirable. Also, I did review making a PR to make serde's features only apply when serde was pulled in, yet it'd be breaking as right now, |
Thanks for having a look. I thought it would be easy, obviously it's not. |
It's easy if you're fine with serde being no longer by default, which I can't issue an opinion on. If you want to keep serde by default, simply independently add serde as a default feature. You did say this'd already be part of a breaking release, so we have leeway. |
Happy to handle any feedback.