-
Notifications
You must be signed in to change notification settings - Fork 59
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
Add integer support #12
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some general comments to the code but could you explain a bit what your use case for this is? Are you trying to make this match some other implementation of base58 of integers?
there's a slight difference between
b58encode(struct.pack('>I', X))
and int_to_b58(X)
which I would like to avoid.
base58.py
Outdated
@@ -27,9 +27,20 @@ | |||
buffer = lambda s: s.buffer | |||
|
|||
|
|||
def int_to_b58(i): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to have these names symmetrical with the ones already existing, so something like b58encode_int
and b68decode_int
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I can revise that.
base58.py
Outdated
'''Encode an integer using Base58''' | ||
string = "" | ||
while i: | ||
idx = i % 58 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because internally the other encoders convert to a number before encoding to base58 this should look exactly the same as the L57-L60 in b58encode
Mostly its that I find myself encoding smaller things in base58, like timestamps. Its also useful for people who use this sort of thing as a link shortener, as you usually won't need to go to very large numbers for a long while. |
Ok, I see. So if you're going to keep reasonable small numbers I think doing For bigger numbers there is This does enforce a fixed size of the messages but I don't think that's a big issue and because this module uses the bitcoin leading-0 compressions even less so, WDYT? |
I'm reluctant to use the I did do a bit of benchmarking on it, and solutions like that ended up taking ~2x as long. So while this doesn't work quite the way the others do, it is the faster solution in this specific case. |
So on a closer look the only difference seems to be when leading zeros are in play, which of course does not make sense for integers. So that's all good. All things being the same these methods should already exists and could be extracted as submethods from the already existing ones. There is some slight difference like the use of divmod which I think will be more performant. |
I'll check in on that in an hour or so and update the PR accordingly |
You were right about divmod, by the way. Shaved off ~0.7us, and made it slightly more consistently fast. |
LGTM |
Awesome, thanks |
* Add integer support * Optimize away slicing in favor of iterator reversed()
This pull request adds the ability to encode integers in Base58. If it gets merged with #11, I will need an additional commit to add type hinting.