[R4R] #2730 add tx search pagination related CLI/REST API parameter #2894

ackratos · 2018-11-24T04:03:30Z

closes #2730
closes #466

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

jackzampolin · 2018-11-28T21:38:09Z

cc @fedekunze per discussion: #2927

codecov · 2018-12-02T12:07:29Z

Codecov Report

Merging #2894 into develop will decrease coverage by 0.44%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop   #2894      +/-   ##
==========================================
- Coverage    55.65%   55.2%   -0.45%     
==========================================
  Files          134     134              
  Lines         9716    9526     -190     
==========================================
- Hits          5407    5259     -148     
+ Misses        3965    3936      -29     
+ Partials       344     331      -13

ackratos · 2018-12-02T14:20:37Z

manually tested

rest-server: https://127.0.0.1:1317/txs?tx.height=1148&page=1&limit=10

[  
   {  
      hash:"B252067C0A2A6917F6CF1B342BA32829EF32B962A7C21449E9CE63C59BF4B196",
      height:"1148",
      tx:{  
         type:"auth/StdTx",
         value:{  
            msg:[  
               {  
                  type:"cosmos-sdk/Send",
                  value:{  
                     inputs:[  
                        {  
                           address:"cosmos1h7k328ca9nt7qx2nltamp2wvsulszn8c98g9aq",
                           coins:[  
                              {  
                                 denom:"validatorToken",
                                 amount:"10"
                              }
                           ]
                        }
                     ],
                     outputs:[  
                        {  
                           address:"cosmos1xdm02sy7xnea9zcpt634uj9tufl7vq30eg3u8v",
                           coins:[  
                              {  
                                 denom:"validatorToken",
                                 amount:"10"
                              }
                           ]
                        }
                     ]
                  }
               }
            ],
            fee:{  
               amount:[  
                  {  
                     denom:"",
                     amount:"0"
                  }
               ],
               gas:"200000"
            },
            signatures:[  
               {  
                  pub_key:{  
                     type:"tendermint/PubKeySecp256k1",
                     value:"AwxauLUFhSTKJtSi87gF+Wt2nVSgNyTlqAJEHx8gjy5r"
                  },
                  signature:"NYPe/C5pUpWVDv/HXRi85I3qKpKWn1aZ3M1ywCgxf/whlR5ecHq/KYOxcaBoB5lBvUY5rcWzDydJSBOT/MWxpg==",
                  account_number:"0",
                  sequence:"1"
               }
            ],
            memo:""
         }
      },
      result:{  
         log:"Msg 0: ",
         gasWanted:"200000",
         gasUsed:"3390",
         tags:[  
            {  
               key:"YWN0aW9u",
               value:"c2VuZA=="
            },
            {  
               key:"c2VuZGVy",
               value:"Y29zbW9zMWg3azMyOGNhOW50N3F4Mm5sdGFtcDJ3dnN1bHN6bjhjOThnOWFx"
            },
            {  
               key:"cmVjaXBpZW50",
               value:"Y29zbW9zMXhkbTAyc3k3eG5lYTl6Y3B0NjM0dWo5dHVmbDd2cTMwZWczdTh2"
            }
         ]
      }
   }
]

command line: gaiacli query txs --tags 'tx.height:1148' --page 1 --limit 30 --chain-id testing

