Skip to content
This repository has been archived by the owner on Dec 19, 2020. It is now read-only.

Things before upstreaming #5

Closed
vmx opened this issue Aug 3, 2020 · 5 comments
Closed

Things before upstreaming #5

vmx opened this issue Aug 3, 2020 · 5 comments

Comments

@vmx
Copy link
Member

vmx commented Aug 3, 2020

There are a few things I'd like to tackle before upstreaming. I put them into a single issue so that they can be discussed easily and we don't span this across zillions of issues or bloat PRs.

I split them into "ideas" which I think would make sense, but I'm, not sure about and "issues" which I think need to be fixed. Though they are all open for dicussions.

Ideas:

  • Re-export all generic_array::typenum:: as multihash::sizes:: or so
    Currently we only re-export some sized

Issues:

  • size() should return the actual size. For most of the digests it's the same as the digest size. But it's not the case for the Identity hash.
    • This is connected to digest() should return the slice of the actual digest. This is again an issue for the Identity hash, where it should be trimmed to size().
  • Implement TryFrom traits (and potentially remove the other functions from the traits (that's open for discussion):
    • For from_bytes()/to_bytes() in the MultihashDigest
    • For from_mh()/to_mh() in RawMultihash

I've crated a list of things a Multihash implementation should implement and I think all those will be addressed. If I miss anything that should be supported (and perhaps already is by this library), please let me know:

  • An instance of a Multihash can be created in several ways:

    • From an existing Multihash, represented as bytes.
      • Also needs to support Multihashes that don't have an implementation. Such a Multihash wouldn't be able to create new hashed, but only be able to return code, size and digest.
    • From an existing (non-Multihash) hash digest together with a Multihash Code. The size may either be given, calculated directly from the digest itself or implied (e.g. the type system).
    • From data that still needs to get hashed together with a Multihash Code. The size may either be given or implied (e.g. the type system).
  • The Multihash instance then provides access to the properties of a hash:

    • code: The code of the Multihash.
    • size: The size needs to be the actual size. It may differ from the default hash size due to two reasons. Multihash supports trunctated hashes and there is also hash functions with arbitrary sizes, like Blake3.
    • digest: The actual hash.
@vmx
Copy link
Member Author

vmx commented Aug 21, 2020

As much as I love the From/TryFrom trait. It just doesn't work well enough for from_bytes()/to_bytes() or from_mh()/to_mh() mostly due to rust-lang/rust#50133.

What is left is then getting size() work correctly and thinking about re-exporting all of generic_array::typenum:: .

@vmx
Copy link
Member Author

vmx commented Aug 26, 2020

I think everything I wanted before upstreaming is now there.

@vmx vmx closed this as completed Aug 26, 2020
@dvc94ch
Copy link
Member

dvc94ch commented Aug 26, 2020

Can we add back the code enum? I'd like for the ipld block api to follow what the codec does and take the multihash code, where it's compatible with the store multihash.
Also need to look into how converting Multihash to RawMultihash works again, it's pretty annoying to construct a cid, even though it's all hidden behind the block api now

@vmx
Copy link
Member Author

vmx commented Aug 26, 2020

Upstreaming will take a while (I also want to take a closer look at the rust-ipld code and the tiny-cid code first), so there's still plenty of time for adding things.

Can we add back the code enum?

Yes sure. The idea would be to just generate that from the multihash-derive, right?

@dvc94ch
Copy link
Member

dvc94ch commented Aug 26, 2020

Exactly

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants