refactor: Move run_transaction params into fields of a TxInput struct and make doc comments of its fields

[ltfschoen]

Purpose of PR

Extension to PR #10742 to address review comment #10742 (comment).

Usage

Check that tests still pass:

cargo test -p evmbin -- --nocapture

View docs and check that user able to click "info" module and then choose the "TxInput" struct that should be generated in target/doc/parity_evm/info/struct.TxInput.html

cargo doc -p evmbin --no-deps --document-private-items --open

Changes made

• Move run_transaction parameters into fields of a new TxInput struct since comments about parameters of the function do not appear in the documentation, whereas doc comments applied to fields of a public struct are.

Note: Issues that were encountered:

Details

* **NOTE** There is a lot of duplication in declaring instances of the `TxInput` struct in main.rs. I haven't figured out how to refactor it to introduce more code reuse, since only the `informant` field changes. Note: Already tried making it mutable with let mut tx_input = TxInput { ... }` and then just modifying the value of that field with say `tx_input.informant = display::std_json::Informant::err_only()` before applying that value as an argument of `run_transaction`.

Example: The following is a snippet of existing code in main.rs

// Execute the given transaction and verify resulting state root
// for CLI option `--std-dump-json` or `--std-json`.
if args.flag_std_dump_json || args.flag_std_json {
	if args.flag_std_err_only {
		let tx_input = TxInput {
			name: &name,
			idx,
			spec: &spec,
			pre_state: &pre,
			post_root,
			env_info: &env_info,
			transaction,
			informant: display::std_json::Informant::err_only(),
			trie_spec: &trie_spec,
		};
		// Use Standard JSON informant with err only
		info::run_transaction(tx_input)
	} else if args.flag_std_out_only {
		let tx_input = TxInput {
			name: &name,
			idx,
			spec: &spec,
			pre_state: &pre,
			post_root,
			env_info: &env_info,
			transaction,
			informant: display::std_json::Informant::out_only(),
			trie_spec: &trie_spec,
		};
		// Use Standard JSON informant with out only
		info::run_transaction(tx_input)
	} else {

I tried changing it to the following

let mut tx_input = TxInput {
	name: &name,
	idx,
	spec: &spec,
	pre_state: &pre,
	post_root,
	env_info: &env_info,
	transaction,
	informant: display::simple::Informant::default(),
	trie_spec: &trie_spec,
};

// Execute the given transaction and verify resulting state root
// for CLI option `--std-dump-json` or `--std-json`.
if args.flag_std_dump_json || args.flag_std_json {
	if args.flag_std_err_only {
		tx_input.informant = display::std_json::Informant::err_only();
		// Use Standard JSON informant with err only
		info::run_transaction(tx_input)
	} else if args.flag_std_out_only {
		tx_input.informant = display::std_json::Informant::out_only();
		// Use Standard JSON informant with out only
		info::run_transaction(tx_input)
	} else {

But I get errors

error[E0308]: mismatched types
   --> evmbin/./src/main.rs:221:28
    |
221 |                         tx_input.informant = display::std_json::Informant::err_only();
    |                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `display::simple::Informant`, found struct `display::std_json::Informant`
    |
    = note: expected type `display::simple::Informant`
               found type `display::std_json::Informant<std::io::Stderr, std::io::Stderr>`

error[E0308]: mismatched types
   --> evmbin/./src/main.rs:225:28
    |
225 |                         tx_input.informant = display::std_json::Informant::out_only();
    |                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `display::simple::Informant`, found struct `display::std_json::Informant`
    |
    = note: expected type `display::simple::Informant`
               found type `display::std_json::Informant<std::io::Stdout, std::io::Stdout>`

error: aborting due to 2 previous errors

[parity-cla-bot]

It looks like @ltfschoen signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[tomusdrw]

Nice!

[tomusdrw]

Is this change strictly required for something? It seems you have to clone the spec anyway, so maybe better take it by value than by reference.

[ltfschoen]

I've tried what you recommended but I haven't been able to implement it successfully as it opened a can of worms. This is where I got to ed5e667

Details

Compiling evmbin v0.1.0 (/home/ltfschoen/code/github/paritytech/parity-ethereum-master/evmbin) error[E0504]: cannot move `params` into closure because it is borrowed --> evmbin/./src/info.rs:95:34 | 94 | run(spec, trie_spec, ¶ms.gas, spec.genesis_state(), |mut client| { | ---------- borrow of `params.gas` occurs here 95 | let result = match client.call(params, &mut trace::NoopTracer, &mut informant) { | ^^^^^^ move into closure occurs here

error[E0504]: cannot move tx_input into closure because it is borrowed
--> evmbin/./src/info.rs:153:43
|
152 | let result = run(&spec_checked, trie_spec, &tx_input.transaction.gas, &pre_state, |mut client| {
| -------------------- borrow of tx_input.transaction occurs here
153 | let result = client.transact(&env_info, tx_input.transaction, trace::NoopTracer, tx_input.informant);
| ^^^^^^^^ move into closure occurs here

error[E0382]: capture of partially moved value: tx_input
--> evmbin/./src/info.rs:153:43
|
136 | let TxInput { name, idx, spec, pre_state, post_root, env_info, trie_spec, .. } = tx_input;
| --------- value moved here
...
153 | let result = client.transact(&env_info, tx_input.transaction, trace::NoopTracer, tx_input.informant);
| ^^^^^^^^ value captured here after move
|
= note: move occurs because tx_input.trie_spec has type ethcore::TrieSpec, which does not implement the Copy trait

error: aborting due to 3 previous errors

[tomusdrw]

yeah, if you destructure tx_input you can't really move it around. just destructure transaction as well and pass it?

[ltfschoen]

Thanks, that worked, very slick! I've pushed changes in this commit 4b98474

[dvdplm]

Do we really need all the members to be pub? It is generally avoided so that the code is easier to change in the future. Maybe it could be a way to use the Default trait to avoid supplying all args all the time? It would also let you do fancy things like have MyChainTxInput types where e.g. the name member is pre-set and you can have logic to ensure users don't misuse the api (say, use a ForkSpec that is invalid for the given chain spec name).

[ltfschoen]

I didn't figure out how to use the Default trait, but I've pushed an approach here
in this commit 426a438, but unfortunately I can't work out how to overcome these errors

Details

error: expected expression, found `}` --> evmbin/./src/info.rs:139:3 | 139 | } | ^ expected expression

error: expected expression, found }
--> evmbin/./src/info.rs:146:3
|
146 | }
| ^ expected expression

error: expected expression, found }
--> evmbin/./src/info.rs:153:3
|
153 | }
| ^ expected expression

error: expected expression, found }
--> evmbin/./src/info.rs:160:3
|
160 | }
| ^ expected expression

error: expected expression, found }
--> evmbin/./src/info.rs:167:3
|
167 | }
| ^ expected expression

