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

core: contractCall from native contracts #1271

Merged
merged 3 commits into from
Aug 7, 2020
Merged

Conversation

AnnaShaleva
Copy link
Member

Closes #1200

I made an example of such call, but not sure if this testcase covers the problem. If so, than we are able to perform calls from native contracts and it might be necessary to implement a separate method for such calls.

@AnnaShaleva AnnaShaleva self-assigned this Aug 5, 2020
@AnnaShaleva AnnaShaleva force-pushed the core/call_from_native branch 2 times, most recently from 9b78da3 to 1b84514 Compare August 5, 2020 11:48
@codecov
Copy link

codecov bot commented Aug 5, 2020

Codecov Report

Merging #1271 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1271      +/-   ##
==========================================
- Coverage   67.51%   67.50%   -0.02%     
==========================================
  Files         216      216              
  Lines       18368    18368              
==========================================
- Hits        12401    12399       -2     
- Misses       5300     5302       +2     
  Partials      667      667              
Impacted Files Coverage Δ
pkg/rpc/client/wsclient.go 83.22% <0.00%> (-1.35%) ⬇️

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 b0f0dc5...db27ddc. Read the comment docs.

@roman-khimov
Copy link
Member

This is native contract calling another native contract. Have you tried calling regular contracts from native contract?

@roman-khimov
Copy link
Member

And it's spawning another VM, which doesn't seem to be correct.

@roman-khimov
Copy link
Member

At the same time C# node mostly changes ApplicationEngine here which is an internal (to that node's implementation) thing. Still, we should consider this scenario without new VM spawning, it should be possible to do using our current VM/interop.Context APIs.

@AnnaShaleva AnnaShaleva changed the title core: add test to call contract from native core: contractCall from native contracts Aug 7, 2020
@AnnaShaleva
Copy link
Member Author

Now we have VM into interop context, so we're able to call other contracts from native contracts, but it's strange a bit, because if VM fails during invoking inner contractCall, we'll have failed VM after inner cantractCall completed. Consequently, outer contractCall fails, as these calls share the VM.

@roman-khimov
Copy link
Member

if VM fails during invoking inner contractCall, we'll have failed VM after inner cantractCall completed. Consequently, outer contractCall fails, as these calls share the VM.

Well, that's a feature. That's what would happen with regular contracts calling each other and native contracts shouldn't be that much different.

@@ -317,7 +317,7 @@ func TestNativeContract_InvokeOtherContract(t *testing.T) {
})

t.Run("non-native contract", func(t *testing.T) {
// put some other contract into chain
// put some other contract into chain (this contract just pushes `5` on stack)
Copy link
Member

Choose a reason for hiding this comment

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

This belongs to some different commit.

@@ -35,7 +35,6 @@ type Context struct {
Notifications []state.NotificationEvent
Log *zap.Logger
Invocations map[util.Uint160]int
ScriptGetter vm.ScriptHashGetter
Copy link
Member

Choose a reason for hiding this comment

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

We probably no longer need vm.ScriptHashGetter type either.

}
_ = vm.Run()
if vm.HasFailed() {
return stackitem.NewBigInteger(big.NewInt(-2))
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit strange that we're returning something here, but I guess there are not that many options we have.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was a test method, so I just put something there. What should we do in this case?

Copy link
Member

Choose a reason for hiding this comment

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

Well, the vm already has failed. We shouldn't touch it, but at the same time we have to return something for Func, so it probably could be just about anything.

We have VM inside the context, so don't need ScriptHashGetter anymore.
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.

I think it's good enough for now, the API might be a bit lacking, but at least there are no blockers for any real native contract requiring this feature.

@roman-khimov roman-khimov merged commit f513149 into master Aug 7, 2020
@roman-khimov roman-khimov deleted the core/call_from_native branch August 7, 2020 18:24
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.

Make sure we can make calls from native contracts
2 participants