Remove BaseTransaction extension requirement

[mitsuaki-u]

What was the problem?

Any custom transaction should not be required to explicitly extend BaseTransaction as long as it implements all the required functionality.

How did I fix it?

Modified registerTransaction checks.

Review checklist

• The PR resolves Remove registerTransaction check for extending BaseTransaction #3821
• All new code is covered with unit tests
• All new code was formatted with Prettier
• Linting passes
• Tests pass
• Commit messages follow the commit guidelines
• Documentation has been added/updated

[shuse2]

@mitsujutsu
This should target release/2.1.0 branch

[nazarhussain]

I am not sure why the issue was initiated. To my understanding the behaviour to extend from BaseTransaction must be required. This will give us flexibility in extending the framework in future with confidence. As we will be sure all transactions are inherited from particular type. You may not look for a particular/categorical use case right now, but in future to release some enchantment related to it without breaking everyones code, this feature must stay.

And this is not blocking developers to in their custom functionality, rather just restricting to follow patterns and structure our sdk/framework defined, which is a good practice in designing any mature framework.

@shuse2 @MaciejBaj

[mitsuaki-u]

@nazarhussain

Extending BaseTransaction should not be required if the user can implement the required functionality on their own. They can still extend BaseTransaction if they want to. Also this check was causing issues with type 5, 6 and 7 transactions in lisk-core, even if they extended BaseTransaction it was still not passing the check.

[nazarhussain]

@mitsujutsu

this check was causing issues with type 5, 6 and 7 transactions in lisk-core

This should be debugged and fixed separately, its not related to this assertion.

Extending BaseTransaction should not be required if the user can implement

With this reference we will loose the structural understanding of any custom transaction, users can do something strange with in their implementation. Also if you look at this code of the constructor for BaseTransaction, that refers we are doing some important stuff that must be done for all transactions.

https://github.com/LiskHQ/lisk-sdk/blob/f9e550505b7342feb22ea2cc05a593f50569f2d8/elements/lisk-transactions/src/base_transaction.ts#L140-L177

Also look at this code which refers to this getter signature which makes use of this._signature. So now user have to implement this getter, that will get more complicated on user end, which can be resolved by just extending from the BaseTransaction

https://github.com/LiskHQ/lisk-sdk/blob/f9e550505b7342feb22ea2cc05a593f50569f2d8/elements/lisk-transactions/src/base_transaction.ts#L187-L193

And see at these lines without extending, we ask user to initialise these properties to their implementation.

https://github.com/LiskHQ/lisk-sdk/blob/f9e550505b7342feb22ea2cc05a593f50569f2d8/elements/lisk-transactions/src/base_transaction.ts#L151-L176

@MaciejBaj

[shuse2]

@nazarhussain
I don't think it's strictly required to extend the BaseTransaction.
User should extend it for usability and support, but at the same time, it doesn't add much value to restrict them either.
Also, it might add extra condition that we don't want (I need to check it)
How does instanceof check if it's the same instance? if it require truly the same instance, user need to use exact same version of the library, even though there is no breaking change, which doesn't make sense.

[shuse2]

Above discussion will be tackle on #3877

[nazarhussain]

We should close this PR and replace this check with better extended approach in other issue if we want. If you use BaseTransaction properly that will work flawless. So far we are not focusing, documenting, providing samples to suggest that transaction can be JSON object. Every sample and every document refers that custom transaction should be extended by BaseTransaction from lisk-sdk package. I don’t see any reason to divert that direction all of sudden.

[shuse2]

@nazarhussain Without this PR, lisk-core is currently not usable, which affects other developments.
It will be tackle within 2.1 release at #3877

[nazarhussain]

That refers to wrong usage BaseTransaction in Lisk-Core. I don't think rather than fixing it, removing this check is a good way to proceed forward.