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.

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

interop: post-preview2 adjustment #1187

Merged
merged 9 commits into from
Jul 17, 2020

Conversation

AnnaShaleva
Copy link
Member

First part of #1055.

  1. Were added:

    • System.Binary.Base64Encode
    • System.Binary.Base64Decode
    • System.Contract.GetCallFlags
  2. Were fixed:

    • System.Blockchain.GetContract
    • System.Blockchain.GetTransactionFromBlock
    • System.Contract.CallEx
    • System.Contract.Create
    • System.Contract.IsStandard
    • System.Contract.Update
  3. No errors in:

    • System.Binary.Deserialize
    • System.Binary.Serialize
    • System.Blockchain.GetBlock
    • System.Blockchain.GetHeight
    • System.Blockchain.GetTransaction
    • System.Blockchain.GetTransactionHeight
    • System.Contract.CreateStandardAccount
    • System.Contract.Destroy
  4. Separate adjustment is needed (see core: adjust System.Contract.Call* interops #1181)

    • System.Contract.Call
    • System.Contract.CallEx

@AnnaShaleva AnnaShaleva self-assigned this Jul 16, 2020
@AnnaShaleva AnnaShaleva changed the title Neo3/interop/post preview2 adjustment interop: post-preview2 adjustment Jul 16, 2020
@AnnaShaleva AnnaShaleva force-pushed the neo3/interop/post-preview2_adjustment branch 3 times, most recently from b251fc6 to d9ace68 Compare July 16, 2020 10:57
@codecov
Copy link

codecov bot commented Jul 16, 2020

Codecov Report

Merging #1187 into master will increase coverage by 0.08%.
The diff coverage is 66.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1187      +/-   ##
==========================================
+ Coverage   66.71%   66.79%   +0.08%     
==========================================
  Files         198      198              
  Lines       16833    16893      +60     
==========================================
+ Hits        11230    11284      +54     
- Misses       5006     5009       +3     
- Partials      597      600       +3     
Impacted Files Coverage Δ
pkg/core/interops.go 100.00% <ø> (ø)
pkg/interop/binary/binary.go 0.00% <0.00%> (ø)
pkg/interop/contract/contract.go 0.00% <0.00%> (ø)
pkg/core/interop_neo.go 47.61% <61.53%> (+7.86%) ⬆️
pkg/smartcontract/manifest/manifest.go 84.50% <71.42%> (-1.44%) ⬇️
pkg/compiler/codegen.go 91.98% <75.00%> (-0.01%) ⬇️
pkg/core/interop_system.go 57.32% <79.31%> (+4.45%) ⬆️
pkg/smartcontract/manifest/method.go 81.25% <100.00%> (+1.25%) ⬆️
pkg/vm/context.go 80.37% <0.00%> (+1.86%) ⬆️
... and 2 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 75dc62f...74d2f43. Read the comment docs.

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

System.Contract.Update needs a bit more love, it also checks for script hash equality now and checks that the new contract can only be non-storage-enabled if there are no storage items lef
t for the old one. It also isn't tested at all.

It'd also be nice to squash 28cab40 with 172b026 and d9ace68, it basically is the same feature.

9e573e5 changes all of syscall definitions and while doing so it'd be nice to sort them by packages.

9e573e5 updates interops, but they should be updated along with implementations, technically you have a broken system between syscall and compiler changes.

But it's a nice update overall.

pkg/smartcontract/manifest/manifest_test.go Outdated Show resolved Hide resolved
pkg/core/interop_system_test.go Outdated Show resolved Hide resolved
pkg/core/interop_neo.go Outdated Show resolved Hide resolved
Part of #1055.

There'll be a lot of interops which result with a struct on stack instead
of interop interface, and sometimes their names are the same, so it's
unrelyable to take into account interop name only and don't pay
attention to it's API (package).

Also sort syscalls by package and name.
To match C# implementation, we should pick all arguments from stack first.
Part of #1055.

It should check not only stored contracts, but also interop context
script container in case if it's a transaction.
@AnnaShaleva AnnaShaleva force-pushed the neo3/interop/post-preview2_adjustment branch from d9ace68 to 54eca98 Compare July 17, 2020 05:20
@AnnaShaleva
Copy link
Member Author

AnnaShaleva commented Jul 17, 2020

I also re-orginised commits, so now we don't have the system broken between commits. The last thing that remains is Contract.Update, I'll update this commit soon.

Part of #1055.

It should put on stack an array instead of interop interface.
Part of #1055.

It should check given scripthash against manifest groups and return the
contract state as a struct (not interop interface).
Part of #1055.

We should check contract scripthash against the one provided in manifest
and manifest groups. We shouldn't put on stack anything after return.
And ofcourse, we mast not destroy the old contract at the end, as
`contractDestroy` removes all storage items associated with the
old contract ID (which equals to the new contract ID). We just remove
old contract state - it's enough.
Part of #1055.

It returns calling flags of the current context.
@AnnaShaleva AnnaShaleva force-pushed the neo3/interop/post-preview2_adjustment branch from 54eca98 to 74d2f43 Compare July 17, 2020 09:36
@roman-khimov roman-khimov merged commit 3f2f0be into master Jul 17, 2020
@roman-khimov roman-khimov deleted the neo3/interop/post-preview2_adjustment branch July 17, 2020 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants