Assign and verify transaction fees in Transactions domain - Closes #3860

[pablitovicente]

What was the problem?

Fee as not a property that could be defined in the transaction

How did I fix it?

• By adding static property FEE
• By removing restriction of fee being greater than 0

How to test it?

Build should be green

Review checklist

• The PR resolves Assign and verify transaction fees in Transactions domain #3860
• 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

[lsilvs]

Looks good. Just small suggestions.

[mitsuaki-u]

lgtm!

[MaciejBaj]

Looks good, minor comments

[nazarhussain]

@pablitovicente Every thing seems good, but have some observation on the mechanism to validate the fee.

1. Validating this.fee is exactly like validating any other attributes of transaction
2. So we don't need to create different validate functions for every other attribute
3. To simplify for user there must be one and only one validate function for a transaction. We already have an issue open to simply and reduce the interfaces in BaseTransaction, so don't tend to create more.

So what I would suggest:

1. Move the logic of validateFee into the validate of the BaseTransaction
2. So actually instead of calling the validateFee, have that fee validation logic right there. Because its already validating all other attributes of transaction.
3. For multi-signature transaction you should override the actual validate method and add your extended validation in it.

@MaciejBaj As discussed earlier as well, we need to keep the transaction interface simple and minimal. I would suggest not to introduce such new methods unless there is no other way to handle the scenario.

[lsilvs]

@nazarhussain I was the one who suggested @pablitovicente to use a new validateFee and overwrite it on MultisignatureTransaction. I don't think it brings extra complexity. Instead, it brings more flexibility. IMO we should even extend this approach and create validation methods for each base attribute allowing users to override its validation according to its use case.

[nazarhussain]

@pablitovicente As just discussed if there is technical limitations of calling super.validate (although I think there must be a solution to it), we should then go with a consistent approach. All attributes of the transaction should have their own validate functions. So the user of the SDK show know one practice, rather than knowing that only for fee there is separate method for all others is there one single validate method. It would be confusing for SDK users.

[pablitovicente]

@pablitovicente As just discussed if there is technical limitations of calling super.validate (although I think there must be a solution to it), we should then go with a consistent approach. All attributes of the transaction should have their own validate functions. So the user of the SDK show know one practice, rather than knowing that only for fee there is separate method for all others is there one single validate method. It would be confusing for SDK users.

Created #3912 to discuss this further @nazarhussain please review again.

[nazarhussain]

@pablitovicente I tried out the analogy and found that you can access the static attributes of child class in the base class. So the argument of not calling super.validate to extend a validation seems not valid any more.

Here is a working example.

class BaseTransaction {
  public fee:number;

  public constructor(fee:number) {
    this.fee = fee;
  }

  public validate() {
    console.log(`${this.getStaticAttribute('name')} (${this.getStaticAttribute('TYPE')})`);
    console.log('Transaction static fee: ', this.getStaticAttribute('FEE'));
    console.log('Transaction fee: ', this.fee);

    return this.fee === this.getStaticAttribute('FEE');
  }

  private getStaticAttribute(attr:string):any {
    return (this.constructor as any)[attr];
  }
}

class MyTransaction extends BaseTransaction{
  static TYPE:string = '1';
  static FEE:number = 123;
}

class MySecondTransaction extends MyTransaction{
  static TYPE:string = '2';
  static FEE:number = 456;

  public validate() {
    return super.validate();
  }
}

const a = new MyTransaction(123);
console.log(a.validate());

const b = new MySecondTransaction(456);
console.log(b.validate());

const c = new MySecondTransaction(568);
console.log(c.validate());

And output of above code is:

TypeScript v3.3.3 linux/amd64
MyTransaction (1)
Transaction static fee:  123
Transaction fee:  123
true

MySecondTransaction (2)
Transaction static fee:  456
Transaction fee:  456
true

MySecondTransaction (2)
Transaction static fee:  456
Transaction fee:  568
false

So that example suggest we can go with single validate method to handle all cases. Please refactor accordingly.

[pablitovicente]

@pablitovicente I tried out the analogy and found that you can access the static attributes of child class in the base class. So the argument of not calling super.validate to extend a validation seems not valid any more.

