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

Add back tx.minHeight and tx.maxHeight #10506

Closed
4 tasks
amaury1093 opened this issue Nov 8, 2021 · 3 comments
Closed
4 tasks

Add back tx.minHeight and tx.maxHeight #10506

amaury1093 opened this issue Nov 8, 2021 · 3 comments
Labels
C: gRPC Issues and PRs related to the gRPC service and HTTP gateway. T:feature-request

Comments

@amaury1093
Copy link
Contributor

Summary

When migrating from the legacy REST API to the new proto endpoints, we forgot to migrate the /tx?tx.{min,max}Height=... query param.

Should we add them to the proto endpoints?

ref: #10448

Problem Definition

Tendermint's tx indexer allows querying using tx.height=..., tx.height>=... and tx.height<=.... For the latter 2, the SDK added a REST helper tx.minHeight=... and tx.maxHeight=... in #5648. However, we didn't add them to the gRPC query handlers.

Proposal

Proposal 1

In the gRPC handler for querying txs (probably in the QueryTxsByEvents function), parse all events, and replace:

  • tx.minHeight=x by tx.height>=x
  • tx.maxHeight=x by tx.height<=x
    before sending the query to Tendermint.

Proposal 2

Stick to what Tendermint proposes and use >=, <= operators. Add docs in the REST API migration guide for minHeight, maxHeight.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@amaury1093 amaury1093 added C: gRPC Issues and PRs related to the gRPC service and HTTP gateway. Type: Feature labels Nov 8, 2021
@amaury1093
Copy link
Contributor Author

amaury1093 commented Nov 8, 2021

I'm personally in favor of Proposal 2. cc @BenWolfaardt @gagbo

@gagbo
Copy link
Contributor

gagbo commented Nov 8, 2021

Hello,

As I said in #10448 I'm not working specifically on Ledger cosmos integration as of now, but I think that proposal 2 is better if < and > are okay in the URL: it clearly shows that the values are inclusive limits for transaction height, and it's probably less technical debt as well :)

Regards,
Gerry

@tac0turtle
Copy link
Member

no one has asked for this, its probably fine we dont add it back

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: gRPC Issues and PRs related to the gRPC service and HTTP gateway. T:feature-request
Projects
None yet
Development

No branches or pull requests

3 participants