-
Notifications
You must be signed in to change notification settings - Fork 141
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
DX: Keyreg bytes #522
DX: Keyreg bytes #522
Conversation
Previously, these fields were required to be base64 encoded, which seems like a strange requirement. Now they may be bytes or b64 strings. Either way, they are stored in the python object as b64 strings to maintain compatibility with our original decision, regardless of how strange it was. Added a few doc string cleanups.
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.
Looks good and nice code cleanup. Consider my suggestion optional but encouraged
return None | ||
if isinstance(key, (bytes, bytearray)) and len(key) == size: | ||
return base64.b64encode(key) | ||
if len(base64.b64decode(key)) == size: |
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.
maybe also add isinstance(key, str)
otherwise b64decode
will probably throw an exception (did not check tho)
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 suppose it will, but that's life in a dynamic language, I think. Before you could have stuffed something crazy in that field, and you'd get an exception somewhere when we tried to serialize. Now it happens earlier, which I think is better.
But I don't want to try to turn Python into Go and insert type checks all over the place.
Allow the various key-ish fields of a keyreg txn to be bytes
Previously, these fields were required to be base64 encoded, which
seems like a strange requirement.
Added a few doc string cleanups.