error[E0433]: failed to resolve: use of undeclared type or module tx_input
--> evmbin/./src/main.rs:221:7
|
221 | tx_input::std_json_err_only();
| ^^^^^^^^ use of undeclared type or module tx_input

error[E0433]: failed to resolve: use of undeclared type or module tx_input
--> evmbin/./src/main.rs:225:7
|
225 | tx_input::std_json_out_only();
| ^^^^^^^^ use of undeclared type or module tx_input

error[E0433]: failed to resolve: use of undeclared type or module tx_input
--> evmbin/./src/main.rs:229:7
|
229 | tx_input::std_json_default();
| ^^^^^^^^ use of undeclared type or module tx_input

error[E0433]: failed to resolve: use of undeclared type or module tx_input
--> evmbin/./src/main.rs:237:7
|
237 | tx_input::json_default();
| ^^^^^^^^ use of undeclared type or module tx_input

error[E0433]: failed to resolve: use of undeclared type or module tx_input
--> evmbin/./src/main.rs:241:7
|
241 | tx_input::simple_default();
| ^^^^^^^^ use of undeclared type or module tx_input

error[E0308]: mismatched types
--> evmbin/./src/info.rs:137:15
|
137 | informant: display::std_json::Informant::err_only(),
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected type parameter, found struct display::std_json::Informant
|
= note: expected type T
found type display::std_json::Informant<std::io::Stderr, std::io::Stderr>

error[E0063]: missing fields env_info, fork_spec_name, post_root and 5 other fields in initializer of info::TxInput<'_, _>
--> evmbin/./src/info.rs:136:3
|
136 | TxInput {
| ^^^^^^^ missing env_info, fork_spec_name, post_root and 5 other fields

error[E0308]: mismatched types
--> evmbin/./src/info.rs:144:15
|
144 | informant: display::std_json::Informant::out_only(),
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected type parameter, found struct display::std_json::Informant
|
= note: expected type T
found type display::std_json::Informant<std::io::Stdout, std::io::Stdout>

error[E0063]: missing fields env_info, fork_spec_name, post_root and 5 other fields in initializer of info::TxInput<'_, _>
--> evmbin/./src/info.rs:143:3
|
143 | TxInput {
| ^^^^^^^ missing env_info, fork_spec_name, post_root and 5 other fields

error[E0308]: mismatched types
--> evmbin/./src/info.rs:151:15
|
151 | informant: display::std_json::Informant::default(),
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected type parameter, found struct display::std_json::Informant
|
= note: expected type T
found type display::std_json::Informant<std::io::Stderr, std::io::Stdout>

error[E0063]: missing fields env_info, fork_spec_name, post_root and 5 other fields in initializer of info::TxInput<'_, _>
--> evmbin/./src/info.rs:150:3
|
150 | TxInput {
| ^^^^^^^ missing env_info, fork_spec_name, post_root and 5 other fields

error[E0308]: mismatched types
--> evmbin/./src/info.rs:158:15
|
158 | informant: display::json::Informant::default(),
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected type parameter, found struct display::json::Informant
|
= note: expected type T
found type display::json::Informant

error[E0063]: missing fields env_info, fork_spec_name, post_root and 5 other fields in initializer of info::TxInput<'_, _>
--> evmbin/./src/info.rs:157:3
|
157 | TxInput {
| ^^^^^^^ missing env_info, fork_spec_name, post_root and 5 other fields

error[E0308]: mismatched types
--> evmbin/./src/info.rs:165:15
|
165 | informant: display::simple::Informant::default(),
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected type parameter, found struct display::simple::Informant
|
= note: expected type T
found type display::simple::Informant

error[E0063]: missing fields env_info, fork_spec_name, post_root and 5 other fields in initializer of info::TxInput<'_, _>
--> evmbin/./src/info.rs:164:3
|
164 | TxInput {
| ^^^^^^^ missing env_info, fork_spec_name, post_root and 5 other fields

error: aborting due to 20 previous errors


[dvdplm]

Should tx_input.spec perhaps be named fork_spec (and spec_name be fork_spec_name)?

[ltfschoen]

Yes great idea. I definitely think fork_spec is a better name than just spec. Yes I think we should rename spec_name to fork_spec_name

[ltfschoen]

I've pushed commits with those changes in commits 2e17bb3 and b828e33