ibc: fix connection paths to use ibc-types #3118
Conversation
avahowell
temporarily deployed
to
smoke-test
GitHub Actions
Inactive
avahowell
had a problem deploying
to
smoke-test
GitHub Actions
Error
avahowell
force-pushed
the
fix-connection-paths
branch
from
318c968 to
ff862d7
Compare
avahowell
temporarily deployed
to
smoke-test
GitHub Actions
Inactive
avahowell
force-pushed
the
fix-connection-paths
branch
from
ff862d7 to
0fee6d4
Compare
avahowell
had a problem deploying
to
smoke-test
GitHub Actions
Error
| // these are internal helpers that are used by penumbra-ibc, but not part of the IBC spec (that is, | ||
| // counterparties don't expect to verify proofs about them) | ||
| pub fn client_processed_heights(client_id: &ClientId, height: &Height) -> String { | ||
| format!("ibc/clients/{client_id}/processedHeights/{height}") | ||
| format!("clients/{client_id}/processedHeights/{height}") | ||
| } | ||
| pub fn client_processed_times(client_id: &ClientId, height: &Height) -> String { | ||
| format!("ibc/clients/{client_id}/processedTimes/{height}") | ||
| format!("clients/{client_id}/processedTimes/{height}") |
There was a problem hiding this comment.
if these aren't to be used by counterparties, and are component-internal, should we be prefixing them like the other component keys?
| @@ -48,7 +48,7 @@ tendermint = { version = "0.33.0", features = ["rust-crypto"] } | |||
| jmt = "0.7" | |||
|
|
|||
| # External dependencies | |||
| ibc-types = { version = "0.6.0", default-features = false, features = ["std", "with_serde"]} | |||
| ibc-types = { version = "0.6.3", features = ["std", "with_serde"] } | |||
There was a problem hiding this comment.
side point, but, i think we should change the ibc-types crate to make the feature be called serde rather than with_serde, since that's a more conventional name
There was a problem hiding this comment.
@hdevalence Cargo gets confused when using colliding feature names that overlap with an optional dependency, even if you specify dep:serde, I never was able to find a way to bundle the serde = [dep:serde, dep:serde_derive] etc. maybe it's a skill issue.
There was a problem hiding this comment.
yeah, i remember it being annoying to figure out. it seems potentially worth it longer-term though.
There was a problem hiding this comment.
Cargo would not complain about it but the end result is that, somehow, serde_derive would not be available to the consumer crate (internal to the ibc-types graph)
avahowell
temporarily deployed
to
smoke-test
GitHub Actions
Inactive
IBC on main will be broken until this is merged; we still had some of the ibc standard paths defined in the
penumbra-ibccrate, which recently got changed. This broke compatibility, but these keys shouldn't even be in this crate. This PR removes the hardcodedconnectionkeys and replaces them with the ones exposed byibc-types, and also fixes theClientConnectionsmapping logic to be consistent with the spec and the ecosystem.This is blocking on release of a new version of ibc-types which includes a domain type we previously missed,
ClientPaths