rpc: refactor invokecontractverify #1825
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #1825 +/- ##
==========================================
- Coverage 83.38% 83.38% -0.01%
==========================================
Files 281 281
Lines 22370 22384 +14
==========================================
+ Hits 18654 18664 +10
- Misses 2575 2578 +3
- Partials 1141 1142 +1
Continue to review full report at Codecov.
|
fyrchik
reviewed
Mar 11, 2021
fyrchik
reviewed
Mar 11, 2021
AnnaShaleva
force-pushed
the
fix-invokecontractverify
branch
2 times, most recently
from
2159d02
to
a2599ad
Compare
It needs only VM and function to get contract state. Also exposed this method and extended Blockchainer. These changes are needed for the next commit.
In `(c *Client) AddNetworkFee` we define network fee for contract witness verification via `invokecontractverify` RPC call, and that's the initial purpose of this RPC method. But it was not implemented correctly. It used `System.Contract.Call` instead of beheiving like `initVerificationVM`. During real contract witness verification the whole contract's script is loaded into VM, and then we jump to the `verify` method. Thus, to define exact contract verification price, we should act like this (and not just perform `System.Contract.Call` of `verify` method). Tests are added. This bug is the reason of adding extra GAS (c.notary.extraVerifyFee) to pre-calculated value in https://github.com/nspcc-dev/neofs-node/pull/404/files#diff-639db437ca2578db46c9e8cbf18f9aa01f8ca5aee30e0fa7e70ba0354822d7b3R237
AnnaShaleva
force-pushed
the
fix-invokecontractverify
branch
from
a2599ad
to
edfca68
Compare
roman-khimov
approved these changes
Mar 16, 2021
There was a problem hiding this comment.
Good, but let's wait for neo-project/neo-modules#564 to ensure we're compatible.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Close #1799.
Problem
In
(c *Client) AddNetworkFeewe define network fee for contractwitness verification via
invokecontractverifyRPC call, and that's theinitial purpose of this RPC method. But it was not implemented
correctly. It used
System.Contract.Callinstead of beheiving likeinitVerificationVM.During real contract witness verification the whole contract's script is
loaded into VM, and then we jump to the
verifymethod. Thus, to defineexact contract verification price, we should act like this (and not just
perform
System.Contract.Callofverifymethod).Tests are added.
This bug is the reason of adding extra GAS (c.notary.extraVerifyFee) to
pre-calculated value in
pkg/morph/client/notary.go, L237.Solution
I would suggest to propose the same changes in C# repo.
Additional info
Our
(c *Client) AddNetworkFeestill is working properly with only those contracts which do not have arguments forverifymethod (because of the line 773). It may be fixed in a separate PR (if needed).