-
Notifications
You must be signed in to change notification settings - Fork 93
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
I have cutted your base58 out! #142
Comments
Hi, I like the idea of cutting out the base58 implementation into its own separate package. I had a quick look at the code. I guess you didn't add the checksum functions as they were not strictly relevant to the base58 encoding and ripple/flickr? You do some property testing which is good as it guarantees internal consistency (decode . encode = id) but I'd also add some unit tests against known correct vectors to be sure that the library is not only internally consistent, but does the correct thing :) If you want to provide a patch for Haskoin, that would be cool. As Base58.hs doesn't do base 58 really anymore, I was thinking of renaming it to Address.hs and link it properly to your new library. I would like to keep all the tests which are currently in Haskoin however just to avoid any regressions. If you want to host the library in the Haskoin github page, it's all up to you. If you want to do that, I'll give you read/write access to the base58 package so you can continue maintaining it. Let me know. Cheers, |
You absolutely right about tests. Is there any golden files from What do you think about adding If yes, then If no, then where |
I've been thinking about this for a while, and my opinion is that base58check is very specific to Bitcoin. It is only relevant for Bitcoin addresses and for a few other things like Bitcoin WIF encodings. I don't think it makes a lot of sense to include a very bitcoin-specific function into a generic base58 bytestring library. And as you pointed out, we would be adding an unnecessary dependency on the SHA256. My suggestion would be to keep base58check functions in the Haskoin library together with the Address type, as they both work together. It makes sense to have them in the same place. |
Huh, I just did this too. Maybe we can just work together? Also, I'm using functionality similar to base58check even though I'm not using bitcoin, so maybe it's worth including base58check as well? |
Using the first 4 bytes of SHA256 as an error detection mechanism works, but it's not the smartest solution. There are much better ways of using codes to detect errors and even correct them. My opinion is that you should use another error detecting/correcting mechanism unless you really need backwards compatibility with bitcoin for some reason. I still believe that this somewhat weird checksum computation (base58check) should remain in bitcoin libraries and not in a generic base58 bytestring library. |
Ok, that seems reasonable to me. Do you have suggestions for a better error detection mechanism? |
Base58check is a particular case of a checksum which is a very simple error-detecting code. It is fast and simple, that's it. Whenever you need a checksum, just use it. I do not think it worthwhile exposing this functionality as a general thing. If you are interested in better ways to detect/correct errors, you should look in Coding Theory (http://en.wikipedia.org/wiki/Error_detection_and_correction). There are loots of schemes and I am sure you will find something satisfying your needs. |
@jprider63 You are welcome to join. I have added some benchmarks https://bitbucket.org/s9gf4ult/base58-bytestring/src/638c1619bca7749f4a2a20079abf062533df8c10/benchmark-results/?at=master and performance is not very good for now. So we still have something to improve in Maybe it is meaningful to generalize things with data BChecker =
BChecker
{ bCheck :: ByteString -> Bool
, bEncode :: ByteString -> ByteString
}
base58check :: BChecker
base58check =
BChecker
{ bCheck = ...
, bEncode = ...
} and implement several ways of checksuming including |
Ok, cool. Do you want to add me? I'm I'm starting to agree that the checksum functionality shouldn't be included in this package, but maybe another package dedicated to various checksums. |
Added. But when me/we prepare patch for haskoin this repo will likely be moved to |
Hello, I have cutted base58 implementation to separate package
https://bitbucket.org/s9gf4ult/base58-bytestring
My question is: do you need a patch to this package adding dependency from
base58-bytestring
and removing it's implementation? Or you guys maybe want to addbase58-bytestring
tohaskoin
team on github?The text was updated successfully, but these errors were encountered: