search

google / mundane

Mundane is a Rust cryptography library backed by BoringSSL that is difficult to misuse, ergonomic, and performant (in that order).
MIT License
1.07k stars 46 forks source link

Rename Signature::verify to is_valid #16

Closed joshlf closed 5 years ago

joshlf commented 5 years ago

Signature::verify takes a public key and a signature and returns a boolean. @zarvox has pointed out that it's not 100% obvious to somebody coming across code like if sig.verify(&pub) { ... } which return value means that the signature is valid and which means that it's invalid.

In order to make it more obvious, we should rename this method to is_valid.

yonson2 commented 5 years ago

@joshlf I've submitted a PR regarding this issue #17, the only problem I have with it is that the EcdsaSignature trait already implemented an is_valid method, I looked at the body of the function and ended up renaming it to has_content, I don't know if you are ok with that naming convention and I'm open to suggestions regarding what to do about it.

categulario commented 5 years ago

@yonson2 maybe you can use the fully qualified syntax for choosing which implementation to call so you don't have to rename anything:

https://doc.rust-lang.org/book/ch19-03-advanced-traits.html#fully-qualified-syntax-for-disambiguation-calling-methods-with-the-same-name

joshlf commented 5 years ago

@yonson2 Ah good point about the name conflict. How does is_valid_format sound?

yonson2 commented 5 years ago

@categulario thanks for that, I'll have it in mind for future occasions for sure but I think in this particular case its better for each trait to have a different method to prevent ambiguity. As for which name I like @joshlf's suggestion better than the one I had initially set so I've submitted that change.