[R4R] #2730 add tx search pagination related CLI/REST API parameter by cryptom-dev · Pull Request #2894 · cosmos/cosmos-sdk

cryptom-dev · 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

cryptom-dev · 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"
            }
         ]
      }
   }
]

cryptom-dev · 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)

cryptom-dev · 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

cryptom-dev · 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?

cryptom-dev · 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).

cryptom-dev · 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

client/tx/search.go

alexanderbez

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

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

baseapp/baseapp.go

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?

cryptom-dev · 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

cryptom-dev · 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

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.

cryptom-dev · 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 !

cryptom-dev 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

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

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

cryptom-dev 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 Outdated Show resolved Hide resolved

fedekunze suggested changes Dec 3, 2018

View reviewed changes

cryptom-dev requested a review from zramsay as a code owner December 4, 2018 15:32

cryptom-dev 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

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

cryptom-dev 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

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

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

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

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

fedekunze reviewed Jan 4, 2019

View reviewed changes

alexanderbez suggested changes Jan 4, 2019

View reviewed changes

cryptom-dev 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

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

cosmos#2730 refactor review comment again

8777d8c

cryptom-dev 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

cryptom-dev 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

cryptom-dev 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

cryptom-dev mentioned this pull request Jan 16, 2019

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

Closed

cryptom-dev deleted the pagination2730 branch April 29, 2019 08:31

Conversation

cryptom-dev commented Nov 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jackzampolin commented Nov 28, 2018

Uh oh!

codecov bot commented Dec 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cryptom-dev commented Dec 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cryptom-dev commented Dec 2, 2018

Uh oh!

cwgoes commented Dec 3, 2018

Uh oh!

fedekunze commented Dec 3, 2018

Uh oh!

Uh oh!

fedekunze left a comment

Choose a reason for hiding this comment

Uh oh!

cryptom-dev commented Dec 4, 2018

Uh oh!

cryptom-dev commented Dec 9, 2018

Uh oh!

jackzampolin commented Dec 10, 2018

Uh oh!

cryptom-dev commented Dec 11, 2018

Uh oh!

cwgoes commented Dec 12, 2018

Uh oh!

cryptom-dev commented Dec 13, 2018

Uh oh!

fedekunze left a comment

Choose a reason for hiding this comment

Uh oh!

fedekunze Jan 4, 2019

Choose a reason for hiding this comment

Uh oh!

alexanderbez Jan 4, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alexanderbez left a comment

Choose a reason for hiding this comment

Uh oh!

alexanderbez Jan 4, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jackzampolin commented Jan 9, 2019

Uh oh!

cryptom-dev commented Jan 11, 2019

Uh oh!

jackzampolin commented Jan 11, 2019

Uh oh!

cryptom-dev commented Jan 13, 2019

Uh oh!

alexanderbez left a comment

Choose a reason for hiding this comment

Uh oh!

alexanderbez Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jackzampolin commented Jan 14, 2019

Uh oh!

cryptom-dev commented Jan 15, 2019

cryptom-dev commented Nov 24, 2018 •

edited

Loading

codecov bot commented Dec 2, 2018 •

edited

Loading

cryptom-dev commented Dec 2, 2018 •

edited

Loading