feat(sign): optimize signing exception log#5446
feat(sign): optimize signing exception log#5446halibobo1205 merged 1 commit intotronprotocol:developfrom
Conversation
lurais
changed the title
lurais
force-pushed
the
feature/optimize_validate_sign_log
branch
from
2b033c6 to
5c8a233
Compare
| } | ||
| return SM2.signatureToAddress(messageHash, signatureBase64); | ||
| } catch(IllegalArgumentException e) { | ||
| if (e.getMessage().contains("Invalid point compression")) { |
There was a problem hiding this comment.
Why special treatment, I understand that you can throw a SignatureException whenever signatureToAddress is incorrect.
There was a problem hiding this comment.
Currently only the exception with message like "Invalid point compression" can be sure to deal like this way, and other kind of messages can not be found now, that means just treat it like this may be not correct and that will not be noticed in this way.
There was a problem hiding this comment.
I'm not sure I agree with this.
There was a problem hiding this comment.
From the method signatureToKeyBytes in class ECKey, other kinds of error messages has been judged, that means not treat them the same, and it is core method ,any error messages deserved to be noticed, if we are not sure how to deal, we should throw it.
There was a problem hiding this comment.
Which scenarios will generate IllegalArgumentException？
There was a problem hiding this comment.
IllegalArgumentException
When the signature is not correct but the length is right, for example, if someone send a transaction , and the correct signature is "abc", but it was replaced to "001", that will bring it a IllegalArgumentException.
There was a problem hiding this comment.
Actually, I think all exceptions in this scene are SignatureException even 001
There was a problem hiding this comment.
Actually, I think all exceptions in this scene are SignatureException even
001
yes , it is clear to throw an SignatureException in this scene, while the point is that shall we just throw a SignatureException in any unknown scene. @tomatoishealthy
There was a problem hiding this comment.
I guess I have got your point, let's analogy with an example: imaging that there is a method func add(Object a, Object b) return int which is used for plus a and b and returns the sum.
As you mentioned above, if a or b is a string type, we should throw IllegalArgumentException. If a or b is a float or double type which is not an integer but can be executed with math calculation, we should throw another exception(whatever the exception type is).
In this example, we can clearly know all exceptions may occur, but for signature, how you can identify other exceptions? Just as you say, the signature was replaced to 001 from abc, and the length is the same, for the signatureToAddress() it won't know whether the signature is matched only after doing the check.
Do you think 001 is a wrong signature just because it was constructed by number? if so, you may need to do research on whether numbers can be parts of a signature.
Meanwhile, if numbers can't be parts of a signature and you want to intercept these exceptions, better to add an extra parameter check logic and put the logic before signatureToAddress()
|
@lurais Please re-check your PR and don't bring any irrelevant information. |
lurais
force-pushed
the
feature/optimize_validate_sign_log
branch
from
8b59783 to
0efeae8
Compare
|
Better link an issue and show your motivation, or give more details in this content |
|
updated. @tomatoishealthy |
lurais
force-pushed
the
feature/optimize_validate_sign_log
branch
2 times, most recently
from
11141c6 to
ffc08a5
Compare
lurais
force-pushed
the
feature/optimize_validate_sign_log
branch
from
ffc08a5 to
52a3e53
Compare
4 participants
What does this PR do?
Optimize signing exception log， close #5423
Why are these changes required?
Standardize the signing related exception
This PR has been tested by:
Follow up
Extra details