Rename transaction steps to "apply" and "undo"

[nazarhussain]

The Transaction interface is the primary and major interaction for most of the Lisk SDK Developers. So the interfaces presented there must be generic and easy to understand specially without detail explanations.

Currently we have the transaction steps prepare, applyAsset and undoAsset. These are the three interfaces which everyone have to implement for any custom transaction. The first step prepare seems straight forward to understand what it would be. But for next two steps applyAsset and undoAsset we are mixing up a complete concept Asset there, while the steps are not just mutate asset also can change non-asset attributes as well. And with that concept, people can get confused if its the asset of account or asset of transaction.

So we need to rename those steps to particular verbs, that will make them easy to read and understand the transaction interface, as well to explain.

class MyTransaction extends BaseTransaction {
   prepare(store) {}
   apply(store) {}
   undo(store) {}
}

Which version(s) does this affect? (Environment, OS, etc...)

2.1.x

[pablitovicente]

This will need not only renaming but also refactoring.

apply() calls applyAsset() and both modify the store so we would need to merge that functionality; maybe override apply() in the transactions that are using applyAsset() and made the required adjustments to code and test

[mitsuaki-u]

There is some functionality in apply which are common across transactions such as verifying signatures and multisignatures as well as subtracting the fee from account, that we should be careful with to not override.

[pablitovicente]

There is some functionality in apply which are common across transactions such as verifying signatures and multisignatures as well as subtracting the fee from account, that we should be careful with to not override.

@lsilvs suggested using super.apply() maybe that could work then? edit: calling super.apply() only will not work directly we'll need to anyway change the apply() function.

[shuse2]

I think this issue is more of naming, maybe it's easy to do

BaseTransaction.apply => BaseTransaction.applyAll
CustomTransaction.applyAsset => CustomTransaction.apply

but it's also bit misleading in a way that custom transaction shouldn't apply the base transaction part of the stuff

[shuse2]

And with that concept, people can get confused if its the asset of account or asset of transaction.

I gave little bit more thought on this, and I think applyAsset is the correct name as what it is doing. It should apply the asset of itself to account(state). I don't know how to do it in a nice way other than documentation, but we need to clearly communicate transaction(itself) cannot be mutated.

I agree that the misleading point is account has asset now, so maybe change the name of account.asset to something else?

[nazarhussain]

@shuse2 Account asset is not misleading in this context. When we process any transaction, it can change any thing in the state store. It can be account or even any custom entity as well. Its not necessary that apply will only use asset attributes of transaction, it can use other attributes to apply to some other custom entity. Mean its all flexible what ever use want to do here.

So using a generic name apply makes more sense rather than applyAsset, which clearly mis misleading concept in reference to what its supposed to do.

[lsilvs]

What about rename it to applyState? Or if one want to be more specific, applyAccountState?

[shuse2]

Superseded by #5639