broadcast_tx via grpc

[ebuchman]

It's nice to have a broadcast_tx available over grpc so app devs can get started with nothing but a grpc lib for both tmsp and sending txs.

That said, grpc is a bit of a headache. It breaks the semantics of BroadcastTxCommit because it doesn't allow you to return a result and an error (eg. CheckTx result but error because it doesn't get committed). It's also not playing nice with glide for unknown reasons.

[tomtruitt]

tendermint/abci@3329868 did you fix this issue with returning results?

[ebuchman]

not sure what you're asking here

[tomtruitt]

In this issue you said;

"grpc is a bit of a headache. It breaks the semantics of BroadcastTxCommit because it doesn't allow you to return a result and an error (eg. CheckTx result but error because it doesn't get committed)."

In the commit I referenced I see addition of:
return types.Result{}
To grpc_client.

So I thought maybe you came up with a fix or a new way to do it. And since we are building on grpc I've been curious about the implications of this issue for us

[ebuchman]

Oh no. That commit is just fixing a compile error. Does not address the problems mentioned in the opening comment

[mdyring]

Why not just populate ResponseBroadcastTx with the values from the step that fails (be it CheckTx or AppendTx).

I don't think from a BroadcastTx user perspective it matters much if it failed in CheckTx or AppendTx, and the response structures are compatible

message ResponseBroadcastTx{
uint64 code = 1; // TODO: import tmsp ...
bytes data = 2;
string log = 3;
}

message ResponseAppendTx{
CodeType code = 1;
bytes data = 2;
string log = 3;
}

message ResponseCheckTx{
CodeType code = 1;
bytes data = 2;
string log = 3;
}

[ebuchman]

So you don't think it matters if the caller knows whether it failed in CheckTx or AppendTx?

It could pass CheckTx then fail in AppendTx, or it could just fail in CheckTx, and caller won't know the difference. I don't think that's right because failing in AppendTx should have some kind of effect on the state, someone ought to pay for it, but we haven't really worked out what that might look like.

[ebuchman]

This is now a breaking change since it changes the response struct for BroadcastTxCommit, but I think that's for the best.

[jaekwon]

StartGRPCClient might eventually need to be refactored out so the client doesn't require importing tendermint/rpc/core and thus the whole kitchen sink. But otherwise LGTM. We can merge this in.

[tomtruitt]

@Jae just a reminder the TMSP bindings need to be upgraded for compatibility with GRPC 1.0

[tomtruitt]

Sorry i guess that is a separate issue

[tomtruitt]

i just now realized this isn't grpc_fix branch.. is that branch still relevant? I'm going with no right? Does this branch just need to be rebased?

[ebuchman]

this branch just needs to be rebased and then it should get merged into develop