search

amstocker / python-multiaddr

Python multiaddr implementation -- https://github.com/jbenet/multiaddr
1 stars 0 forks source link

base58 ? #1

Open JulienPalard opened 8 years ago

JulienPalard commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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

amstocker commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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

amstocker commented 8 years ago

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

JulienPalard commented 8 years ago

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 commented 8 years ago

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

candeira commented 8 years ago

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

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 commented 8 years ago

@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 commented 8 years ago

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