Provide API endpoint for transactions of an account - Closes #1892 #1920

Tschakki · 2018-04-23T13:01:40Z

What was the problem?

The new API had no endpoint to collect all the transactions of a specific address at once.

How did I fix it?

Create new filter senderIdOrRecipientId for transactions.

How to test it?

Make API call: /api/transactions?senderIdOrRecipientId=<address>

Review checklist

The PR solves Provide API endpoint for transactions of an account #1892
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

nazarhussain · 2018-04-23T13:19:34Z

schema/swagger.yml

+      senderIdOrRecipientId:
+        type: string
+        format: address
+        example: 12668885769632475474L


@Tschakki senderIdOrRecipientId is just a request param. It would not be part of POST /transactions request.

nazarhussain · 2018-04-23T13:19:51Z

schema/swagger.yml

+      senderIdOrRecipientId:
+        type: string
+      	format: address
+      	example: 12668885769632475474L


@Tschakki senderIdOrRecipientId is just a request param. It would not be part of GET /transactions response.

lsilvs · 2018-04-23T14:04:54Z

modules/transactions.js

@@ -102,6 +102,8 @@ __private.list = function(filter, cb) {
 	const params = {};
 	const where = [];
 	const allowedFieldsMap = {
+		senderIdOrRecipientId:
+			't_senderId" IN (${senderId:csv}) OR t_recipientId" IN (${recipientId:csv})',


It seems to be missing double quote in the beginning of t_senderId and t_recipientId

nazarhussain · 2018-04-23T15:48:09Z

schema/swagger.yml

-  	maxLength: 22
+    name: senderIdOrRecipientId
+    in: query
+    description: Public key to query


@Tschakki Its the address not the public key. Seems you forget to update it.

lsilvs · 2018-04-24T07:57:00Z

modules/transactions.js

@@ -102,6 +102,8 @@ __private.list = function(filter, cb) {
 	const params = {};
 	const where = [];
 	const allowedFieldsMap = {
+		senderIdOrRecipientId:
+			'"t_senderId" IN (${senderId:csv}) OR "t_recipientId" IN (${recipientId:csv})',


(${senderId:csv}) and (${recipientId:csv}) should be (${senderIdOrRecipientId:csv})

haha yes, just found it out myself ^^

shuse2 · 2018-04-25T05:20:38Z

rather than having another query param senderIdOrRecipientId=<address>, which can be asked with other parameters, I think it would be better to add this feature on
/accounts/{address}/transactions.

nazarhussain · 2018-04-25T07:17:17Z

@shuse2 Yes probably it would be nice to have a nested resource for accounts. Iff it would, then it must support all filters already provided in transactions endpoint.

Current implementation is a the shortest and easy way to meet the requirements we have. After 1.0.0 release will probably be doing improvements in API specially providing feature to compose filters with custom modifiers. That will be the permanent fix for requirement.

nazarhussain · 2018-04-25T07:21:27Z

test/functional/http/get/transactions.js

+					.then(res => {
+						expectSwaggerParamError(res, 'senderIdOrRecipientId');
+					});
+			});


@Tschakki This case handles the case fo empty string. You should also add a case where you specify value but invalid value.

nazarhussain · 2018-04-25T07:26:43Z

test/functional/http/get/transactions.js

+							}
+						});
+						expect(sender).to.have.length(1);
+						expect(recipient).to.have.length(1);


The transactions you posted are deterministic and you know there are only two such transactions. So having more clear expectations will be nice.

expect(res.body.data).to.not.empty; expect(res.body.data[0].recipientId).to.be.eql(accountId); expect(res.body.data[1].senderId).to.be.eql(accountId);

Notice how clean three line expectations look like and easy to understand.

It is true there are only these 2 transactions, but are they returned always in the same chronological order inside of res.body.data?
If not these expectations could fail randomly.

Logically there should be one specific order whatever it would be. There can't be random order.

ok, will change it then!

nazarhussain · 2018-04-25T07:28:44Z

test/functional/http/get/transactions.js

+					.then(res => {
+						expectSwaggerParamError(res, 'senderIdOrRecipientId');
+					});
+			});


This definitely will fail, as the format of the param you specified in swagger is string not the array. So I don't think its a right test to be added.

nazarhussain · 2018-04-25T07:30:39Z

modules/transactions.js