Here is a working example.

class BaseTransaction {
  public fee:number;

  public constructor(fee:number) {
    this.fee = fee;
  }

  public validate() {
    console.log(`${this.getStaticAttribute('name')} (${this.getStaticAttribute('TYPE')})`);
    console.log('Transaction static fee: ', this.getStaticAttribute('FEE'));
    console.log('Transaction fee: ', this.fee);

    return this.fee === this.getStaticAttribute('FEE');
  }

  private getStaticAttribute(attr:string):any {
    return (this.constructor as any)[attr];
  }
}

class MyTransaction extends BaseTransaction{
  static TYPE:string = '1';
  static FEE:number = 123;
}

class MySecondTransaction extends MyTransaction{
  static TYPE:string = '2';
  static FEE:number = 456;

  public validate() {
    return super.validate();
  }
}

const a = new MyTransaction(123);
console.log(a.validate());

const b = new MySecondTransaction(456);
console.log(b.validate());

const c = new MySecondTransaction(568);
console.log(c.validate());

And output of above code is:

TypeScript v3.3.3 linux/amd64
MyTransaction (1)
Transaction static fee:  123
Transaction fee:  123
true

MySecondTransaction (2)
Transaction static fee:  456
Transaction fee:  456
true

MySecondTransaction (2)
Transaction static fee:  456
Transaction fee:  568
false

So that example suggest we can go with single validate method to handle all cases. Please refactor accordingly.

[pablitovicente]

@pablitovicente I tried out the analogy and found that you can access the static attributes of child class in the base class. So the argument of not calling super.validate to extend a validation seems not valid any more.
Here is a working example.

class BaseTransaction {
  public fee:number;

  public constructor(fee:number) {
    this.fee = fee;
  }

  public validate() {
    console.log(`${this.getStaticAttribute('name')} (${this.getStaticAttribute('TYPE')})`);
    console.log('Transaction static fee: ', this.getStaticAttribute('FEE'));
    console.log('Transaction fee: ', this.fee);

    return this.fee === this.getStaticAttribute('FEE');
  }

  private getStaticAttribute(attr:string):any {
    return (this.constructor as any)[attr];
  }
}

class MyTransaction extends BaseTransaction{
  static TYPE:string = '1';
  static FEE:number = 123;
}

class MySecondTransaction extends MyTransaction{
  static TYPE:string = '2';
  static FEE:number = 456;

  public validate() {
    return super.validate();
  }
}

const a = new MyTransaction(123);
console.log(a.validate());

const b = new MySecondTransaction(456);
console.log(b.validate());

const c = new MySecondTransaction(568);
console.log(c.validate());

And output of above code is:

TypeScript v3.3.3 linux/amd64
MyTransaction (1)
Transaction static fee:  123
Transaction fee:  123
true

MySecondTransaction (2)
Transaction static fee:  456
Transaction fee:  456
true

MySecondTransaction (2)
Transaction static fee:  456
Transaction fee:  568
false

So that example suggest we can go with single validate method to handle all cases. Please refactor accordingly.

@nazarhussain can you please expand your example with the case of Multi-signatures where the fee is not static?

[nazarhussain]

@pablitovicente Original analogy was that since we can't access child static attributes in base class so we have to introduce a separate function to override. And above example refers to solution of that analogy.

But if the reasoning to have validateFee a separate method is to get dynamic fee based on transaction data rather the statically defined fee (current multi signature case), then whole situation changes. With that reference in mind, I would suggest following solutions.

• Make the FEE to be hybrid as getter as well as static value, user can define a static value or define it as a function. So in base transaction we do static validation if FEE defined is a static value, and invoke the function with transaction to get the fee incase its a function.

• Or rather then a static attribute go with function getFee. Each class implement that method and return the fee. The argument of the function would be the transaction data. If its not dependent one can simply return a constant value from function (as in normal transactions). Or compute the fee and return based on data (as in multi-signature)

The original issue refers to the following two points:

• fees of transactions should be verified in validate step of BaseTransaction.
• it should be possible to create "zero fee" transactions extending BaseTransaction interface

