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 gRPC endpoints for querying txs, TxsByEvents, tx/broadcast #7355

Closed
anilcse opened this issue Sep 21, 2020 · 17 comments · Fixed by #7852
Closed

Add gRPC endpoints for querying txs, TxsByEvents, tx/broadcast #7355

anilcse opened this issue Sep 21, 2020 · 17 comments · Fixed by #7852
Assignees
Labels
C: gRPC Issues and PRs related to the gRPC service and HTTP gateway.

Comments

@anilcse
Copy link
Collaborator

anilcse commented Sep 21, 2020

  • Currently there are only legacy REST endpoints for querying TxsByEvents and for querying TxByHash. Having gRPC methods/endpoints would be handy for applications that uses gRPC alone.

cc @aaronc @amaurymartiny @alexanderbez

@aaronc
Copy link
Member

aaronc commented Sep 21, 2020

It seems like clients could use tendermint JSON RPC directly without needing gRPC, and that gRPC would just be a very thin wrapper over tendermint. What benefits would this bring?

/cc @marbar3778

@alexanderbez
Copy link
Contributor

The idea I think is that it's one less service to have to interface with if the client doesn't actually need Tendermint's JSON RPC.

@aaronc
Copy link
Member

aaronc commented Sep 21, 2020

Yeah that's reasonable. We should also wrap broadcast (and maybe some other tendermint methods) if we do this.

1 similar comment
@aaronc
Copy link
Member

aaronc commented Sep 21, 2020

Yeah that's reasonable. We should also wrap broadcast (and maybe some other tendermint methods) if we do this.

@anilcse
Copy link
Collaborator Author

anilcse commented Sep 21, 2020

The idea I think is that it's one less service to have to interface with if the client doesn't actually need Tendermint's JSON RPC.

Correct @alexanderbez , and the less services to interface for clients, the better UX.

Yeah that's reasonable. We should also wrap broadcast (and maybe some other tendermint methods) if we do this.

+1

@anilcse anilcse added API C: gRPC Issues and PRs related to the gRPC service and HTTP gateway. labels Sep 26, 2020
@sahith-narahari
Copy link
Contributor

@anilcse @clevinson this should be addressed for 0.40 right, or is there a change of plan I missed

@amaury1093
Copy link
Contributor

In reading Tendermint's v1.0 roadmap, I saw they were evaluating using gRPC instead of their own RPC layer. If that happens, I guess this issue is not relevant anymore?

The 2 gRPC servers will be on 2 different ports, if clients want everything on one single port, maybe there's a workaround to combine them under one port?

@anilcse
Copy link
Collaborator Author

anilcse commented Oct 8, 2020

We can address this after 0.40 as well, not a blocker. @amaurymartiny as this would be a small rework even after updating the TM side changes, I suggest to address this now. Wdyt?

@aaronc
Copy link
Member

aaronc commented Oct 8, 2020

In reading Tendermint's v1.0 roadmap, I saw they were evaluating using gRPC instead of their own RPC layer. If that happens, I guess this issue is not relevant anymore?

Exactly.

But it's small enough (hopefully) that it's not a problem to add it in the meantime.

Not a blocker for 0.40.

@clevinson clevinson added this to the v0.40.1 milestone Oct 14, 2020
@aaronc
Copy link
Member

aaronc commented Oct 23, 2020

I suggest we backport at least tx by hash to Stargate. It shouldn't be that hard

@clevinson
Copy link
Contributor

@amaurymartiny's PR currently only covers GetTxByHash, we should decide if the following other endpoints are priority for stargate:

  • broadcast Tx REST endpoint (currently proto Tx's are only able to be received by tendermint RPC endpoints)
  • TxByEvents REST endpoint

Thoughts @aaronc @anilcse ?

@aaronc
Copy link
Member

aaronc commented Oct 28, 2020

TxByEvents is higher priority IMHO. Broadcast I'm not sure we should even offer that actually, it should take TxRaw if so.

@aaronc
Copy link
Member

aaronc commented Oct 28, 2020

Maybe broadcast is useful to just use a single set of generated types...

@anilcse
Copy link
Collaborator Author

anilcse commented Oct 28, 2020

@amaurymartiny's PR currently only covers GetTxByHash, we should decide if the following other endpoints are priority for stargate:

  • broadcast Tx REST endpoint (currently proto Tx's are only able to be received by tendermint RPC endpoints)
  • TxByEvents REST endpoint

Thoughts @aaronc @anilcse ?

I suggest to include both if it's not too much involved for broadcast. TxByEvents is a priority anyway, better to address it in the same PR

@clevinson
Copy link
Contributor

The only argument that I can see for broadcast, is for some users who still want to have a future supported REST endpoint for broadcasting transactions in Stargate. Unless tendermint RPC is basically REST over a specific port (sorry for not knowing this...)?

@aaronc
Copy link
Member

aaronc commented Oct 29, 2020

Tendermint is not gRPC protocol (yet). Adding this stuff to gRPC allows clients to talk to a single endpoint (eventually).

@alexanderbez
Copy link
Contributor

#7355 (comment)

Having the ability for clients to broadcast and query from a single gRPC/gateway endpoint would be ideal.

@clevinson clevinson changed the title Add gRPC endpoints for querying txs, TxsByEvents Add gRPC endpoints for querying txs, TxsByEvents, tx/broadcast endpoint Oct 29, 2020
@clevinson clevinson changed the title Add gRPC endpoints for querying txs, TxsByEvents, tx/broadcast endpoint Add gRPC endpoints for querying txs, TxsByEvents, tx/broadcast Oct 29, 2020
@mergify mergify bot closed this as completed in #7852 Dec 2, 2020
@aaronc aaronc removed the in progress label Dec 2, 2020
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants