-
Notifications
You must be signed in to change notification settings - Fork 0
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
Choosing between the 2 PRs implementing base32 support #60
Comments
My own opinion: Maybe it was unfair to just let DangerousFreedom's PR to just hang around for so long, but anyway, already when I saw it for the first time I was surprised about the length of the code - over 450 lines - and a quite high number of methods. I asked back then: Is really so much code needed to implement something so relatively simple as Base32? My suspicion: The code mostly is longer and more complex than it could be because it comes from a multi-codings library. jeffro256's "hand carved" code shows that it is possible with less complex and shorter code: His main .cpp file has less than 200 lines, half of the competition, and only a handful of methods. You may remember statements from me that ousted me as somebody with a deep distrust of complexity, seeing that as one of the major problems plaguing today's IT: I really can't do other than giving my vote to jeffro256's solution. |
I have just ran some perf tests and I also vote for @jeffro256 's implementation as it is shorter and 60% faster than mine. It also didnt fail when reading the serialized structs that I have in other unit_tests. So it seems to be working fine. |
This issue can probably be closed now, yeah? |
Right, of course. |
For Jamtis, or more exactly Jamtis style addresses, we need support for Base32 encoding and decoding which is not yet part of the Monero code base.
We currently thave the somewhat unfortunate situation that we don't have a single PR with code for that, but two. As we obviously need only a single Base32 implementation, seems to me we will have to "throw away" one of the two.
Quite a while ago already @DangerousFreedom1984 made PR #2. He took the code from this multi-codings library, extracted everything necessary for Base32 and turned that into a single .cpp file that you can see here. @vtnerd recently made a review of the code.
Also recently @jeffro256 made PR #6. In a first version that still can be seen through this link he proposed to make the same library that DangerousFreedom took as starting point a submodule and added code to configure an instance of a class from the library to support the special character set that Jamtis uses; the result was a dependency on a new library, but very little additional code, under 100 lines.
A few days ago he dropped that proposal however and wrote new code implementing exactly the functionality that is needed; the resulting single .cpp file is here. It currently waits for a review.
The unit tests that come with both PRs show quite convincingly IMHO that both solutions work i.e. encode and decode correctly.
What's the opinion of workgroup members, which PR would you prefer we pick, with which arguments?
The text was updated successfully, but these errors were encountered: