rpc: add sort_order in tx_search #4253

princesinha19 · 2019-12-16T10:02:51Z

I have added order_by which can be "asc" or "desc" (should be in string format) in the tx_search RPC method.

Fixes: #3333

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

rpc/core/tx.go

rpc/client/httpclient.go

rpc/client/localclient.go

lite2/rpc/client.go

lite2/proxy/routes.go

rpc/swagger/swagger.yaml

rpc/client/httpclient.go

lite2/proxy/routes.go

lite2/rpc/client.go

rpc/client/httpclient.go

rpc/client/interface.go

rpc/client/localclient.go

rpc/core/tx.go

rpc/swagger/swagger.yaml

… into tx-search

melekes

this needs an actual test case
rpc: add sort_order in tx_search #4253 (comment)

CHANGELOG_PENDING.md

rpc/client/rpc_test.go

rpc/swagger/swagger.yaml

… into tx-search deb http://apt.postgresql.org/pub/repos/apt/ bionic-pgdg main

… into tx-search

princesinha19 · 2019-12-19T10:11:57Z

@melekes I have added test cases and removed unnecessary sorting from kv-store. Please have a look.

melekes

Thanks for the changes. I have a few more comments though...

rpc/client/rpc_test.go

melekes · 2020-01-02T11:03:09Z

rpc/core/tx.go

+			}
+		}
+
+		if orderBy == "desc" {


switch orderBy { case "desc": ... case "asc": ... }

@melekes actually this will create issue because by default we wanted to keep "asc" order but, if we will use switch case then if someone will not specify the correct order then the transaction will be unordered.

melekes · 2020-01-02T11:04:19Z

rpc/core/tx.go

@@ -101,5 +103,29 @@ func TxSearch(ctx *rpctypes.Context, query string, prove bool, page, perPage int
 		}
 	}

