LiskArchive / lisk-sdk

🔩 Lisk software development kit
https://lisk.com
Apache License 2.0
2.72k stars 454 forks source link

Transaction must encapsulate its domain "fully" #3866

Closed nazarhussain closed 4 years ago

nazarhussain commented 5 years ago

Each transaction must encapsulate all its logic fully. There should not be any business logic related to a particular transaction defined outside transaction scope.

Currently there are few code mentioned outside their particular domain;

const transactionResponse = transaction.apply(stateStore);
votes.apply(stateStore, transaction, this.exceptions);
stateStore.transaction.add(transaction);

In this example it seems vote logic is outside vote transaction. If its not the vote logic, then we should refactor and rename it properly.

public addMultisignature(
store: StateStore,
signatureObject: SignatureObject,
): TransactionResponse {

public addVerifiedMultisignature(signature: string): TransactionResponse

These methods available in base transaction, but actually belongs to the "MultiSignature" transaction. We need to come up with more elegant solution for:

  1. If a custom transaction need to change behaviour of BaseTransaction, how to define it.
  2. The interface specification or hooks which can be used to hook in or modify base transaction behaviour.
  3. Do we or how we should expose similar functionality for this custom transaction developer
  4. Improve documentation for this behaviour in the SDK

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

2.1.0

shuse2 commented 4 years ago

With the new multisignature #4836 , and vote transaction #4915 , this should be resolved.