Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

Allow ll-specific multihash #1

Merged
merged 5 commits into from
Feb 16, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
module github.com/ipfs/go-verifcid

go 1.15

require (
github.com/ipfs/go-cid v0.0.1
github.com/multiformats/go-multihash v0.0.1
Expand Down
6 changes: 6 additions & 0 deletions validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ func IsGoodHash(code uint64) bool {
return true
}
}
// XXX: This has to be on par with what the LazyLedger IPLD plugin registers as a multihash
// namely, Sha256Namespace8Flagged. We simply repeat the constant here instead of
// importing the corresponding package from lazyledger-core as this would be overkill.
if code == 0x7701 {
return true
}
Copy link
Member Author

Choose a reason for hiding this comment

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

An alternative would be to import the constant from where it is defined, currently in lazyledger-core:
https://github.com/lazyledger/lazyledger-core/blob/755a7175168e57e9f1996341e533683eace5c5d3/p2p/ipld/plugin/nodes/nodes.go#L27-L29

For now it seems more reasonable to repeat this here instead of introducing a massive dependency and assert that this checks out by the consumer of verifcid:
https://github.com/lazyledger/lazyledger-core/blob/7b9550659e2bd957b4042880b8339d9e1d84c9c0/p2p/ipld/plugin/nodes/nodes_test.go#L26-L30

Copy link
Member

Choose a reason for hiding this comment

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

Another option is to extract "common" dependency. But creating a module just for single const seems like overengineering.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thought about this too. But I agree, it would be overkill for now.

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to make this use of a magic number a bit more...documented (can't think of a better word)? I.e. this file documents what's happening, but if you go modify the magic number the other file you have no idea this one exists. Even putting it in the PR's description instead of a comment would go a long way.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I'll put it in the PR description.

BTW, there is a test in the consuming code that would fail if the constant was modified without updating it here accordingly: https://github.com/lazyledger/lazyledger-core/blob/3308e7636503ded830e476cd79ec4f9a5cf16de0/p2p/ipld/plugin/nodes/nodes_test.go#L26-L30


return false
liamsi marked this conversation as resolved.
Show resolved Hide resolved
}
Expand Down