[  
   {  
      "hash":"B252067C0A2A6917F6CF1B342BA32829EF32B962A7C21449E9CE63C59BF4B196",
      "height":"1148",
      "tx":{  
         "type":"auth/StdTx",
         "value":{  
            "msg":[  
               {  
                  "type":"cosmos-sdk/Send",
                  "value":{  
                     "inputs":[  
                        {  
                           "address":"cosmos1h7k328ca9nt7qx2nltamp2wvsulszn8c98g9aq",
                           "coins":[  
                              {  
                                 "denom":"validatorToken",
                                 "amount":"10"
                              }
                           ]
                        }
                     ],
                     "outputs":[  
                        {  
                           "address":"cosmos1xdm02sy7xnea9zcpt634uj9tufl7vq30eg3u8v",
                           "coins":[  
                              {  
                                 "denom":"validatorToken",
                                 "amount":"10"
                              }
                           ]
                        }
                     ]
                  }
               }
            ],
            "fee":{  
               "amount":[  
                  {  
                     "denom":"",
                     "amount":"0"
                  }
               ],
               "gas":"200000"
            },
            "signatures":[  
               {  
                  "pub_key":{  
                     "type":"tendermint/PubKeySecp256k1",
                     "value":"AwxauLUFhSTKJtSi87gF+Wt2nVSgNyTlqAJEHx8gjy5r"
                  },
                  "signature":"NYPe/C5pUpWVDv/HXRi85I3qKpKWn1aZ3M1ywCgxf/whlR5ecHq/KYOxcaBoB5lBvUY5rcWzDydJSBOT/MWxpg==",
                  "account_number":"0",
                  "sequence":"1"
               }
            ],
            "memo":""
         }
      },
      "result":{  
         "log":"Msg 0: ",
         "gasWanted":"200000",
         "gasUsed":"3390",
         "tags":[  
            {  
               "key":"YWN0aW9u",
               "value":"c2VuZA=="
            },
            {  
               "key":"c2VuZGVy",
               "value":"Y29zbW9zMWg3azMyOGNhOW50N3F4Mm5sdGFtcDJ3dnN1bHN6bjhjOThnOWFx"
            },
            {  
               "key":"cmVjaXBpZW50",
               "value":"Y29zbW9zMXhkbTAyc3k3eG5lYTl6Y3B0NjM0dWo5dHVmbDd2cTMwZWczdTh2"
            }
         ]
      }
   }
]

ackratos · 2018-12-02T14:22:57Z

@ebuchman @cwgoes could you please review this PR?

cwgoes · 2018-12-03T11:14:28Z

cc @fedekunze I think you had thoughts on this.

fedekunze · 2018-12-03T11:28:04Z

@cwgoes @ackratos yes it was related to #2927, but pagination is still something we want regardless of the decision of that discussion

client/tx/search.go

fedekunze

Changes so far LGTM. Thanks @ackratos for fixing this ! A few additions are required before merging:

May I ask you to update the docs by adding the flags on gaiacli.md as well?
Can you add a small test case on cli_test.go too ?
Change perPage for limit according to standards (Source)

ackratos · 2018-12-04T14:41:22Z

Changes so far LGTM. Thanks @ackratos for fixing this ! A few additions are required before merging:

thanks for the suggestions. I am working on them

ackratos · 2018-12-09T15:47:58Z

Changes so far LGTM. Thanks @ackratos for fixing this ! A few additions are required before merging:

May I ask you to update the docs by adding the flags on gaiacli.md as well?

Can you add a small test case on cli_test.go too ?

Change perPage for limit according to standards (Source)

fixed, could you please review this PR again? Thanks

jackzampolin · 2018-12-10T19:07:36Z

