Skip to content
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

Decode minikeys (like Casascius coin ones) #141

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

juestr
Copy link
Contributor

@juestr juestr commented Jul 1, 2017

This adds decode_minikey to base58.py minikey.py. It checks for format and checksum errors.

There are some test cases and an example cli tool:

$ ./examples/minikey.py -h 
usage: minikey.py [-h] minikey

Decode a minikey to base58 format.

positional arguments:
  minikey     the minikey

optional arguments:
  -h, --help  show this help message and exit

Security warning: arguments may be visible to other users on the same host.

$ ./examples/minikey.py S6c56bnXQiBjk9mqSYE7ykVQ7NzrRy 
5JPy8Zg7z4P7RSLsiqcqyeAF1935zjNUdMxcDeVrtU1oarrgnB7

minikey = minikey.encode('ascii')
length = len(minikey)
if length not in [22, 30]:
raise InvalidMinikeyError('Minikey length %d is not 22 or 30' % length)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest using str("length is {} yo").format(length) instead of using the % operator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you please explain why?

  • % is not deprecated, and I find it even more readable
  • % is used throughout the whole project
  • for this use case the extra power of format provides no benefit as far as I can see

@petertodd
Copy link
Owner

I think the minikey functionality should be in a different file than base58.

Also, it doesn't make sense to decode minikeys to base58 - minikey decoding should be done to/from raw private keys.

@juestr
Copy link
Contributor Author

juestr commented Jul 21, 2017

Right, I was blinded by my use case. Btw there is no way to create a minikey from a raw key, it's one way only. I am going to extract everything to a minikey.py when I find time.

@petertodd
Copy link
Owner

Ah right, I should have realized a minikey is obviously a one-way transformation.

Note that you also may consider just making this functionality a separate Python library - it might even be easier for people to find that way given that they may not be expecting to find such an old standard in a newer library.

@juestr
Copy link
Contributor Author

juestr commented Jul 30, 2017

It's just a few lines of code so I moved it to minikey.py and fixed the return type. If you don't want it in here at all that's fine for me too.

Btw, the magic constructor of CBitcoinSecret confused me quite a bit.

@juestr juestr closed this Jul 30, 2017
@juestr juestr reopened this Jul 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants