Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nextKey is not returned when calling GetTxsEvent #11538

Closed
4 tasks
swmd opened this issue Apr 4, 2022 · 19 comments · Fixed by #12261
Closed
4 tasks

nextKey is not returned when calling GetTxsEvent #11538

swmd opened this issue Apr 4, 2022 · 19 comments · Fixed by #12261
Assignees

Comments

@swmd
Copy link

swmd commented Apr 4, 2022

Summary of Bug

It appears that the pagination does not work properly when calling QueryClient.queryUnverified().
There are two issues I found in the response:

  1. nextKey is always an empty array.
  2. offset and limit keys don't work correctly. When I passed offset=4, limit=7, it just returned the result from the beginning. The response had the transactions of indexes 0-6.

Version

v0.44

Steps to Reproduce

Here's the code snippet I used to get the transactions history.

import { Tendermint34Client } from "@cosmjs/tendermint-rpc"
import { QueryClient } from "@cosmjs/stargate"
import { GetTxsEventRequest } from "cosmjs-types/cosmos/tx/v1beta1/service"

const client = QueryClient.withExtensions(
    await Tendermint34Client.connect({rpc_endpoint}),
)

client.queryUnverified("/cosmos.tx.v1beta1.Service/GetTxsEvent", GetTxsEventRequest.encode({
    events: [
        "message.action='delegate'",
	"delegate.validator='{validator_address}'",
    ],
    pagination: {
	countTotal: true,
	key: new Uint8Array(),
	offset: Long.fromNumber(0, true),
	limit: Long.fromNumber(5, true),
	reverse: false,
    },
    orderBy: OrderBy.ORDER_BY_ASC,
}).finish())

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez
Copy link
Contributor

So one thing to note about this gRPC query method: It's just a proxy to Tendermint which does the indexing, unlike native gRPC queries that go against the application state. Ref QueryTxsByEvents -> node.TxSearch.

What does this mean? This means there is now way, or at least no direct or accessible way to know NextKey. So that hopefully addresses your first point.

As for offset and page, that should be working but that does seem broken due to NewSearchTxsResult

@alexanderbez
Copy link
Contributor

alexanderbez commented Apr 5, 2022

So to summarize I think there are two issues:

1. Tendermint pagination is 1-based, SDK is 0, so we need to account for that in QueryTxsByEvents.
2. Fix QueryTxsByEvents to apply the offset.

@swmd
Copy link
Author

swmd commented Apr 5, 2022

@alexanderbez Could you hopefully provide a working code example with pagination?

@alexanderbez
Copy link
Contributor

@alexanderbez Could you hopefully provide a working code example with pagination?

Of state-based queries? If so, that happens via a generic auxiliary function FilteredPaginate.

@swmd
Copy link
Author

swmd commented Apr 6, 2022

@alexanderbez I couldn't find a useful doc or examples of FilteredPaginate.
My goal is to get the whole transactions history using any query. Can you help me to identify the best way to do it?

@alexanderbez
Copy link
Contributor

FilteredPaginate is used for state kvstore based queries, as I mentioned, it doesn't apply to querying for txs as that is indexed by Tendermint. However, you can see an example of FilteredPaginate used here.

My goal is to get the whole transactions history using any query.

Use GetTxsEvent as you already are. Just keep iterating over the pages.

@swmd
Copy link
Author

swmd commented Apr 14, 2022

@alexanderbez Can you provide a working example of the GetTxsEvent query? It was my original question.
The pagination doesn't work properly especially when I pass the offset.

@DeimonDB
Copy link

DeimonDB commented Jun 1, 2022

Hey @swmd this is late but I hope it would help.

I faced the same issue. It seems that currently the pagination is incorrectly implemented.

Let's say in total there are 10 data.

  • If we pass limit = 5 and offset = 0, it will return data [0..4]
  • if we pass limit = 5 and offset = 5, it will return data[5..9]
  • but if we pass limit = 5 and offset = 2:
    • it will return data[0..4] instead of returning data[2..6]
    • The pagination logic seems to try to figure out the "page" of the pagination by the given limit and offset.
    • I guess it determines page by this formula page = floor(offset / limit).
    • That's why when we give limit = 5 and offset = 2, it calculates page=0, and returns data for the first page

The pagination doesn't work properly especially when I pass the offset.

Therefore, when you use the offset, you need to make sure the offset is a multiple of the limit, i.e., if you give limit = 10, your possible offsets are 0, 10, 20, 30, etc

@alexanderbez
Copy link
Contributor

@facundomedica or @julienrbrt would you have some bandwidth to look into pagination?

@blushi
Copy link
Contributor

blushi commented Jun 8, 2022

The thing is that node.TxSearch from tendermint that is used internally by GetTxsEvent only accepts some page and perPage parameters: https://github.com/tendermint/tendermint/blob/v0.35.6/rpc/client/interface.go#L93, which basically makes it impossible by design to accept an offset that wouldn't be a multiple of the limit...

Probably GetTxsEvent shouldn't accept sdk query.PageRequest (nor return a query.PageResponse) and just use page and limit/perPage params like it's the case for the corresponding CLI query command QueryTxsByEventsCmd?

@alexanderbez
Copy link
Contributor

Probably GetTxsEvent shouldn't accept sdk query.PageRequest (nor return a query.PageResponse) and just use page and limit/perPage params like it's the case for the corresponding CLI query command QueryTxsByEventsCmd?

Yes, this seems reasonable to me. It should not accept and return those parameters 👍

@blushi
Copy link
Contributor

blushi commented Jun 9, 2022

Probably GetTxsEvent shouldn't accept sdk query.PageRequest (nor return a query.PageResponse) and just use page and limit/perPage params like it's the case for the corresponding CLI query command QueryTxsByEventsCmd?

Yes, this seems reasonable to me. It should not accept and return those parameters 👍

A few UX questions regarding this change:

  • should we just deprecate those pagination fields in the request/response for now?
  • node.TxSearch accepts a page params >= 1, which means you would always need to provide page=1 as part of the request, should we just default to page=1 if page is not provided as part of the request?

@alexanderbez
Copy link
Contributor

should we just deprecate those pagination fields in the request/response for now?

Yes, mark them as deprecated and remove their usage from the implementation.

node.TxSearch accepts a page params >= 1, which means you would always need to provide page=1 as part of the request, should we just default to page=1 if page is not provided as part of the request?

Yes, just default to 1 when omitted 👍

@mergify mergify bot closed this as completed in #12261 Jun 16, 2022
mergify bot pushed a commit that referenced this issue Jun 16, 2022
## Description

Closes: #11538
Also see thread after #11538 (comment)



---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
mergify bot pushed a commit that referenced this issue Jun 16, 2022
## Description

Closes: #11538
Also see thread after #11538 (comment)

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit c68fa3b)

# Conflicts:
#	api/cosmos/orm/query/v1alpha1/query.pulsar.go
tac0turtle added a commit that referenced this issue Jun 16, 2022
* fix: GetTxsEvent pagination (#12261)

## Description

Closes: #11538
Also see thread after #11538 (comment)

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit c68fa3b)

# Conflicts:
#	api/cosmos/orm/query/v1alpha1/query.pulsar.go

* delete file

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>
Co-authored-by: marbar3778 <marbar3778@yahoo.com>
@xloem
Copy link

xloem commented Jun 19, 2023

@alexanderbez
Copy link
Contributor

This has been posted and mentioned in many places -- SearchTxs is a CometBFT proxy RPC query, so it does NOT support the SDK's native pagination logic.

@xloem
Copy link

xloem commented Jun 19, 2023

It returns an empty set whether pagination parameters are passed or not. Is there a new example of a successful query?

@alexanderbez
Copy link
Contributor

The pagination params won't do anything. An example query could be message.module='bank'

@xloem
Copy link

xloem commented Jul 29, 2023

JeancarloBarrios pushed a commit to agoric-labs/cosmos-sdk that referenced this issue Sep 28, 2024
* fix: GetTxsEvent pagination (cosmos#12261)

## Description

Closes: cosmos#11538
Also see thread after cosmos#11538 (comment)

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit c68fa3b)

# Conflicts:
#	api/cosmos/orm/query/v1alpha1/query.pulsar.go

* delete file

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>
Co-authored-by: marbar3778 <marbar3778@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants