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

Provide documentation for VerifyingKey and SigningKey #117

Merged
merged 3 commits into from
Oct 17, 2019

Conversation

tomato42
Copy link
Member

Document the VerifyingKey, SigningKey and few directly related classes.

fixes #56

work towards #95

@tomato42 tomato42 added the maintenance issues related to making the project usable or testable label Sep 28, 2019
@tomato42 tomato42 self-assigned this Sep 28, 2019
@tomato42 tomato42 added this to the v0.14 milestone Sep 28, 2019
@coveralls
Copy link

coveralls commented Sep 28, 2019

Coverage Status

Coverage increased (+0.1%) to 92.602% when pulling 81391a0 on tomato42:docs into 78f7cf4 on warner:master.

@tomato42
Copy link
Member Author

@ansasaki please review

Copy link

@ansasaki ansasaki left a comment

Choose a reason for hiding this comment

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

I left some comments, not all of them are requests for changes, but requests for clarification.

Overall the proposed documentation is well written and concise.

src/ecdsa/keys.py Outdated Show resolved Hide resolved
src/ecdsa/keys.py Show resolved Hide resolved
src/ecdsa/keys.py Outdated Show resolved Hide resolved
src/ecdsa/keys.py Outdated Show resolved Hide resolved
src/ecdsa/keys.py Outdated Show resolved Hide resolved
src/ecdsa/util.py Outdated Show resolved Hide resolved
src/ecdsa/util.py Outdated Show resolved Hide resolved
@tomato42
Copy link
Member Author

thanks for review; will fix the issues I haven't commented on

@tomato42
Copy link
Member Author

@ansasaki updated, please re-check

Copy link

@ansasaki ansasaki left a comment

Choose a reason for hiding this comment

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

I found some typos.

"""
Encode the signature to raw format (:term:`raw encoding`)

:param int r: first parametr of the signature

Choose a reason for hiding this comment

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

s/parametr/parameter/


:param int r: first parametr of the signature
:param int s: second parameter of the signature
:param int order: the order of curve over which the signature was computed

Choose a reason for hiding this comment

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

s/the order of curve/the order of the curve/

It's expected that this function will be used as a `sigencode=` parameter
in SigningKey.sign() method.

:param int r: first parametr of the signature

Choose a reason for hiding this comment

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

s/parametr/parameter


:param int r: first parametr of the signature
:param int s: second parameter of the signature
:param int order: the order of curve over which the signature was computed

Choose a reason for hiding this comment

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

s/the order of curve/the order of the curve/

src/ecdsa/keys.py Show resolved Hide resolved
@tomato42
Copy link
Member Author

I found some typos.

@ansasaki good catch, can you verify?

@tomato42
Copy link
Member Author

(I've also "sphinxified" the cross-links and made them a bit more consistent)

Copy link

@ansasaki ansasaki left a comment

Choose a reason for hiding this comment

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

Found a small typo

Abstract Syntax Notation 1 is a standard description language for
specifying serialisation and deserialisation of data structures in a
portable and cross-platform way.
"""

Choose a reason for hiding this comment

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

Just a nit, the glossary could be alphabetically sorted.

Copy link
Member Author

Choose a reason for hiding this comment

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

it will be once it is processed by sphinx :)

# raw encoding of point is invalid in DER files
if not point_str.startswith(b("\x00")) or \
len(point_str[1:]) == curve.verifying_key_length:
# the bitsting encoding is padded with a zero byte

Choose a reason for hiding this comment

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

s/bitsting/bitstring/

Copy link
Member Author

Choose a reason for hiding this comment

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

heh, that was actually fixed by the new code that fixed behaviour of bitstring so the rebase fixed it

also do minor cleanup with initialisers and imports
use canonical name for first parameter in classmethods

minor fixes with formatting
Copy link

@ansasaki ansasaki left a comment

Choose a reason for hiding this comment

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

LGTM!

@tomato42 tomato42 merged commit 5fa2fd8 into tlsfuzzer:master Oct 17, 2019
@tomato42
Copy link
Member Author

Thank you!

@tomato42 tomato42 deleted the docs branch October 17, 2019 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance issues related to making the project usable or testable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python 3: SigningKey.to_string() return a bytes object instead of a string
3 participants