Implement ext_call in ink! #133

taskooh · 2019-07-08T06:45:35Z

I wanna call another contract in a contract.
Is this the right approach to do that?

Robbepop

All in all looks decent.
However, you should note that this is a fairly low-level API to what you want to do.
For ink! we had in mind to support a more high-level approach to solve the underlying problem.
For the high-level parts we also need the low-level parts such as implemented in your PR so if you need it I am fine to merge the PR after some minor improvements (also docs are missing).

core/src/env/srml/srml_only/impls.rs

core/src/env/traits.rs

taskooh

I added the gas generic type

core/src/env/srml/srml_only/impls.rs

Robbepop

You should simply rebase to master, not do a merge commit. I suggest you to rewind and do a simple rebase.

codecov-io · 2019-07-23T11:59:38Z

Codecov Report

Merging #133 into master will decrease coverage by 0.12%.
The diff coverage is 11.76%.

@@            Coverage Diff             @@
##           master     #133      +/-   ##
==========================================
- Coverage   79.54%   79.41%   -0.13%     
==========================================
  Files          67       67              
  Lines        4986     4994       +8     
==========================================
  Hits         3966     3966              
- Misses       1020     1028       +8

Impacted Files	Coverage Δ
model/src/exec_env.rs	`72.72% <ø> (ø)`	⬆️
core/src/env/srml/types.rs	`0% <ø> (ø)`	⬆️
core/src/env/test_env.rs	`37.61% <11.76%> (-3%)`	⬇️
model/src/contract.rs	`89.28% <0%> (+13.92%)`	⬆️

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 55fb144...724ebe1. Read the comment docs.

taskooh · 2019-07-24T09:22:08Z

I implemented ReturnData as trait because implementation of get() differs between srml and test_env
to do this, it was difficult to add generic types to ReturnData itself so i added generic types to the get() function.

taskooh · 2019-07-26T06:16:48Z

@Robbepop
I rebased from master. is there something missing?

core/src/env/mod.rs

core/src/env/srml/srml_only/impls.rs

Robbepop · 2019-07-26T10:47:41Z

Sorry, I forgot to push my latest code review for your PR and was already wondering why you don't react. Let me elaborate a bit more on the idea I stated in a comment above about splitting the ext_call API.

There currently is a problem with the proposed API that ext_call might return data that is to be fetch through the scratch buffer. However, not all calls actually return something. So if you implement an automatic scratch buffer retrieval for potential return data you might end up doing this wasted work.

Also the proposed work around with the additional guard type that you already implemented is not working properly either due to the stated problems with in-between-calls.

The idea to fix this is splitting this API up into two parts:

fn ext_call_invoke(..)
Won't return anything but also won't touch the scratch buffer.
Users should use this for calls where they either won't use the result or where the call won't return data.
#[must_use] fn ext_call_evaluate<T: Decode>(..) -> T
Always fetches and decodes the data in the scratch buffer after the call has been made.
Also guarantees that users actually use the return value.

I am sorry for causing more work than necessary but we want to get this right and I think this is the way to go.

Robbepop · 2019-07-29T17:26:59Z

Please rebase this on current master to fix CI issues.

model/src/exec_env.rs

Robbepop · 2019-07-29T21:58:45Z

model/src/exec_env.rs

+
+    /// Calls a remote smart contract and return access to scratch buffer
+    pub fn call_evaluate<U:Decode>(
+        &self,


Same here: &mut self

model/src/exec_env.rs

core/src/env/traits.rs

core/src/env/test_env.rs

core/src/env/srml/srml_only/impls.rs

Robbepop · 2019-08-08T08:21:29Z

Hey how is it going with this PR?
Can you please rebase on to master so we can actually merge it as soon as you fixed what I have already mentioned to you?
It would be very nice if we could finally pull this over the finishing line.

taskooh · 2019-08-12T09:47:39Z

@Robbepop Sorry for being late, i was busy for my job.
I realize that test_env is quite not done.

Robbepop

Please incorporate my suggestions into your PR and run rustfmt over it - many formatting errors.

Robbepop · 2019-08-12T10:24:13Z

core/src/env/traits.rs

+    ) -> Result<(), Self::CallError>;
+
+    /// Calls a remote smart contract and return encoded data
+    #[must_use]


