search

penumbra-zone / tower-abci

Tower-based ABCI interface.
MIT License
75 stars 24 forks source link

Rely on `tendermint-rs@v0.30.0` and bump version #25

Closed erwanor closed 1 year ago

hdevalence commented 1 year ago

I don't think we'll be able to move to 0.30 until we get a revised upstream that splits up the Request enums, since we need to handle the different ABCI versions separately.

Also the two versions may also encode ABCI messages differently on the wire, we should check if that change survived the 0.35 fork.

hdevalence commented 1 year ago

I think we can use the existing Request enums, since the only change is the addition of Process/PrepareProposal, which don't change the semantics of the other messages. 0.34 users can just ignore the messages, I guess ?

On the encoding front, the ABCI message encoding changed from 034 to 0.37. So we'll need to separate the API by tendermint version, and we should want to, because we'll expect that there will be bigger changes with 0.38. I'd propose re-rooting the entire API into submodules v034, v037, v038.

The encoding difference is here: https://github.com/penumbra-zone/tower-abci/blob/main/src/codec.rs#L7-L16

I think the easiest and cleanest thing would be to duplicate codec.rs into v034/codec.rs and v037/codec.rs and just change the definitions of the length encoding functions, and similarly with the server code. We'll want to end up with an external API like:

BoxError
split
v034::{Server, ServerBuilder}
v037::{Server, ServerBuilder}
hdevalence commented 1 year ago

We don't currently have any integration tests in the crate; I don't know if we should block this change on adding them, but we should probably at least manually test the kvstore example against cometbft 0.37, to make sure the encoding isn't broken.

erwanor commented 1 year ago

Definitely, that's in progress :)