refactor: call_main utility for integration test

[jakmeier]

We have been copying the same code multiple times, this refactor
pulls out the functionality the TestEnv.

[matklad]

Mostly 👍 but curious if you think we can do better with the interface to specify transactions?

[matklad]

One thing I don't like here is that the API is non-compistional. If at some point we introduce some kind of transaction builder which returns a Tx, we'd have to mirror the API here, as there isn't a function which takes a Tx.

This is a non-trivial problem: tx has a nonce, and a block hash, and has to be signed, and we don't want the caller to get the nonce.

So perhaps we need the following, slightly hacky API?

pub fn tx(&mut self, mut tx: Transaction) {
  tx.nonce = tip.height + 1;
  tx.block_hash = tip.hash;
  let signer = Signer::from_seed(tx.signer_id.as_str())
  let tx = tx.sign(&signer)
}

The caller would then basically specify garbage nonce & hash

[jakmeier]

Hm, I was really hoping to get rid of the caller-site giving garbage nonce and hash.

I have a different suggestion how to make it more compositional, basically split the "create tx" part from the "execute tx" into two functions.

fn tx_from_actions(
    &mut self,
    actions: Vec<Action>,
    signer: &InMemorySigner,
    receiver: AccountId,
) -> SignedTransaction;

pub fn execute_tx(&mut self, tx: SignedTransaction) -> FinalExecutionOutcomeView;

This also opened up more removal of duplicated code in test_deploy_cost_increased. The new interface and this additional refactor are included in the latest two commits.

@matklad Do you think this is better now?

[matklad]

Yeah, I definitelly like the new API better, in particular, execute_tx(&mut self, tx: SignedTransaction) is very much the right boundary.

I think I'd still prefer garbage nonce approach, but that's debatable. In particular, I feel like that's more of a problem of the split between Tx and SignedTx -- ideally, "what the TX logically is" (list of actions, sender, receiver) should be separate from stuff we need to make cryptograhpy work (nonce, block hash, signature), at least in the testing API. Ideally, we'd have some kind of TxBuilder, but a Tx with Default nonce&block hash can stand in its place. But either of these is a bigger change than warranted in this PR, even if they are good ideas.

[jakmeier]

I think the logical piece of work that we generally care about is just a list of actions, not a transaction.

What you describe as

"what the TX logically is" (list of actions, sender, receiver)

in my opinion is not a Transaction. The crypto stuff to define valid placement in the chain very much belongs to a valid transaction definition. The difference between Transaction and SignedTransaction is just how we represent it in memory but logically they describe the same thing.

But I support the idea of a TX builder that prepares actions/signer/receiver and builds into a full TX. But fn tx_from_actions is already quite simple to build a TX. I think to make a builder valuable, it would need to do more than just that. For example, track accounts included in genesis, or help somehow with preparing the receiver with a deployed contract first etc.

[matklad]

Magically defaulting to 0 client is somewhat odd, but I also don't feel like we should add client_id param everywhere. I'd say this OK, maybe at some point in the future we'll learn how a better API would look like.

[matklad]

Would be great to make it just a method of the builder object! But, with the way real and kv runtimes are setup, that's not going to be trivial, so lgtm.

[matklad]

Yeah, I definitelly like the new API better, in particular, execute_tx(&mut self, tx: SignedTransaction) is very much the right boundary.

I think I'd still prefer garbage nonce approach, but that's debatable. In particular, I feel like that's more of a problem of the split between Tx and SignedTx -- ideally, "what the TX logically is" (list of actions, sender, receiver) should be separate from stuff we need to make cryptograhpy work (nonce, block hash, signature), at least in the testing API. Ideally, we'd have some kind of TxBuilder, but a Tx with Default nonce&block hash can stand in its place. But either of these is a bigger change than warranted in this PR, even if they are good ideas.