Since we are using Result as return type we can remove this #[must_use] again since Result is #[must_use] on a type level already.

Robbepop · 2019-08-12T10:28:34Z

core/src/env/srml/srml_only/impls.rs

@@ -107,6 +112,8 @@ impl<T> Env for SrmlEnv<T>
 where
    T: EnvTypes,
 {
+    type CallError = CallError;


We do not want to introduce an associated type for CallError at this point.

Do you have any idea where to implement CallError struct?
I want to use same struct in core/src/env/* and in model/src/exec_env.rs

Since ink_model depends on ink_core just define it in ink_core in ink_core::env and use it from ink_model.

core/src/env/srml/srml_only/impls.rs

core/src/env/test_env.rs

Robbepop · 2019-08-12T10:42:32Z

core/src/env/test_env.rs

+        input_data: &[u8],
+    ) -> Result<T,CallError> {
+        add_call(callee, gas, value, input_data);
+        unimplemented!();


remove this

Suggested change

unimplemented!();

I can see why you left it there. The returning of this is a bit more involved.
What I would recommend doing is the following:
Create another field in the test environment data (Vec<u8>) that is settable by the user and that is actually taken upon calling call to decode into the expected T. So upon testing a smart contract using the test environment a tester can completely control it and say what it expects from the outside.

I got it!
Also, i need to check error of call. Should i add a field of error flag?
Now it can catch only the decoding error, but cant catch error while executing call.

core/src/env/test_env.rs

Robbepop · 2019-08-12T10:47:30Z

lang/src/encode_input_data.rs

+use ink_utils;
+use parity_codec::Encode;
+
+pub trait EncodeSafe{


It is not idiomatic to do that and confusing. Please find another way to test this functionality. You are free to make limited use of macro_rules! macros in order to provide the functionality of your tests if required.

Robbepop · 2019-08-14T12:56:57Z

@taskooh we are about to implement a more elaborate version of this feature.

For this we want to base our work on your PR, however, this requires the last remaining work done by you, otherwise we cannot merge it. We do not want to throw away all the work done by you so far. ;)
It would be nice if you could fix the remaining problems so that we can move on this whole feature.

…xt_call remove #[allow(unused)] from ext_call() implment call_invoke and call_evaluate fix format call_invoke and call_evaluate returns Return<_,CallError> fixed

Robbepop

please apply my code suggestions and remove the one test file I mentioned.
also it is important that you fix the things I mentioned in your current TestEnvData::call implementation -> it is still calling unimplemented!().
rest looks O.K.

core/src/env/srml/srml_only/impls.rs

core/src/env/test_env.rs

lang/src/encode_input_data.rs

…t_env

Robbepop · 2019-08-15T17:17:45Z

core/src/env/test_env.rs

@@ -47,6 +50,15 @@ impl EventData {
    }
 }

+/// Emulates the data given to remote smart contract call instructions.
+#[allow(unused)]


why would you need #[allow(unused)]?

Make the fields pub so that they are accessible from the outside. ;)

core/src/env/test_env.rs

taskooh · 2019-08-15T17:58:31Z

@Robbepop @ascjones Is the return_data in
https://github.com/paritytech/ink/pull/133/conflicts
same as call_return which I implemented?

Robbepop · 2019-08-15T17:33:48Z

core/src/env/test_env.rs

@@ -47,6 +50,15 @@ impl EventData {
    }
 }

+/// Emulates the data given to remote smart contract call instructions.
+#[allow(unused)]


Make the fields pub so that they are accessible from the outside. ;)

core/src/env/srml/srml_only/impls.rs

ascjones

In general looks good. Though as I say in the comment I would move call_invoke and call_evaluate up to model::EnvHandler

core/src/env/srml/srml_only/impls.rs

Robbepop · 2019-08-16T12:04:03Z

Even though there are some outstanding things to do, we (@ascjones and me) decided that we are going to merge this PR and refine it quickly to base our enhancements on top of it. I hope this is to the interest of you @taskooh . Thank you for your work!

Robbepop requested changes Jul 8, 2019

View reviewed changes

core/src/env/srml/srml_only/impls.rs Outdated Show resolved Hide resolved

core/src/env/srml/srml_only/impls.rs Show resolved Hide resolved

core/src/env/traits.rs Outdated Show resolved Hide resolved

taskooh commented Jul 9, 2019

View reviewed changes

core/src/env/srml/srml_only/impls.rs Show resolved Hide resolved

Robbepop requested changes Jul 9, 2019

View reviewed changes

core/src/env/srml/srml_only/impls.rs Outdated Show resolved Hide resolved

Robbepop reviewed Jul 10, 2019

View reviewed changes

core/src/env/srml/srml_only/impls.rs Outdated Show resolved Hide resolved

core/src/env/srml/srml_only/impls.rs Outdated Show resolved Hide resolved

core/src/env/srml/srml_only/impls.rs Outdated Show resolved Hide resolved

Robbepop requested changes Jul 15, 2019

View reviewed changes

core/src/env/srml/srml_only/impls.rs Outdated Show resolved Hide resolved

core/src/env/srml/srml_only/impls.rs Outdated Show resolved Hide resolved

Robbepop reviewed Jul 15, 2019

View reviewed changes

taskooh force-pushed the addcall branch from cea7c37 to 0d17a72 Compare July 15, 2019 16:26

Robbepop changed the title ~~use ext_call in ink!~~ Implement ext_call in ink! Jul 23, 2019

taskooh force-pushed the addcall branch from c2e4d18 to 9a9854e Compare July 26, 2019 05:19

Robbepop reviewed Jul 26, 2019

View reviewed changes

core/src/env/mod.rs Show resolved Hide resolved

core/src/env/srml/srml_only/impls.rs Outdated Show resolved Hide resolved

Robbepop mentioned this pull request Jul 26, 2019

ink! 2.0 Roadmap #153

Closed

Robbepop reviewed Jul 29, 2019

View reviewed changes

taskooh force-pushed the addcall branch from 975966d to ca14fe0 Compare July 30, 2019 06:49

taskooh force-pushed the addcall branch from ca14fe0 to 8a4685e Compare August 12, 2019 08:54

Robbepop requested changes Aug 12, 2019

View reviewed changes

import ext_call to trait and add function to generate input_data of e…

ad40643

…xt_call remove #[allow(unused)] from ext_call() implment call_invoke and call_evaluate fix format call_invoke and call_evaluate returns Return<_,CallError> fixed

taskooh force-pushed the addcall branch from 707fbc8 to ad40643 Compare August 15, 2019 14:58

fix format and change return type of test_env

afa251e

Robbepop requested changes Aug 15, 2019

View reviewed changes

core/src/env/srml/srml_only/impls.rs Outdated Show resolved Hide resolved

core/src/env/srml/srml_only/impls.rs Outdated Show resolved Hide resolved

core/src/env/test_env.rs Outdated Show resolved Hide resolved

lang/src/encode_input_data.rs Outdated Show resolved Hide resolved

taskooh added 2 commits August 16, 2019 02:08

[core] implement CallError as struct and add call_return field to tex…

bfde69e

…t_env

fix trivial and remove lang/src/encode_input_data

6eb55e3

Robbepop requested changes Aug 15, 2019

View reviewed changes

ascjones self-requested a review August 15, 2019 17:36

[core] use map_err while handling Result<>

defdf59

Robbepop requested changes Aug 15, 2019

View reviewed changes

ascjones reviewed Aug 16, 2019

View reviewed changes

core/src/env/srml/srml_only/impls.rs Show resolved Hide resolved

Merge branch 'master' into addcall

724ebe1

Robbepop merged commit 1228905 into use-ink:master Aug 16, 2019

Robbepop mentioned this pull request Aug 16, 2019

[core] ext_call PR followup and refinements #171

Merged

Implement ext_call in ink! #133

Implement ext_call in ink! #133

Uh oh!

Conversation

taskooh commented Jul 8, 2019

Uh oh!

Robbepop left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taskooh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Robbepop left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

taskooh commented Jul 24, 2019

Uh oh!

taskooh commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Robbepop commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Robbepop commented Jul 29, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Robbepop commented Aug 8, 2019

Uh oh!

taskooh commented Aug 12, 2019

Uh oh!

Robbepop left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Jul 23, 2019 •

edited

Loading

taskooh commented Jul 26, 2019 •

edited

Loading

Robbepop commented Jul 26, 2019 •

edited

Loading