@ackratos how do I query the second page? Can we add an offset (either # of txns or pages) here so that subsequent pages can be queried?

ackratos · 2018-12-11T02:04:43Z

@ackratos how do I query the second page? Can we add an offset (either # of txns or pages) here so that subsequent pages can be queried?

This PR added two (missing) parameters to cli and rest api: page and limit, setting page to 2 while keep limit with the same with page 1 can navigate you to page 2. (see the test cases I added)

This is pseudo-implementation because at tendermint tx searching side, it still query all txs meet the tags requirement only return the pagination data. This will help reduce network roundtrips but not saving query CPU time on node.

cwgoes · 2018-12-12T12:35:02Z

Needs merge conflict resolution, then I'd be glad to review (maybe @fedekunze could take a look again too).

ackratos · 2018-12-13T02:44:28Z

Needs merge conflict resolution, then I'd be glad to review (maybe @fedekunze could take a look again too).

thanks, done

fedekunze

sorry for the delay here. One final check and we should be good to go

fedekunze · 2019-01-04T13:43:50Z

baseapp/baseapp.go

@@ -86,7 +86,7 @@ var _ abci.Application = (*BaseApp)(nil)
 // NewBaseApp returns a reference to an initialized BaseApp.
 //
 // NOTE: The db is used to store the version number for now.
-// Accepts a user-defined txDecoder
+// Accepts a usemake install_examplesr-defined txDecoder


client/tx/search.go

alexanderbez

Looks good @ackratos, just a few minor bits of feedback :)

alexanderbez · 2019-01-04T14:15:08Z

baseapp/baseapp.go

@@ -86,7 +86,7 @@ var _ abci.Application = (*BaseApp)(nil)
 // NewBaseApp returns a reference to an initialized BaseApp.
 //
 // NOTE: The db is used to store the version number for now.
-// Accepts a user-defined txDecoder
+// Accepts a usemake install_examplesr-defined txDecoder


client/tx/search.go

jackzampolin · 2019-01-09T23:03:40Z

@ackratos Sorry for the long review process, but if you could address this last round of comments we can get this merged. Will you be able to do that this week?

ackratos · 2019-01-11T13:02:33Z

@ackratos Sorry for the long review process, but if you could address this last round of comments we can get this merged. Will you be able to do that this week?

yes, really sorry I was busy this week...
Will do that in next two days.

jackzampolin · 2019-01-11T15:53:03Z

@ackratos Totally understand! Thank you for working with us to get this right!

# Conflicts: # cmd/gaia/cli_test/cli_test.go

ackratos · 2019-01-13T03:50:05Z

@ackratos Totally understand! Thank you for working with us to get this right!

updated this PR, could you please review again?

alexanderbez

Thanks @ackratos -- I think there is still an issue with the use of constants. Also see my comments on parseHTTPArgs, otherwise, I think we can merge this PR afterwards 👍

alexanderbez · 2019-01-14T14:38:18Z

x/gov/client/utils/query.go

-	infos, err := tx.SearchTxs(cliCtx, cdc, tags)
+	// NOTE: SearchTxs is used to facilitate the txs query which does not currently
+	// support configurable pagination.
+	infos, err := tx.SearchTxs(cliCtx, cdc, tags, 1, 30)


Looks like these still need to be constants?

cmd/gaia/cli_test/test_helpers.go

client/tx/search.go

x/gov/client/utils/query.go

jackzampolin · 2019-01-14T19:07:46Z

@ackratos agree with @alexanderbez and @alessio. Thanks for updating the tests as well! Glad they are passing.

ackratos · 2019-01-15T15:00:32Z

@ackratos agree with @alexanderbez and @alessio. Thanks for updating the tests as well! Glad they are passing.

thanks! fixed again:)

fedekunze

LGTM, thanks @ackratos !

ackratos requested review from cwgoes, ebuchman and rigelrozanski as code owners November 24, 2018 04:03

alexanderbez added gaia-lite C:CLI labels Nov 24, 2018

ackratos force-pushed the pagination2730 branch from 94f253f to d0faa4f Compare December 2, 2018 12:07

ackratos force-pushed the pagination2730 branch from ac3c33f to 81b1219 Compare December 2, 2018 14:22

ackratos changed the title ~~[WIP] #2730 add tx search pagination related CLI/REST API parameter~~ [R4R] #2730 add tx search pagination related CLI/REST API parameter Dec 2, 2018

fedekunze reviewed Dec 3, 2018

View reviewed changes

client/tx/search.go Show resolved Hide resolved

fedekunze suggested changes Dec 3, 2018

View reviewed changes

ackratos requested a review from zramsay as a code owner December 4, 2018 15:32

ackratos changed the title ~~[R4R] #2730 add tx search pagination related CLI/REST API parameter~~ [WIP] #2730 add tx search pagination related CLI/REST API parameter Dec 4, 2018

ackratos force-pushed the pagination2730 branch from 3d779a9 to 884aab7 Compare December 9, 2018 15:41

ackratos changed the title ~~[WIP] #2730 add tx search pagination related CLI/REST API parameter~~ [R4R] #2730 add tx search pagination related CLI/REST API parameter Dec 9, 2018

ackratos force-pushed the pagination2730 branch from 884aab7 to 7933438 Compare December 9, 2018 16:05

ackratos force-pushed the pagination2730 branch from 7933438 to fa1c193 Compare December 13, 2018 02:35

ackratos force-pushed the pagination2730 branch from fa1c193 to 687a222 Compare December 15, 2018 01:31

fedekunze requested review from alexanderbez, jackzampolin and faboweb January 4, 2019 01:18

fedekunze reviewed Jan 4, 2019

View reviewed changes

alexanderbez suggested changes Jan 4, 2019

View reviewed changes

ackratos added 3 commits January 6, 2019 18:03

cosmos#2730 add tx search pagination related CLI/REST API parameter

06027f0

cosmos#2730 Fix tx.height functionality broken

f752ac2

cosmos#2730 refactor review comment

8ca0553

ackratos force-pushed the pagination2730 branch from 4172565 to 5898c6f Compare January 12, 2019 10:00