+	if len(apiResults) > 1 {
+		for _, item := range apiResults {


can you explain what this loop does? it looks like it just appends apiResults to itself.

Yes, you are correct @melekes. But, it will remove the nil items present in the array. So, we will only get the txs which are not null. If we will allow nil items then, at the time of order by it will not able to sort correctly and it will fail because, the one item from (a,b) is nil.

melekes · 2020-01-02T11:04:56Z

state/txindex/kv/kv_test.go

@@ -221,60 +221,6 @@ func TestTxSearchOneTxWithMultipleSameTagsButDifferentValues(t *testing.T) {
 	assert.Equal(t, []*types.TxResult{txResult}, results)
 }

-func TestTxSearchMultipleTxs(t *testing.T) {


why remove??

Because we have removed #4253 (comment)

RIght. But we still want to test "indexed fourth (to test we don't include txs with similar events)" use-case. See #2908

So should I take back #4253 (comment)?

melekes · 2020-01-02T11:23:37Z

rpc/client/rpc_test.go

+		// Transaction should be in the ascending order
+		result, err = c.TxSearch(fmt.Sprintf("tx.height >= 1"), true, 1, 30, "asc")
+		require.Nil(t, err, "%+v", err)
+		for k := 0; k < len(result.Txs)-1; k++ {


here we only have one transaction, so this does not actually test what we want

Actually there are multiple. Throughout the test, many tx has been performed.

melekes · 2020-01-02T11:24:15Z

state/txindex/kv/kv_test.go

@@ -221,60 +221,6 @@ func TestTxSearchOneTxWithMultipleSameTagsButDifferentValues(t *testing.T) {
 	assert.Equal(t, []*types.TxResult{txResult}, results)
 }

-func TestTxSearchMultipleTxs(t *testing.T) {


RIght. But we still want to test "indexed fourth (to test we don't include txs with similar events)" use-case. See #2908

#4253 (review)

melekes · 2020-01-27T12:22:15Z

Closing in favor of #4342.

added sort_order

a5c6cba

princesinha19 requested review from ebuchman, melekes and tessr as code owners December 16, 2019 10:02

golangcibot reviewed Dec 16, 2019

View reviewed changes

rpc/core/tx.go Outdated Show resolved Hide resolved

rpc/client/httpclient.go Outdated Show resolved Hide resolved

rpc/client/localclient.go Outdated Show resolved Hide resolved

lite2/rpc/client.go Outdated Show resolved Hide resolved

lite2/proxy/routes.go Outdated Show resolved Hide resolved

solved line-length

7b81849

tac0turtle reviewed Dec 16, 2019

View reviewed changes

rpc/swagger/swagger.yaml Outdated Show resolved Hide resolved

melekes reviewed Dec 16, 2019

View reviewed changes

rpc/client/httpclient.go Outdated Show resolved Hide resolved

tac0turtle added T:enhancement Type: Enhancement C:rpc Component: JSON RPC, gRPC labels Dec 16, 2019

tessr changed the title ~~rpc: Added sort_order in tx_search~~ rpc: add sort_order in tx_search Dec 16, 2019

princesinha19 and others added 3 commits December 16, 2019 21:33

changes sort_order to order_by

cd2abd8

tx_search changes

224650c

Merge branch 'master' into tx-search

20877a5

melekes suggested changes Dec 17, 2019

View reviewed changes

princesinha19 added 3 commits December 17, 2019 16:00

changed sortOrder to orderBy

347e129

Merge branch 'tx-search' of https://github.com/princesinha19/tendermint…

6b885e9

… into tx-search

Merge branch 'master' into tx-search

59f510f

princesinha19 requested review from tac0turtle and melekes December 17, 2019 13:09

tac0turtle added feature Feature work that definitely changes system behavior and removed T:enhancement Type: Enhancement labels Dec 17, 2019

melekes suggested changes Dec 18, 2019

View reviewed changes

melekes reviewed Dec 18, 2019

View reviewed changes

rpc/swagger/swagger.yaml Outdated Show resolved Hide resolved

princesinha19 added 6 commits December 18, 2019 14:30

removed comment

797a8b7

Merge branch 'tx-search' of https://github.com/princesinha19/tendermint…

824bdac

… into tx-search deb http://apt.postgresql.org/pub/repos/apt/ bionic-pgdg main

Merge branch 'master' into tx-search

cf21e5b

Merge branch 'master' into tx-search

afd9e66

added test cases

0c2cff2

Merge branch 'tx-search' of https://github.com/princesinha19/tendermint…

a25d903

… into tx-search

princesinha19 requested a review from melekes December 19, 2019 10:11

Merge branch 'master' into tx-search

284faa8

princesinha19 and others added 2 commits December 20, 2019 08:07

Merge branch 'master' into tx-search

7320c9b

Merge branch 'master' into tx-search

106d05d

melekes suggested changes Jan 2, 2020

View reviewed changes

melekes reviewed Jan 2, 2020

View reviewed changes

Merge branch 'master' into tx-search

1d3e215

sorawit mentioned this pull request Jan 12, 2020

Add temporary implementation to make /d3n/txs/latest perform correctly bandprotocol/bandchain#191

Closed

melekes added a commit that referenced this pull request Jan 27, 2020

fixes after my own review

5430c1f

#4253 (review)

melekes closed this Jan 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: add sort_order in tx_search #4253

rpc: add sort_order in tx_search #4253

princesinha19 commented Dec 16, 2019 •

edited

melekes left a comment

princesinha19 commented Dec 19, 2019

melekes left a comment

melekes Jan 2, 2020 •

edited

princesinha19 Jan 2, 2020

melekes Jan 2, 2020

princesinha19 Jan 2, 2020

melekes Jan 2, 2020

princesinha19 Jan 2, 2020

melekes Jan 2, 2020

princesinha19 Jan 2, 2020

melekes Jan 2, 2020

princesinha19 Jan 2, 2020

melekes Jan 2, 2020

melekes commented Jan 27, 2020

rpc: add sort_order in tx_search #4253

rpc: add sort_order in tx_search #4253

Conversation

princesinha19 commented Dec 16, 2019 • edited

melekes left a comment

Choose a reason for hiding this comment

princesinha19 commented Dec 19, 2019

melekes left a comment

Choose a reason for hiding this comment

melekes Jan 2, 2020 • edited

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

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

melekes commented Jan 27, 2020

princesinha19 commented Dec 16, 2019 •

edited

melekes Jan 2, 2020 •

edited