Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

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

Merged
merged 16 commits into from
Jul 4, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 1 addition & 12 deletions elements/lisk-transactions/src/0_transfer_transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export const transferAssetFormatSchema = {
export class TransferTransaction extends BaseTransaction {
public readonly asset: TransferAsset;
public static TYPE = 0;
public static FEE = TRANSFER_FEE.toString();

public constructor(rawTransaction: unknown) {
super(rawTransaction);
Expand Down Expand Up @@ -100,18 +101,6 @@ export class TransferTransaction extends BaseTransaction {
);
}

if (!this.fee.eq(TRANSFER_FEE)) {
errors.push(
new TransactionError(
`Fee must be equal to ${TRANSFER_FEE}`,
this.id,
'.fee',
this.fee.toString(),
TRANSFER_FEE,
),
);
}

if (!this.recipientId) {
errors.push(
new TransactionError(
Expand Down
13 changes: 1 addition & 12 deletions elements/lisk-transactions/src/1_second_signature_transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export const secondSignatureAssetFormatSchema = {
export class SecondSignatureTransaction extends BaseTransaction {
public readonly asset: SecondSignatureAsset;
public static TYPE = 1;
public static FEE = SIGNATURE_FEE.toString();

public constructor(rawTransaction: unknown) {
super(rawTransaction);
Expand Down Expand Up @@ -112,18 +113,6 @@ export class SecondSignatureTransaction extends BaseTransaction {
);
}

if (!this.fee.eq(SIGNATURE_FEE)) {
errors.push(
new TransactionError(
`Fee must be equal to ${SIGNATURE_FEE}`,
this.id,
'.fee',
this.fee.toString(),
SIGNATURE_FEE,
),
);
}

if (this.recipientId) {
errors.push(
new TransactionError(
Expand Down
13 changes: 1 addition & 12 deletions elements/lisk-transactions/src/2_delegate_transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ export class DelegateTransaction extends BaseTransaction {
public readonly asset: DelegateAsset;
public readonly containsUniqueData: boolean;
public static TYPE = 2;
public static FEE = DELEGATE_FEE.toString();

public constructor(rawTransaction: unknown) {
super(rawTransaction);
Expand Down Expand Up @@ -117,18 +118,6 @@ export class DelegateTransaction extends BaseTransaction {
);
}

if (!this.fee.eq(DELEGATE_FEE)) {
errors.push(
new TransactionError(
`Fee must be equal to ${DELEGATE_FEE}`,
this.id,
'.fee',
this.fee.toString(),
DELEGATE_FEE,
),
);
}

if (this.recipientId) {
errors.push(
new TransactionError(
Expand Down
13 changes: 1 addition & 12 deletions elements/lisk-transactions/src/3_vote_transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ export class VoteTransaction extends BaseTransaction {
public readonly containsUniqueData: boolean;
public readonly asset: VoteAsset;
public static TYPE = 3;
public static FEE = VOTE_FEE.toString();

public constructor(rawTransaction: unknown) {
super(rawTransaction);
Expand Down Expand Up @@ -173,18 +174,6 @@ export class VoteTransaction extends BaseTransaction {
);
}

if (!this.fee.eq(VOTE_FEE)) {
errors.push(
new TransactionError(
`Fee must be equal to ${VOTE_FEE}`,
this.id,
'.fee',
this.fee.toString(),
VOTE_FEE,
),
);
}

return errors;
}

Expand Down
32 changes: 17 additions & 15 deletions elements/lisk-transactions/src/4_multisignature_transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ export interface MultiSignatureAsset {
export class MultisignatureTransaction extends BaseTransaction {
public readonly asset: MultiSignatureAsset;
public static TYPE = 4;
public static FEE = MULTISIGNATURE_FEE.toString();
protected _multisignatureStatus: MultisignatureStatus =
MultisignatureStatus.PENDING;

Expand Down Expand Up @@ -171,21 +172,6 @@ export class MultisignatureTransaction extends BaseTransaction {
return errors;
}

const expectedFee = new BigNum(MULTISIGNATURE_FEE).mul(
this.asset.multisignature.keysgroup.length + 1,
);
if (!this.fee.eq(expectedFee)) {
errors.push(
new TransactionError(
`Fee must be equal to ${expectedFee.toString()}`,
this.id,
'.fee',
this.fee.toString(),
expectedFee.toString(),
),
);
}

if (
this.asset.multisignature.min > this.asset.multisignature.keysgroup.length
) {
Expand Down Expand Up @@ -224,6 +210,22 @@ export class MultisignatureTransaction extends BaseTransaction {
return errors;
}

public validateFee(): TransactionError | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this function is meant to be public, used only within the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Protected methods can not be overridden and we need to handle the special case of multi-signatures that's why public seems the only possible solution in this case but if you alternatives please share!

const expectedFee = new BigNum(MultisignatureTransaction.FEE).mul(
this.asset.multisignature.keysgroup.length + 1,
);

return !this.fee.eq(expectedFee)
? new TransactionError(
`Fee must be equal to ${expectedFee.toString()}`,
this.id,
'.fee',
this.fee.toString(),
expectedFee.toString(),
)
: undefined;
}

public processMultisignatures(_: StateStore): TransactionResponse {
const transactionBytes = this.getBasicBytes();

Expand Down
12 changes: 1 addition & 11 deletions elements/lisk-transactions/src/5_dapp_transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ export class DappTransaction extends BaseTransaction {
public readonly containsUniqueData: boolean;
public readonly asset: DappAsset;
public static TYPE = 5;
public static FEE = DAPP_FEE.toString();

public constructor(rawTransaction: unknown) {
super(rawTransaction);
Expand Down Expand Up @@ -230,17 +231,6 @@ export class DappTransaction extends BaseTransaction {
);
}

if (!this.fee.eq(DAPP_FEE)) {
errors.push(
new TransactionError(
`Fee must be equal to ${DAPP_FEE}`,
this.id,
'.fee',
this.fee.toString(),
DAPP_FEE,
),
);
}
const validLinkSuffix = ['.zip'];

if (errors.length > 0) {
Expand Down
19 changes: 19 additions & 0 deletions elements/lisk-transactions/src/base_transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ export abstract class BaseTransaction {
public receivedAt?: Date;

public static TYPE: number;
public static FEE: string;

protected _id?: string;
protected _senderId?: string;
Expand Down Expand Up @@ -300,9 +301,27 @@ export abstract class BaseTransaction {
);
}

const feeError = this.validateFee();

if (feeError) {
errors.push(feeError);
}

return createResponse(this.id, errors);
}

public validateFee(): TransactionError | undefined {
return !this.fee.eq((this.constructor as typeof BaseTransaction).FEE)
? new TransactionError(
`Invalid fee`,
this.id,
'.fee',
this.fee.toString(),
(this.constructor as typeof BaseTransaction).FEE.toString(),
)
: undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is undefined needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MaciejBaj because if there's no error it returns undefined and I saw this was being used for example validateTransactionId so I tried to use the same "pattern"

}

public verifyAgainstOtherTransactions(
transactions: ReadonlyArray<TransactionJSON>,
): TransactionResponse {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,9 @@ export const validateAddress = (address: string): boolean => {

export const isGreaterThanZero = (amount: BigNum) => amount.cmp(0) > 0;

export const isGreaterThanOrEqualToZero = (amount: BigNum) =>
amount.cmp(0) >= 0;

export const isGreaterThanMaxTransactionAmount = (amount: BigNum) =>
amount.cmp(MAX_TRANSACTION_AMOUNT) > 0;

Expand All @@ -165,7 +168,7 @@ export const isValidTransferData = (data: string): boolean =>

export const validateFee = (data: string) =>
isNumberString(data) &&
isGreaterThanZero(new BigNum(data)) &&
pablitovicente marked this conversation as resolved.
Show resolved Hide resolved
isGreaterThanOrEqualToZero(new BigNum(data)) &&
!isGreaterThanMaxTransactionAmount(new BigNum(data));

export const isValidInteger = (num: unknown) =>
Expand Down
39 changes: 39 additions & 0 deletions elements/lisk-transactions/test/base_transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,13 @@ describe('Base transaction class', () => {
);
});

it('should call validateFee', async () => {
sandbox.spy(validTestTransaction, 'validateFee');
validTestTransaction.validate();

expect(validTestTransaction.validateFee).to.be.called;
});

it('should return a failed transaction response with invalid signature', async () => {
const invalidSignature = defaultTransaction.signature.replace('1', '0');
const invalidSignatureTransaction = {
Expand Down Expand Up @@ -579,6 +586,38 @@ describe('Base transaction class', () => {
});
});

describe('#validateFee', () => {
it('should return undefined if fee is valid', async () => {
const validFee = validTestTransaction.validateFee();

expect(validFee).to.be.undefined;
});

it('should return transactionError if fee is invalid', async () => {
const invalidTransaction = {
type: 0,
amount: '00001',
fee: '0000',
recipientId: '',
senderPublicKey: '11111111',
senderId: '11111111',
timestamp: 79289378,
asset: {},
signature: '1111111111',
id: '1',
};
const invalidTestTransaction = new TestTransaction(
invalidTransaction as any,
);

const feeError = invalidTestTransaction.validateFee();

expect(feeError)
.to.be.instanceof(TransactionError)
.and.to.have.property('message', 'Invalid fee');
});
});

describe('#verifyAgainstOtherTransactions', () => {
it('should return a transaction response', async () => {
const otherTransactions = [defaultTransaction, defaultTransaction];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@
import { BaseTransaction } from '../../src/base_transaction';
import { TransactionJSON } from '../../src/transaction_types';
import { TransactionError } from '../../src/errors';
import { TRANSFER_FEE } from '../../src/constants';

export class TestTransaction extends BaseTransaction {
public static TYPE = 0;
public static FEE = TRANSFER_FEE.toString();

public assetToJSON(): object {
return {};
Expand Down
23 changes: 21 additions & 2 deletions elements/lisk-transactions/test/utils/validation/validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
validateFee,
isGreaterThanMaxTransactionAmount,
isGreaterThanZero,
isGreaterThanOrEqualToZero,
isGreaterThanMaxTransactionId,
isNumberString,
isValidInteger,
Expand Down Expand Up @@ -289,8 +290,8 @@ describe('validation', () => {
});

describe('#validateFee', () => {
it('should return false is amount is 0', () => {
return expect(validateFee('0')).to.be.false;
it('should return true if amount is 0', () => {
return expect(validateFee('0')).to.be.true;
});

it('should return true when amount is a number greater than 0 and less than maximum transaction amount', () => {
Expand All @@ -310,6 +311,24 @@ describe('validation', () => {
});
});

describe('#isGreaterThanOrEqualToZero', () => {
lsilvs marked this conversation as resolved.
Show resolved Hide resolved
it('should return true when amount is 0', () => {
return expect(isGreaterThanOrEqualToZero(new BigNum('0'))).to.be.true;
});

it('should return true when amount is greater than 0', () => {
return expect(
isGreaterThanOrEqualToZero(
new BigNum('9223372036854775808987234289782357'),
),
).to.be.true;
});

it('should return false when amount is less than 0', () => {
return expect(isGreaterThanOrEqualToZero(new BigNum('-1'))).to.be.false;
});
});

describe('#isGreaterThanMaxTransactionAmount', () => {
it('should return false when amount is less than maximum transaction amount', () => {
return expect(
Expand Down
4 changes: 2 additions & 2 deletions elements/lisk-transactions/test/utils/validation/validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,8 @@ describe('validator', () => {
return expect(validate({ target: '000000100' })).to.be.true;
});

it('should validate to false when amount is 0', () => {
return expect(validate({ target: '0' })).to.be.false;
it('should validate to true when amount is 0', () => {
return expect(validate({ target: '0' })).to.be.true;
});

it('should validate to false when number greater than maximum is provided', () => {
Expand Down
4 changes: 4 additions & 0 deletions framework/test/mocha/integration/matcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ class CustomTransationClass extends BaseTransaction {
return 7;
}

static get FEE() {
return TransferTransaction.FEE;
}

assetToJSON() {
return this.asset;
}
Expand Down