Both of these requirements can be achieved in above mentioned directions, defining fee as static value is not a hard-core requirement of the issue itself.

Keeping the near future in mind having a static fee value is also no feasible. As for dynamic fee system we always have to calculate the fee. So the second direction would make more sense here.

[pablitovicente]

@pablitovicente Original analogy was that since we can't access child static attributes in base class so we have to introduce a separate function to override. And above example refers to solution of that analogy.

But if the reasoning to have validateFee a separate method is to get dynamic fee based on transaction data rather the statically defined fee (current multi signature case), then whole situation changes. With that reference in mind, I would suggest following solutions.

• Make the FEE to be hybrid as getter as well as static value, user can define a static value or define it as a function. So in base transaction we do static validation if FEE defined is a static value, and invoke the function with transaction to get the fee incase its a function.
• Or rather then a static attribute go with function getFee. Each class implement that method and return the fee. The argument of the function would be the transaction data. If its not dependent one can simply return a constant value from function (as in normal transactions). Or compute the fee and return based on data (as in multi-signature)

The original issue refers to the following two points:

• fees of transactions should be verified in validate step of BaseTransaction.
• it should be possible to create "zero fee" transactions extending BaseTransaction interface

Both of these requirements can be achieved in above mentioned directions, defining fee as static value is not a hard-core requirement of the issue itself.

Keeping the near future in mind having a static fee value is also no feasible. As for dynamic fee system we always have to calculate the fee. So the second direction would make more sense here.

@nazarhussain the main issue here is Multisig is the only exception currently that need work arounds.

I think I understand your suggestion but the original issue states "Transaction fees should be assignable as part of created transactions extending BaseTransaction interface, e.g. by creating a new static FEE property." and we discussed with @MaciejBaj and @shuse2 and the current implementation respected that issue requirement.

@MaciejBaj @shuse2 @nazarhussain what you think about moving further refactoring to a separate issue where we can discuss further about general lisk-transaction architecture as is being proposed in this issue?

[nazarhussain]

e.g. by creating a new static FEE property. was a suggestion not a requirement. 😃

[pablitovicente]

e.g. by creating a new static FEE property. was a suggestion not a requirement. 😃

But TYPE is already implemented as a static property so I think that the idea was to implement as static property this kind of properties. So I guess that if would take the suggested path we should also refactor the previously merged PR with TYPE as static so as to normalize?

[MaciejBaj]

@nazarhussain the main issue here is Multisig is the only exception currently that need work arounds.

I agree! Eventually, we would aim for introducing the calculation of the fees based on the fee per byte -> not on other transaction properties as multisignature does. My suggestion would be to bring this assumption further and make multisignature overriding the validate step. You would be able then to implement a static property FEE and use it the same way you use TYPE.

[nazarhussain]

As far there is no extra interface introduced in BaseTransaction and there is only one validate to validate data then I am ok.

[pablitovicente]

As far there is no extra interface introduced in BaseTransaction and there is only one validate to validate data then I am ok.

Not having the method forces us to use an if() clause to handle the multisig case in validate() but I changed that based in feedback in favour of validateFee() your suggestion is to remove this method so we need to go back to the previous code I committed.

@shuse2 @nazarhussain @lsilvs thoughts ?

[lsilvs]

I still think having validateFee method is more flexible and easier for custom transactions developer

[pablitovicente]

As far there is no extra interface introduced in BaseTransaction and there is only one validate to validate data then I am ok.

Not having the method forces us to use an if() clause to handle the multisig case in validate() but I changed that based in feedback in favour of validateFee() your suggestion is to remove this method so we need to go back to the previous code I committed.

@shuse2 @nazarhussain @lsilvs thoughts ?

We discussed to continue conversation about the validation design for transactions in #3912 currently validateFee does NOT need to be implemented by developers of custom transactions - though if needed they can - for cases of static fees developers only need to define the class member FEE and it will be validated with no more effort.

[nazarhussain]

After discussion we agreed that in coming release we will add validate methods for individual attributes e.g. validateId, validateSignature Or we will remove validateFee and only keep the standard validate method.

Addressed with above discussions