cosmos#2730 refactor review comment again

8777d8c

ackratos force-pushed the pagination2730 branch from 5898c6f to 8777d8c Compare January 12, 2019 10:19

Merge branch 'develop' into pagination2730

449f199

# Conflicts: # cmd/gaia/cli_test/cli_test.go

ackratos mentioned this pull request Jan 13, 2019

pagination response and default parameter should be refined tendermint/tendermint#3113

Closed

alexanderbez suggested changes Jan 14, 2019

View reviewed changes

alessio reviewed Jan 14, 2019

View reviewed changes

x/gov/client/utils/query.go Outdated Show resolved Hide resolved

cosmos#2730 bring query txs tests back

7e5c3b9

ackratos force-pushed the pagination2730 branch from c779e59 to 7e5c3b9 Compare January 15, 2019 14:59

alexanderbez approved these changes Jan 15, 2019

View reviewed changes

fedekunze approved these changes Jan 15, 2019

View reviewed changes

alessio approved these changes Jan 15, 2019

View reviewed changes

cwgoes merged commit 916ea85 into cosmos:develop Jan 15, 2019

ackratos mentioned this pull request Jan 16, 2019

cosmos api-server tx search pagination functionality is incomplete bnb-chain/node#262

Closed

ackratos deleted the pagination2730 branch April 29, 2019 08:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[R4R] #2730 add tx search pagination related CLI/REST API parameter #2894

[R4R] #2730 add tx search pagination related CLI/REST API parameter #2894

ackratos commented Nov 24, 2018 •

edited

Loading

jackzampolin commented Nov 28, 2018

codecov bot commented Dec 2, 2018 •

edited

Loading

ackratos commented Dec 2, 2018 •

edited

Loading

ackratos commented Dec 2, 2018

cwgoes commented Dec 3, 2018

fedekunze commented Dec 3, 2018

fedekunze left a comment

ackratos commented Dec 4, 2018

ackratos commented Dec 9, 2018

jackzampolin commented Dec 10, 2018

ackratos commented Dec 11, 2018

cwgoes commented Dec 12, 2018

ackratos commented Dec 13, 2018

fedekunze left a comment

fedekunze Jan 4, 2019

alexanderbez Jan 4, 2019

alexanderbez left a comment

alexanderbez Jan 4, 2019

jackzampolin commented Jan 9, 2019

ackratos commented Jan 11, 2019

jackzampolin commented Jan 11, 2019

ackratos commented Jan 13, 2019

alexanderbez left a comment

alexanderbez Jan 14, 2019

jackzampolin commented Jan 14, 2019

ackratos commented Jan 15, 2019

fedekunze left a comment

[R4R] #2730 add tx search pagination related CLI/REST API parameter #2894

[R4R] #2730 add tx search pagination related CLI/REST API parameter #2894

Conversation

ackratos commented Nov 24, 2018 • edited Loading

jackzampolin commented Nov 28, 2018

codecov bot commented Dec 2, 2018 • edited Loading

Codecov Report

ackratos commented Dec 2, 2018 • edited Loading

ackratos commented Dec 2, 2018

cwgoes commented Dec 3, 2018

fedekunze commented Dec 3, 2018

fedekunze left a comment

Choose a reason for hiding this comment

ackratos commented Dec 4, 2018

ackratos commented Dec 9, 2018

jackzampolin commented Dec 10, 2018

ackratos commented Dec 11, 2018

cwgoes commented Dec 12, 2018

ackratos commented Dec 13, 2018

fedekunze left a comment

Choose a reason for hiding this comment

fedekunze Jan 4, 2019

Choose a reason for hiding this comment

alexanderbez Jan 4, 2019

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez Jan 4, 2019

Choose a reason for hiding this comment

jackzampolin commented Jan 9, 2019

ackratos commented Jan 11, 2019

jackzampolin commented Jan 11, 2019

ackratos commented Jan 13, 2019

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez Jan 14, 2019

Choose a reason for hiding this comment

jackzampolin commented Jan 14, 2019

ackratos commented Jan 15, 2019

fedekunze left a comment

Choose a reason for hiding this comment

ackratos commented Nov 24, 2018 •

edited

Loading

codecov bot commented Dec 2, 2018 •

edited

Loading

ackratos commented Dec 2, 2018 •

edited

Loading