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

feat: update penumbra deps to v1 crates #31

Merged
merged 10 commits into from
Mar 6, 2025

Conversation

conorsch
Copy link
Contributor

Opening early to discuss implementation options. Refs #30.

Simply for consistency's sake, shouldn't affect the runtime logic.
Doing this for readability, so it's obvious which types are used
throughout the module.
This just pulls in the new v1 dependencies [0], doesn't actually
consume them yet.
Not finished yet, but I think this is the pattern we're forced to use.
We cannot round-trip via serde because the tendermint crates use
protobuf for serialization, which means versioning info is preserved,
prohibiting naive round-tripping across versions. So instead I'm
manually unpacking every field, reducing it to primitive types, and
re-encoding.
@conorsch conorsch force-pushed the 30-penumbra-deps-to-v1-crates branch from f402090 to 0b78f48 Compare February 28, 2025 00:43
@cronokirby
Copy link
Collaborator

I think the overall approach we have in the current version of the draft is good.

I think for Block, we unfortunately do need to store the latest version, I think. The alternative would be just storing raw bytes, but I think we do end up needing to parse out the height, so we need to parse out those bytes as some versioned tendermint type.

For other types, I think we can get away with something more independent:

  • For DeliverTx, we can just store the bytes of the transaction as a Vec.
  • For EndBlock we can store the height, as a u64 or i64 (of the top of my head I forget which one is more prevalent throughout the crate).
  • For Event, I think the common pattern would be something isomorphic to (String, Vec<(Vec<u8>, Vec<u8>, bool)>). (Eventually, it's assumed that these bytes are valid utf-8, but this assumption can turn its head when we actually convert it to the right versioned type).

Also, for juggling multiple versions, maybe what we should do is:

  • inside tendermint_compat, have:
  • mod v0o37
  • mod v0o40
  • the base module itself.

Then, each sub module would re-export the same types with the same name, importing from the same tendermint protos.

Then, it would be easy to see that if you have a function of type:

fn foo(x: tendermint_compat::DeliverTx) -> tendermint_compat::v0o37::DeliverTx

what's going on.

This scales to the inevitable time when we have more than two version as well, since we won't need to change anything.

Still fleshing out compat types for everything we need... will update
the trait as I go. It's going to take a lot of verbose translations
between 0.37 and 0.40 tendermint types to provide the compat we need.
@conorsch conorsch force-pushed the 30-penumbra-deps-to-v1-crates branch from 5e1fb75 to e48ffd5 Compare March 3, 2025 17:25
Rough-and-ready first pass, with copious unwraps. Just trying to get
full coverage on porting the type signatures, will need to circle back
and port the helper fns for converting Evidence, Misbehavior.
@conorsch
Copy link
Contributor Author

conorsch commented Mar 4, 2025

Pushed a rough draft of the compat types. Still doesn't compile, but the shape of the impls seems workable. There's too many unwraps for velocity: maybe it's worthwhile to update the Penumbra trait to return Results now, given the fallibility of the conversions.

@conorsch conorsch requested a review from cronokirby March 4, 2025 22:58
@conorsch
Copy link
Contributor Author

conorsch commented Mar 5, 2025

Beautiful! With the latest additions, I can run the archive integration tests to completion. This looks pretty well-shaped to me. As you mentioned in chat, there's likely some additional cleanup that's warranted here, but this looks overall sufficient to me that we can start working on #32 in parallel.

@cronokirby cronokirby marked this pull request as ready for review March 5, 2025 23:46
@conorsch conorsch changed the title WIP: update penumbra deps to v1 crates feat: update penumbra deps to v1 crates Mar 5, 2025
Using minor version bump because the CLI interface hasn't changed.
The Penumbra trait interface has changed drastically, but we're still in
in 0.x, so let's rock.
@conorsch conorsch merged commit 938d8ae into main Mar 6, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants