-
Notifications
You must be signed in to change notification settings - Fork 324
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
Move Verify to SealedEnvelope #3197
Move Verify to SealedEnvelope #3197
Conversation
I believe that the |
Verify function is now SealedEnvelope method
ea0bf29
to
9b6e49c
Compare
Codecov Report
@@ Coverage Diff @@
## master #3197 +/- ##
=======================================
Coverage 74.99% 74.99%
=======================================
Files 227 227
Lines 21196 21196
=======================================
Hits 15895 15895
Misses 4463 4463
Partials 838 838
Continue to review full report at Codecov.
|
thx for contribution. It'd be better if you can write unit test for |
@@ -179,3 +183,27 @@ func wrapStakingActionIntoExecution(ab AbstractAction, toAddr []byte, pb proto.M | |||
data: data, | |||
}, nil | |||
} | |||
|
|||
// Verify verifies the action using sender's public key | |||
func (sealed *SealedEnvelope) Verify() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is hard to say whether it is appropriate to have this function as a function of struct sealedenvelope for the following reasons:
- It uses so many public functions of itself, which is not recommended in our codebase
- The original
Verify
function is more like a util function, which groups some checks together. It is only used inaction/protocol/generic_validator.go
. The usage of this function in several test files are misuse. We can split it into several checks:
a. check public key
b. check gas limit
c. check signature
check a and b could be moved intoaction/protocol/generic_validator.go
Verify function is now SealedEnvelope method