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

More IS refactoring #939

Merged
merged 1 commit into from
Aug 12, 2016
Merged

More IS refactoring #939

merged 1 commit into from
Aug 12, 2016

Conversation

UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Aug 12, 2016

No description provided.

@crowning-
Copy link

Nit: I'm used to have boolean methods named "is_" or "has_", e.g. vote.IsSignatureValid() instead of vote.CheckSignature().
You already did this with IsAllVotesValid(), so I think this would be more consistent naming wise.

@UdjinM6
Copy link
Author

UdjinM6 commented Aug 12, 2016

@crowning- Yeah, we actually have all variants of it in code - CheckSignature, VerifySignature and SignatureValid in different modules. (And even IsSignatureValid itself which actually should be IsScriptSigValid, lol, I just realized that, will fix :) ). There is VerifySignature in original bitcoin code which also returns bool but it takes signature as an argument (and it is closer to VerifyMessage in that way). And then there is CheckSignature in alerts module which also returns bool and verifies signature it already has (and in that sense it's much closer to what we do imo) but it has pubkey as an argument. Furthermore, we are already using CheckSignature in PS and sporks. So after going through all of this I though that CheckSignature would be a good candidate actually.

PS. Should have written all this in PR description initially but thanks for bringing the question up :)

@schinzelh
Copy link

Yeah, the codebase is not consistent re. this.
utACK 149f27a anyway :)

@schinzelh schinzelh merged commit e20e1dc into dashpay:v0.12.1.x Aug 12, 2016
@UdjinM6 UdjinM6 deleted the imprISRef branch August 28, 2016 12:22
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