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

Functional test tx type 6 - Subtask #678 - Closes #935 #953

Merged
merged 9 commits into from
Nov 17, 2017

Conversation

diego-G
Copy link

@diego-G diego-G commented Nov 13, 2017

Subtask #678
Closes #935

@diego-G diego-G force-pushed the 935-functional_test_tx_type_6 branch 2 times, most recently from 213fd24 to c9b3456 Compare November 13, 2017 11:18
@diego-G diego-G force-pushed the 935-functional_test_tx_type_6 branch from c9b3456 to bb31fbe Compare November 13, 2017 14:03
@diego-G diego-G force-pushed the 935-functional_test_tx_type_6 branch from 830f168 to ef68d1a Compare November 14, 2017 09:30
@diego-G diego-G force-pushed the 935-functional_test_tx_type_6 branch 5 times, most recently from c790155 to 9478e4d Compare November 14, 2017 16:14
@diego-G diego-G force-pushed the 935-functional_test_tx_type_6 branch 3 times, most recently from 081e3ce to fac39c6 Compare November 15, 2017 10:48
@diego-G diego-G force-pushed the 935-functional_test_tx_type_6 branch 4 times, most recently from 07b500b to 632a168 Compare November 16, 2017 11:13
@diego-G diego-G force-pushed the 935-functional_test_tx_type_6 branch from 632a168 to 5f04316 Compare November 16, 2017 14:51
@diego-G diego-G force-pushed the 935-functional_test_tx_type_6 branch from 5f04316 to 77ecc26 Compare November 17, 2017 09:15

return sendTransactionPromise(transaction).then(function (res) {
node.expect(res).to.have.property('status').to.equal(400);
node.expect(res).to.have.nested.property('body.message').to.equal('Invalid transaction body - Failed to validate transaction schema: Value -1 is less than minimum 0');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we allowing 0 lsk in transfers? The minimum should be 1, just like a type 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

This the minimum defined by the parent transaction schema, where child types might only have a fee.

Then for an actual transfer it is checked here in verify: https://github.com/LiskHQ/lisk/blob/development/logic/transfer.js#L62

Schema checks are designed to be less concrete to allow for inheritance pattern.

describe('transactions processing', function () {

it('invented dapp id should fail', function () {
var inventedDappId = '1';
Copy link
Contributor

@Isabello Isabello Nov 17, 2017

Choose a reason for hiding this comment

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

Maybe call this unknownDapp instead.

@@ -104,6 +104,9 @@ function invalidAssets (account, option, badTransactions) {
case 'dapp':
transaction = node.lisk.dapp.createDapp(account.password, null, node.guestbookDapp);
break;
case 'inTransfer':
transaction = node.lisk.transfer.createInTransfer(node.guestbookDapp.id, Date.now(), node.gAccount.password);
break;
Copy link
Contributor

@Isabello Isabello Nov 17, 2017

Choose a reason for hiding this comment

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

The naming here is odd. The other cases go node.lisk.NAME.

Here we are using the name of type 0 as the base name. Should it be called intransfer instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

The naming here is odd. The other cases go node.lisk.NAME.

I believe naming relates to how the lisk-js API is designed.

Here we are using the name of type 0 as the base name. Should it be called intransfer instead?

This is a correct strategy, using parent/base type name, which all cases are.

@karmacoma karmacoma dismissed Isabello’s stale review November 17, 2017 21:04

Thanks for the review

@karmacoma karmacoma merged commit 2d61ac5 into 1.0.0 Nov 17, 2017
@karmacoma karmacoma deleted the 935-functional_test_tx_type_6 branch November 17, 2017 21:04
shuse2 added a commit that referenced this pull request Apr 15, 2019
Implement P2P.getNetworkStatus() functionality - Closes #985 and #897, addresses #896 and #953
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants