Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

eth_call returns output of contract creations #6420

Merged
merged 5 commits into from
Sep 5, 2017
Merged

Conversation

tomusdrw
Copy link
Collaborator

Required to optimize token balances fetch in a light client friendly mode. @ngotchac please have a look.

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Aug 30, 2017
@@ -294,7 +308,8 @@ impl<'a, B: 'a + StateBackend, E: Engine + ?Sized> Executive<'a, B, E> {
data: None,
call_type: CallType::None,
};
(self.create(params, &mut substate, &mut tracer, &mut vm_tracer), vec![])
let mut out = if output_from_create { Some(vec![]) } else { None };
(self.create(params, &mut substate, &mut tracer, &mut vm_tracer, &mut out), out.unwrap_or_else(Vec::new))
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to keep the same prototype as for call? (out between substate and tracer)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Fixed.

Copy link
Collaborator

@debris debris left a comment

Choose a reason for hiding this comment

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

We already have a tracer. Can't we just reuse output from it? Imo this pr brings unnecessary complexity.

@debris debris added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 1, 2017
@ngotchac
Copy link
Contributor

ngotchac commented Sep 1, 2017

I think it's mostly for light client compatibility. And it's only about 20 more lines, doesn't look that complexe, does it ?

@tomusdrw
Copy link
Collaborator Author

tomusdrw commented Sep 1, 2017

@debris The call is run without a tracer (we don't need it), but we are interested in the output (that's what eth_call is all about - you submit a call and see what it returned).

Also as @ngotchac mentioned tracing is not (and most likely won't be) available for light clients, so we cannot use that in the UI if we want it to be compatible.

@debris
Copy link
Collaborator

debris commented Sep 1, 2017

If you don't want to / can't trace everything, we could introduce just another type of tracer, which traces only transaction output

@ngotchac
Copy link
Contributor

ngotchac commented Sep 1, 2017

Wouldn't that add a lot more complexity ?

@debris debris added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Sep 1, 2017
@debris
Copy link
Collaborator

debris commented Sep 1, 2017

ok, after talking with @tomusdrw I understand the code better and understand why the code is implemented as it is

I am ok with the changes, but it would be good to get at least 1 more person to review this code

@@ -528,7 +544,7 @@ impl<'a, B: 'a + StateBackend, E: Engine + ?Sized> Executive<'a, B, E> {
let mut subvmtracer = vm_tracer.prepare_subtrace(params.code.as_ref().expect("two ways into create (Externalities::create and Executive::transact_with_tracer); both place `Some(...)` `code` in `params`; qed"));

let res = {
self.exec_vm(params, &mut unconfirmed_substate, OutputPolicy::InitContract(trace_output.as_mut()), &mut subtracer, &mut subvmtracer)
self.exec_vm(params, &mut unconfirmed_substate, OutputPolicy::InitContract(output.as_mut().or(trace_output.as_mut())), &mut subtracer, &mut subvmtracer)
Copy link
Contributor

@rphmeier rphmeier Sep 4, 2017

Choose a reason for hiding this comment

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

does this mean that whenever saving creation output, trace output won't be saved? the two settings aren't used together yet, but something to watch out for

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. Because trace_output is always the same as output (if both are selected) we just re-use the same value to get it. Couple of lines below this you can see that trace_output is copied either from output or trace_output.

@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 4, 2017
@gavofyork gavofyork merged commit 7462a69 into master Sep 5, 2017
@gavofyork gavofyork deleted the eth-call-result branch September 5, 2017 11:22
tomusdrw added a commit that referenced this pull request Sep 11, 2017
* eth_call returns output of contract creations

* Fix parameters order.

* Save outputs for light client as well.
gavofyork pushed a commit that referenced this pull request Sep 11, 2017
* Fix slow balances (#6471)

* Update token updates

* Update token info fetching

* Update logger

* Minor fixes to updates and notifications for balances

* Use Pubsub

* Fix timeout.

* Use pubsub for status.

* Fix signer subscription.

* Process tokens in chunks.

* Fix tokens loaded by chunks

* Linting

* Dispatch tokens asap

* Fix chunks processing.

* Better filter options

* Parallel log fetching.

* Fix signer polling.

* Fix initial block query.

* Token balances updates : the right(er) way

* Better tokens info fetching

* Fixes in token data fetching

* Only fetch what's needed (tokens)

* Fix linting issues

* Revert "Transaction permissioning (#6441)"

This reverts commit eed0e8b.

* Revert "Revert "Transaction permissioning (#6441)""

This reverts commit 8f96415.

* Update wasm-tests.

* Fixing balances fetching

* Fix requests tracking in UI

* Fix request watching

* Update the Logger

* PR Grumbles Fixes

* PR Grumbles fixes

* Linting...

* eth_call returns output of contract creations (#6420)

* eth_call returns output of contract creations

* Fix parameters order.

* Save outputs for light client as well.

* Don't accept transactions above block gas limit.

* Expose health status over RPC (#6274)

* Node-health to a separate crate.

* Initialize node_health outside of dapps.

* Expose health over RPC.

* Bring back 412 and fix JS.

* Add health to workspace and tests.

* Fix compilation without default features.

* Fix borked merge.

* Revert to generics to avoid virtual calls.

* Fix node-health tests.

* Add missing trailing comma.

* Fixing/removing failing JS tests.

* do not activate genesis epoch in immediate transition validator contract (#6349)

* Fix memory tracing.

* Add test to cover that.

* ensure balances of constructor accounts are kept

* test balance of spec-constructed account is kept
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants