Merge abci-rs in tendermint-rs repository #489
Conversation
abci-rs/Cargo.toml
Outdated
| async-std = { version = "1.6", optional = true } | ||
| async-trait = "0.1" | ||
| bytes = "0.5" | ||
| integer-encoding = { version = "1.1", optional = true } |
There was a problem hiding this comment.
there's a small bug in the released integer-encoding: dermesser/integer-encoding-rs#10
not sure of the impact. but it's a fairly small crate / code needed here, so could be possibly copied / vendored in here directly?
There was a problem hiding this comment.
From tendermint/rust-abci#134 it seems we're not reading the first byte? Was it just being ignored or? Trying to understand the state of things and how this bug was surfaced in the first place
There was a problem hiding this comment.
It seems it's being read (https://github.com/tendermint/rust-abci/blob/develop/src/codec.rs#L26 -- or @yihuang why it's not?) and under normal circumstances, TM would be writing terminated varints as lengths ?
There was a problem hiding this comment.
what does terminated mean here? do we mean like a terminal byte in the varint encoding, ie. where the MSB is not set? I think that first "length of length" is always just one byte here so should always be terminal, and should probably also always be small (like 1-3, maybe max 10, since theres a limit on how big a msg tendermint would send).
There was a problem hiding this comment.
terminated varint -- the last byte has MSB set.
the code in rust-abci seems to match what's in Golang:
https://github.com/tendermint/tendermint/blob/master/abci/types/messages.go#L54
Done. |
|
@xla it'll also be needed that @zramsay transfers https://crates.io/crates/abci ownership |
Good point, maybe @ebuchman can follow up on this. |
|
This PR may need some reworking around the generated proto files after: #508 lands. |
|
#508 is merged -- @marbar3778 I guess the natural thing here will be to remove abci-rs's types + build.rs and just use the "tendermint-proto" crate? |
|
since this will be in the same repo, the dependency can be specified with |
Pushed the changes. Also included changes for tendermint |
| while let Ok(request) = decode(&mut stream).await { | ||
| match request { | ||
| Some(request) => { | ||
| let response = inner.process(request).await; | ||
|
|
||
| if let Err(err) = encode(response, &mut stream).await { | ||
| error!(message = "Error while writing to stream", %err, ?peer_addr); | ||
| } | ||
| } | ||
| None => debug!(message = "Received empty request", ?peer_addr), | ||
| } |
There was a problem hiding this comment.
awaiting may need some re-work to follow the async CheckTx/DeliverTx semantics when it could potentially be processing another request while response to the previous one is being written out (in order): #29 (comment)
|
@devashishdxt, after much discussion the team decided to go with #794. We still, however, think that |
Merging
abci-rscrate intendermint-rsrepository as suggested by @tomtau here. Also, continuing discussion on #29.