refactor signatureValidator #295
Conversation
charlesjhongc
force-pushed
the
v5-refactor-SignatureValidator
branch
from
5c5db12 to
f591e29
Compare
| // may lead the caller to incorrectly believe that the | ||
| // signature was invalid.) | ||
| revert("SignatureValidator#isValidSignature: unsupported signature"); | ||
| revert("SignatureValidator#isValidSignature: illegal signature"); |
There was a problem hiding this comment.
Maybe we can skip or replace SignatureValidator#isValidSignature?
There was a problem hiding this comment.
@charlesjhongc Should the prefix be updated?
There was a problem hiding this comment.
Updated in 472d85b
| // that we currently support. In this case returning false | ||
| // may lead the caller to incorrectly believe that the | ||
| // signature was invalid.) | ||
| revert("SignatureValidator: unsupported signature"); |
There was a problem hiding this comment.
This will never be reached because we check signatureTypeRaw <= uint8(SignatureType.ZX1271)?
There was a problem hiding this comment.
The SignatureType .Invalid will reach here.
There was a problem hiding this comment.
Maybe can remove SignatureType.Illegal case?
There was a problem hiding this comment.
Since Illegal also serves the purpose of representing the default value of enum type, maybe we can remove Invalid instead? (This final revert is implicitly the case for Invalid anyway.)
There was a problem hiding this comment.
The goal is not changing the enum order. But if you're talking about if (signatureType == SignatureType.Illegal) case, yeah I was thinking about removing this.
There was a problem hiding this comment.
yeah I was thinking about removing this.
Yeah that would make sense. Though I'm still not fully understand the difference between Illegal and Invalid.🧐
There was a problem hiding this comment.
After discussion, fixed in 472d85b
No description provided.