-
Notifications
You must be signed in to change notification settings - Fork 454
Add optional data field with transaction - Closes #26 #470
Add optional data field with transaction - Closes #26 #470
Conversation
…Khan/lisk into 26-add_data_field_with_transaction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good @SargeKhan 👍
I have left some comments on what we could do to improve on this proposal. Main points being length of data and column type, plus some further unit tests.
We would also need to add some functional API tests for PUT /api/transactions
and POST /peer/transactions
. The two ways in which we accept transactions over the API. First being for end users, second being for peer to peer (using pre-signed transactions).
Overall though, very good indeed.
|
||
BEGIN; | ||
|
||
ALTER TABLE "trs" ADD COLUMN "data" VARCHAR(16); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think BYTEA
column type would be more efficient on disk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Makes sense.
logic/transaction.js
Outdated
for (i = 0; i < 16; i++) { | ||
bb.writeByte(dataBuffer[i] || 0); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should increase the buffer size to at least 64. Typical use case would be for saving hashes. This is obviously debatable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, will do.
schema/transactions.js
Outdated
data: { | ||
type: 'string', | ||
minLength: 0, | ||
maxLength: 16 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check format of string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of format would we check against? Isn't data optional, and can hold any type of string value?
test/unit/logic/transaction.js
Outdated
@@ -0,0 +1,466 @@ | |||
'use strict'; /*jslint mocha:true, expr:true */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are no longer using jslint
, but rather eslint
.
test/unit/logic/transaction.js
Outdated
it('should throw an error with no param', function () { | ||
expect(transaction.objectNormalize).to.throw(); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we should have some examples of schema checks. Please see: https://github.com/LiskHQ/lisk/blob/development/logic/transaction.js#L789. With regard to the new data attribute that is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. will do.
test/unit/logic/transaction.js
Outdated
expect(firstCalculation.equals(secondCalculation)).to.be.ok; | ||
}); | ||
|
||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we should check byte calculation works with and without the optional data property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Will add test case for it.
test/unit/logic/transaction.js
Outdated
it('should return transaction with optional data field', function () { | ||
var trsData = getValidTransactionData(); | ||
expect(transaction.create(trsData).data).to.be.a('string'); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should test creation with and without the optional data property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Will add test case for it.
test/unit/logic/transaction.js
Outdated
it('should throw an error with no param', function () { | ||
expect(transaction.dbSave).to.throw(); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we should test returned value includes optional data property. See: https://github.com/LiskHQ/lisk/blob/development/logic/transaction.js#L658
We also need to test: https://github.com/LiskHQ/lisk/blob/development/logic/transaction.js#L642
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
test/unit/logic/transaction.js
Outdated
it('should throw an error with no param', function () { | ||
expect(transaction.dbRead).to.throw(); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we should also test returned value includes optional data property. See: https://github.com/LiskHQ/lisk/blob/development/logic/transaction.js#L817
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I will do it.
test/unit/logic/transaction.js
Outdated
expect(trsBytesFromLogic.equals(trsBytesFromLiskJs)).to.be.ok; | ||
}); | ||
|
||
it('should return same result of getBytes using /logic/transaction and list-js package (with data field)', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karmacoma This test case will fail since getBytes doesn't use data field we've added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes understood. We will add support for it in lisk-js
@Tosch110.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling: list-js
logic/transaction.js
Outdated
if (trs.data) { | ||
var dataBuffer = new Buffer(trs.data, 'utf8'); | ||
for (i = 0; i < 64; i++) { | ||
bb.writeByte(dataBuffer[i] || 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karmacoma what do you think about ||
expression in this statement? Should I change this loop to something like:
for (i = 0; i < dataBuffer.length; i++) { bb.writeByte(dataBuffer[i]); }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think this would be better.
test/api/peer.transactions.main.js
Outdated
@@ -77,6 +77,18 @@ describe('POST /peer/transactions', function () { | |||
}); | |||
}); | |||
|
|||
it('using data field should be ok', function (done) { | |||
var account = node.randomAccount(); | |||
var transaction = node.lisk.transaction.createTransaction(account.address, 1, node.gAccount.password); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karmacoma this test case will fail, because here createTransaction()
from lisk-js package doesn't take into account data field we have added in the backed. So, signature and id will not be generated correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok fair point. We will work on adding support for this in lisk-js
: LiskArchive/lisk-elements#52
Closes #26