-
Notifications
You must be signed in to change notification settings - Fork 79
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
Add tests for verifyTx
and verifyHeader
#1318
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1318 +/- ##
==========================================
+ Coverage 68.07% 68.25% +0.17%
==========================================
Files 215 215
Lines 18508 18524 +16
==========================================
+ Hits 12599 12643 +44
+ Misses 5227 5212 -15
+ Partials 682 669 -13
Continue to review full report at Codecov.
|
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.
verifyTxWitnesses
and verifyHashAgainstScript
remain.
return ErrWitnessHashMismatch | ||
} | ||
} else { | ||
cs, err := interopCtx.DAO.GetContractState(hash) |
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's a separate non-preview3-compatible feature, it shouldn't be a part of this PR.
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.
Are you sure? It was 4 months ago neo-project/neo#1481
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.
Let me check, I was thinking more of neo-project/neo#1800.
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.
Yeah, this part is really preview3, but it should substitute ScriptFromWitness()
then.
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.
ScriptFromWitness
has no necessary context, and its signature will become not so understandable.
I am thinking about making verifyHashAgainstScript
public.
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.
That's a good option too, simplifying consensus a bit along the way.
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.
I'd postpone it until after release, as it requires some refactoring of both core
and consensus
to do right.
In NEO3 we can't just appcall hash, as verification script has no access to state. Instead we use `verify` method of an arbitrary contract.
Close #1305 .
Related #1292 .