base58 ?

[JulienPalard]

I was browsing your code about multiaddr, and just FYI, if the base58 you implemented is the "bitcoin flavored" one, it looks like it won't passe the bitcon base58 test, but as you're not using it, and as I did not found any spec about using base58 in multiaddr, I don't know if that normal or not ^^

https://github.com/bitcoin/bitcoin/blob/master/src/test/data/base58_encode_decode.json

[amstocker]

@JulienPalard to be honest I'm not sure which alphabet the multiaddr spec uses but I'm assuming you're right in that it uses the bitcoin base58 alphabet. The one I was using was just one I found online.

[JulienPalard]

Where is the multiaddr part of the spec that speak about base58 ? Did not found it.

[amstocker]

@JulienPalard here is the spec which has relatively little info. I'm assuming the base58 util is based on something in go-ipfs but I haven't gone looking for it.

[JulienPalard]

OK that's why I have not found "base58" while searching for it ;-)

I think we should create a ticket asking them instead of assuming ?

[amstocker]

@JulienPalard yes that is probably a good idea :) I am just being lazy. I'll ask in IRC and make a ticket later.

[amstocker]

Ok @jbenet confirmed in IRC that IPFS uses the bitcoin alphabet

[JulienPalard]

if IPFS is using the bitcoin alphabet, I infer that it also should pass the bitcon unit tests, but your implementation don't, you should not close this issue.

[amstocker]

@JulienPalard I added a commit 7729dfe which changed the alphabet and checks the conversions that you posted, which now pass. Are there other unit tests?

[candeira]

I've looked at your b58 implementation, and since it will be used in other places outside multiaddr, maybe we should either:

• package it out into its own package
• use one of the already packaged ones

As to the implementation, our standard digest format, if we are going to follow hashlib.digest(), is a bytes object, not an int. Maybe our b58 encoder/decoder should take a bytestring as its unencoded input/output.

[amstocker]

@candeira I agree that it should be its own package. Is there a pre-existing package that you have used that is good? Maybe we should just make out own?

[candeira]

I've used two, none of them memorable. I have no preference except that it's not vendored in.