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

Typing annotations for BIP-0340 #202

Merged
merged 1 commit into from
Apr 10, 2020
Merged

Typing annotations for BIP-0340 #202

merged 1 commit into from
Apr 10, 2020

Conversation

ysangkok
Copy link

@ysangkok ysangkok commented Mar 19, 2020

This is on top of #200.

Typing makes the code easier to read. It also makes it easier to see where assumptions about non-infinity are being made.

@real-or-random
Copy link

I like the typing annotations, that's neat! I'm not so sure about the assertions though. Since this is a reference implementation, people could get the idea that they need to implement them in their code.

@ysangkok
Copy link
Author

I can remove the assertions, but they do have a purpose. The code will not type check with the --strict flag unless they are there. I think the annotations are still an improvement even if it doesn't type check.

@sipa
Copy link
Owner

sipa commented Apr 7, 2020

@real-or-random I'm not sure that's a problem, actually. I would hope that most implementations of the inner logic used here is implemented in compiled, statically-typed languages, so the typing annotations would actually be a help for that.

Concept ACK

EDIT: I realize I misread, and you were talking about the assertions and not typing annotations. My comment doesn't apply then, but I still think it's not really a problem - real implementations will be very different anyway (at least I hope so).

@sipa
Copy link
Owner

sipa commented Apr 7, 2020

Maybe rebase on current branch?

Passes mypy's strict-mode with mypy 0.770.
@ysangkok
Copy link
Author

ysangkok commented Apr 7, 2020

@sipa Rebased on 1d999cf. debug_print_vars needed adjusting, it now has an extra assert.

def is_square(x):
return pow(x, (p - 1) // 2, p) == 1
def is_square(x: int) -> bool:
return int(pow(x, (p - 1) // 2, p)) == 1
Copy link
Owner

Choose a reason for hiding this comment

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

What is this int conversion for? Does pow return another type?

Copy link
Author

Choose a reason for hiding this comment

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

pow can return float. For example, try pow(2, -1). This is also noted in the common issues section of the Mypy manual. More discussion at mypy issue 285. If you prefer to avoid the coercion (which won't actually coerce), I think it would be possible to use cast, which is always guaranteed to be the identity function at runtime. Which do you prefer?

Copy link
Owner

Choose a reason for hiding this comment

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

Let's keep it like this. I suspect cast is more obscure to most readers.

@sipa
Copy link
Owner

sipa commented Apr 9, 2020

@real-or-random @jonasnick: opinions?

@real-or-random
Copy link

ACK

Copy link

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

Concept ACK

This is a nice improvement to readability. I wasn't aware of these developments in python. I noticed that when you replace assert P is not None with P = None there's no runtime exception thrown by a type checker. How do you actually type check "with --strict"?

@real-or-random
Copy link

@jonasnick I think there's a misunderstanding. The (commonly used) type checker is a different binary mypy (http://mypy-lang.org/). It's a different project, so you may even need to install it separately. It accepts --strict.

Copy link

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

Ah, so more dependencies. Can confirm that this works with mypy and that the asserts are required. We "just" need to remember to run this whenever we change the code. Well I guess someone will speak up eventually if it doesn't pass mypy.

ACK

@sipa sipa merged commit cf2937c into sipa:bip-taproot Apr 10, 2020
@ysangkok ysangkok deleted the bip-0340-typing branch April 11, 2020 04:20
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.

4 participants