Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement System.Callback.* interops #1238

Merged
merged 9 commits into from
Jul 30, 2020
Merged

Implement System.Callback.* interops #1238

merged 9 commits into from
Jul 30, 2020

Conversation

fyrchik
Copy link
Contributor

@fyrchik fyrchik commented Jul 28, 2020 •
edited
Loading

Close #1197 .
Close #1237 .

Regarding syscalls, only parameter count is stored, as we don't need parameter types.

@codecov
Copy link

codecov bot commented Jul 28, 2020 •
edited
Loading

Codecov Report

Merging #1238 into master will increase coverage by 3.63%.
The diff coverage is 95.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1238      +/-   ##
==========================================
+ Coverage   62.86%   66.49%   +3.63%     
==========================================
  Files         200      204       +4     
  Lines       17251    17297      +46     
==========================================
+ Hits        10844    11501     +657     
+ Misses       5823     5181     -642     
- Partials      584      615      +31
Impacted Files Coverage Δ
pkg/core/gas_price.go 100.00% <ø> (ø)
pkg/core/opcode_price.go 100.00% <ø> (ø)
pkg/vm/interop.go 78.26% <55.55%> (-2.20%) ⬇️
pkg/core/interop/context.go 96.72% <92.59%> (-3.28%) ⬇️
pkg/consensus/payload.go 97.53% <100.00%> (+0.01%) ⬆️
pkg/core/blockchain.go 69.26% <100.00%> (ø)
pkg/core/interop/callback/callback.go 100.00% <100.00%> (ø)
pkg/core/interop/callback/method.go 100.00% <100.00%> (ø)
pkg/core/interop/callback/pointer.go 100.00% <100.00%> (ø)
pkg/core/interop/callback/syscall.go 100.00% <100.00%> (ø)
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f24e707...9a99286. Read the comment docs.

AnnaShaleva
AnnaShaleva requested changes Jul 29, 2020
View reviewed changes
pkg/core/gas_price.go Show resolved Hide resolved
pkg/core/interop/crypto/interop.go Show resolved Hide resolved
pkg/core/interop/json/json_test.go Show resolved Hide resolved
pkg/core/interop/crypto/interop.go Outdated Show resolved Hide resolved
pkg/vm/interop.go Show resolved Hide resolved
pkg/core/interops.go Outdated Show resolved Hide resolved
pkg/core/interops.go Outdated Show resolved Hide resolved
pkg/core/interops.go Outdated
Comment on lines 109 to 110
{Name: "Neo.Crypto.CheckMultisigWithECDsaSecp256r1", Func: crypto.ECDSASecp256r1CheckMultisig, Price: 0, ParamCount: 1},
{Name: "Neo.Crypto.CheckMultisigWithECDsaSecp256k1", Func: crypto.ECDSASecp256k1CheckMultisig, Price: 0, ParamCount: 1},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these two interops there should be at least 3 parameters, but there could be more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, there should be exactly 3 parameters. And we need to fix (s *Stack) PopSigElements() function (just remove default case, now both pubs and sigs are arrays), perhaps I missed that in the interop refactoring PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there are 3 parameters, but we don't need to remove the default case.
Parsing in C# code is a bit complicated, as "Array" can be unpacked. See https://github.com/neo-project/neo/blob/master/src/neo/SmartContract/ApplicationEngine.cs#L179

pkg/core/interop/context.go Show resolved Hide resolved
pkg/core/interop/context.go Show resolved Hide resolved
AnnaShaleva
AnnaShaleva requested changes Jul 29, 2020
View reviewed changes
pkg/core/interops.go Outdated Show resolved Hide resolved
AnnaShaleva
AnnaShaleva requested changes Jul 29, 2020
View reviewed changes
pkg/core/interops.go Outdated Show resolved Hide resolved
@fyrchik
*: move syscall handling out of VM
51ae12e 
Remove interop-related structures from the `vm` package.

Signed-off-by: Evgenii Stratonikov <evgeniy@nspcc.ru>
@fyrchik
core: add prices for TRY* opcodes
c09ea04 
Signed-off-by: Evgenii Stratonikov <evgeniy@nspcc.ru>
@fyrchik
core: implement System.Callback.* interops
382a7f5 
Support creating callbacks from pointers.

Signed-off-by: Evgenii Stratonikov <evgeniy@nspcc.ru>
@fyrchik
core/tests: introduce loadScript helper
e654d22 
Signed-off-by: Evgenii Stratonikov <evgeniy@nspcc.ru>
@fyrchik
core/tests: refactor getting contract state
f96c217 
Get 2 contracts in pair which is useful everytime we need to test
syscall with one contract calling the other.

Signed-off-by: Evgenii Stratonikov <evgeniy@nspcc.ru>
@fyrchik
core: implement System.Callback.CreateFromMethod interop
c54b45e 
Support creating callbacks from contract methods.

Signed-off-by: Evgenii Stratonikov <evgeniy@nspcc.ru>
@fyrchik
core: implement System.Callback.CreateFromSyscall interop
99b0397 
Allow to create callbacks from syscalls.

Signed-off-by: Evgenii Stratonikov <evgeniy@nspcc.ru>
@fyrchik
core: allow to restrict creating callbacks from syscalls
23a1430 
Specify DisallowCallback flag if syscall is not allowed to be used in a
callback.

Signed-off-by: Evgenii Stratonikov <evgeniy@nspcc.ru>
@fyrchik
interop: add Secp2k to the list of crypto interops
9a99286 
Allow to use them during verification.
@fyrchik fyrchik merged commit 82692d8 into master Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AnnaShaleva AnnaShaleva AnnaShaleva approved these changes

@roman-khimov roman-khimov Awaiting requested review from roman-khimov roman-khimov is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move syscall handling out of VM Implement System.Callback.* interops
2 participants
@fyrchik @AnnaShaleva