@@ -102,6 +102,8 @@ __private.list = function(filter, cb) {
 	const params = {};
 	const where = [];
 	const allowedFieldsMap = {
+		senderIdOrRecipientId:
+			'"t_senderId" IN (${senderIdOrRecipientId:csv}) OR "t_recipientId" IN (${senderIdOrRecipientId:csv})',


One test case you should add is to check :csv formatter. Means send comma separated string value of this param. I believe it will fail because of address format we specified in swagger. So either you have to create new format for it or drop :csv here.

nazarhussain · 2018-04-25T12:40:03Z

modules/transactions.js

@@ -102,6 +102,8 @@ __private.list = function(filter, cb) {
 	const params = {};
 	const where = [];
 	const allowedFieldsMap = {
+		senderIdOrRecipientId:
+			'"t_senderId" IN (${senderIdOrRecipientId}) OR "t_recipientId" IN (${senderIdOrRecipientId})',


@Tschakki That needs to be changed, now as you are doing checking for only one variable then you should do with the direct assignment.

"t_senderId" = (${senderIdOrRecipientId}) OR "t_recipientId" = (${senderIdOrRecipientId})

Tschakki added 3 commits April 23, 2018 12:18

🌱 Add additional filter to api/transactions

5e7d62d

Change param name to senderIdOrRecipientId

f41418e

Add senderIdOrRecipientId to swagger

5931c44

MaciejBaj requested a review from nazarhussain April 23, 2018 13:09

MaciejBaj assigned Tschakki and MaciejBaj Apr 23, 2018

MaciejBaj added the 👁️ pending review label Apr 23, 2018

Add senderIdOrRecipientId property

e22c2fd

nazarhussain suggested changes Apr 23, 2018

View reviewed changes

MaciejBaj changed the base branch from 1.0.0 to 1.0.0-beta.7 April 23, 2018 13:39

MaciejBaj added 🏗️ in progress and removed 👁️ pending review labels Apr 23, 2018

MaciejBaj changed the title ~~New api endpoint for all tx of an account - Closes #1892~~ Provide API endpoint for transactions of an account - Closes #1892 Apr 23, 2018

lsilvs reviewed Apr 23, 2018

View reviewed changes

Tschakki added 2 commits April 23, 2018 16:54

Add missing quotes, remove wrong properties

04eccfc

Fix indents

4e14157

nazarhussain suggested changes Apr 23, 2018

View reviewed changes

lsilvs reviewed Apr 24, 2018

View reviewed changes

Tschakki added 2 commits April 24, 2018 10:38

Fix params in query

9c287d5

Add description for new new param

94eeb83

Tschakki added 👁️ pending review and removed 🏗️ in progress labels Apr 24, 2018

Tschakki requested a review from nazarhussain April 24, 2018 08:45

Tschakki added 🏗️ in progress and removed 👁️ pending review labels Apr 24, 2018

✅ Add functional tests for senderIdOrRecipientId

cb8f94b

Tschakki added 👁️ pending review and removed 🏗️ in progress labels Apr 24, 2018

nazarhussain suggested changes Apr 25, 2018

View reviewed changes

Tschakki added 2 commits April 25, 2018 14:12

Remove csv formatting for senderIdOrRecipientId

98e6324

Add test for invalid senderIdOrRecipientId, remove unnecessary test

b5cd640

Tschakki force-pushed the 1892-api_endpoint_for_all_tx_of_an_account branch from c2f3f89 to b5cd640 Compare April 25, 2018 12:35

nazarhussain suggested changes Apr 25, 2018

View reviewed changes

Make filter expect only single address each query

3baca6c

nazarhussain approved these changes Apr 25, 2018

View reviewed changes

Tschakki added ✅ ready and removed 👁️ pending review labels Apr 25, 2018

MaciejBaj merged commit 0219f8f into 1.0.0-beta.7 Apr 25, 2018

MaciejBaj deleted the 1892-api_endpoint_for_all_tx_of_an_account branch April 25, 2018 16:11

willclarktech mentioned this pull request Apr 26, 2018

Add API endpoint for transactions of an account LiskArchive/lisk-elements#648

Closed

lsilvs mentioned this pull request Apr 26, 2018

Improve All Transactions tab LiskArchive/lisk-explorer#591

Closed

slaweet mentioned this pull request May 15, 2018

Update to Lisk Core 1.0.0 API - Closes #341 LiskArchive/lisk-nano#1032

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide API endpoint for transactions of an account - Closes #1892 #1920

Provide API endpoint for transactions of an account - Closes #1892 #1920

Tschakki commented Apr 23, 2018 •

edited

Loading

nazarhussain Apr 23, 2018

nazarhussain Apr 23, 2018

lsilvs Apr 23, 2018

nazarhussain Apr 23, 2018

lsilvs Apr 24, 2018

Tschakki Apr 24, 2018

shuse2 commented Apr 25, 2018

nazarhussain commented Apr 25, 2018

nazarhussain Apr 25, 2018

nazarhussain Apr 25, 2018

Tschakki Apr 25, 2018 •

edited

Loading

nazarhussain Apr 25, 2018

Tschakki Apr 25, 2018

nazarhussain Apr 25, 2018

nazarhussain Apr 25, 2018

nazarhussain Apr 25, 2018

Provide API endpoint for transactions of an account - Closes #1892 #1920

Provide API endpoint for transactions of an account - Closes #1892 #1920

Conversation

Tschakki commented Apr 23, 2018 • edited Loading

What was the problem?

How did I fix it?

How to test it?

Review checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuse2 commented Apr 25, 2018

nazarhussain commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tschakki Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tschakki commented Apr 23, 2018 •

edited

Loading

Tschakki Apr 25, 2018 •

edited

Loading