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

update to ibc-proto 0.46, tendermint 0.37 #89

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dynst
Copy link

@dynst dynst commented Jun 11, 2024

This involved adding a lot of unfortunate .unwrap() calls that could probably be rethinked later.

Adding them isn't any worse than the status quo, I think, it just moves where the panic happens further up the stack. The tendermint 0.36 API change just made the edge case more obvious.

@conorsch
Copy link
Contributor

This is helpful, and I appreciate your submitting it, @dynst. However, we should be careful about merge timing: we're currently pinning 0.34.0 throughout the Penumbra monorepo:

https://github.com/penumbra-zone/penumbra/blob/v0.77.2/Cargo.toml#L215-L219

and we should maintain version parity on either side. A quick check on bumping the monorepo versions shows that we'd have to make fixes throughout the IBC handlers, so I'm inclined to defer this change for now.

Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes simply to block premature merge: let's coordinate version bumps here with matching PRs in the monorepo. Also, the unwraps throughout may have liveness implications that we should think through carefully.

@dynst
Copy link
Author

dynst commented Jun 14, 2024

The .unwrap() calls only potentially fail on a block from a chain that ran CometBFT 0.34, because EventAttribute::V034 predates the changes that ensured key and value were both valid strings. That isn't a concern for penumbra at least which is launching with cometbft 0.37.

Penumbra is using ibc-types 0.12, and 0.13.0 is the latest version, so I figured they were on different release cadences and not tightly synced, is there a reason Penumbra shouldn't just keep depending on 0.12 like it does now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

2 participants