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

initial pass converting to string-based multihashes #83

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Aug 29, 2018

TODO:

  • Consider Nil versus Zero.
  • Performance (especially better decoding).
  • Some nice decode method for extracting information from a validated multihash. Need to discuss how this would look.

Updates to reverse-dependencies:

Note: there are a bunch of FIXME(steb) comments in these related to the third
todo. Currently, I'm just using mh.Decode(hash.Bytes()) but that kind of sucks.

@Stebalien
Copy link
Member Author

Ignore the broken CI. That's happening because we can't build GX with this version of go-multihash because it breaks go-multiaddr.

@kevina
Copy link
Contributor

kevina commented Aug 30, 2018

Okay. When I initially did the conversion I tried to do a more complete conversion. If we are okay with leaving DecodedMultihash alone and leaving the functions that work on raw bytes alone I am okay with this. I would suggest we add a SumToBytes() function as that adds no overhead and if there are any external uses that depend on a Multihash being a byte they can likely get by using the combination of the Decode() Encode() and SumToBytes() functions.

I created a second p.r. that continues this one: #84.

@kevina kevina added the status/deferred Conscious decision to pause or backlog label Aug 30, 2018
@kevina
Copy link
Contributor

kevina commented Aug 30, 2018

Performance (especially better decoding).

My FromBinary() takes care of this in #84. I am okay with adding a few more checks but I want this to be fast as so that cid.Hash() is also fast as we can no longer directly cast to a Multihash type.

Some nice decode method for extracting information from a validated multihash.

I think we should keep this simple and just have a Parts() method as I did in #82 and carried over in #84.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/deferred Conscious decision to pause or backlog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants