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

ibc: fix connection paths to use ibc-types #3118

Merged
merged 4 commits into from
Sep 29, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
344 changes: 186 additions & 158 deletions Cargo.lock

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion crates/bin/pcli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"] }
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, i remember it being annoying to figure out. it seems potentially worth it longer-term though.

Copy link
Member

Choose a reason for hiding this comment

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

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)


ibc-proto = { version = "0.33.0" }
ark-ff = { version = "0.4", default-features = false }
Expand Down
2 changes: 1 addition & 1 deletion crates/bin/pclientd/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ directories = "4.0.1"
tempfile = "3.3.0"
assert_cmd = "2.0"
base64 = "0.20"
ibc-types = "0.6.0"
ibc-types = { version = "0.6.3" }
ibc-proto = { version = "0.33.0", default-features = false, features = ["server"] }

[build-dependencies]
Expand Down
2 changes: 1 addition & 1 deletion crates/bin/pd/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ tendermint-config = "0.33.0"
tendermint-proto = "0.33.0"
tendermint = "0.33.0"
tendermint-light-client-verifier = "0.33.0"
ibc-types = "0.6.0"
ibc-types = { version = "0.6.3" }

ibc-proto = { version = "0.33.0", default-features = false, features = ["server"] }
prost = "0.11"
Expand Down
2 changes: 1 addition & 1 deletion crates/core/app/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ parking_lot = "0.12"
tendermint = "0.33.0"
tendermint-proto = "0.33.0"
tendermint-light-client-verifier = "0.33.0"
ibc-types = { version = "0.6.0", default-features = false }
ibc-types = { version = "0.6.3", default-features = false }
ibc-proto = { version = "0.33.0", default-features = false, features = ["server"] }

[dev-dependencies]
Expand Down
2 changes: 1 addition & 1 deletion crates/core/component/chain/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ penumbra-num = { path = "../../../core/num", default-features = false }
decaf377 = "0.5"

tendermint = "0.33.0"
ibc-types = { version = "0.6.0", default-features = false }
ibc-types = { version = "0.6.3", default-features = false }
ics23 = "0.10.1"

# Crates.io deps
Expand Down
2 changes: 1 addition & 1 deletion crates/core/component/ibc/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ penumbra-num = { path = "../../../core/num", default-features = false }
penumbra-keys = { path = "../../../core/keys", default-features = false }

# Penumbra dependencies
ibc-types = { version = "0.6.0", default-features = false }
ibc-types = { version = "0.6.3", default-features = false }
ibc-proto = { version = "0.33.0", default-features = false }

# Crates.io deps
Expand Down
44 changes: 30 additions & 14 deletions crates/core/component/ibc/src/component/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,14 @@ use async_trait::async_trait;
// TODO(erwan): remove in polish MERGEBLOCK
// use ibc_types::core::ics02_client::client_def::AnyClient;
// use ibc_types::core::ics02_client::client_def::ClientDef;
use ibc_types::core::{connection::ConnectionEnd, connection::ConnectionId};
use ibc_types::{
core::{
client::ClientId,
connection::ConnectionId,
connection::{ClientPaths, ConnectionEnd},
},
path::{ClientConnectionPath, ConnectionPath},
};
use penumbra_proto::{StateReadProto, StateWriteProto};
use penumbra_storage::{StateRead, StateWrite};

Expand All @@ -12,7 +19,13 @@ use super::{connection_counter::ConnectionCounter, state_key};
#[async_trait]
pub trait StateWriteExt: StateWrite {
fn put_connection_counter(&mut self, counter: ConnectionCounter) {
self.put(state_key::connections::counter().into(), counter);
self.put(state_key::counter().into(), counter);
}
fn put_client_connection(&mut self, client_id: &ClientId, paths: ClientPaths) {
self.put(
ClientConnectionPath::new(client_id).to_string(),
paths.clone(),
);
}

// puts a new connection into the state, updating the connections associated with the client,
Expand All @@ -23,13 +36,14 @@ pub trait StateWriteExt: StateWrite {
connection: ConnectionEnd,
) -> Result<()> {
self.put(
state_key::connections::by_connection_id(connection_id),
connection.clone(),
);
self.put(
state_key::connections::by_client_id(&connection.client_id, connection_id),
ConnectionPath::new(connection_id).to_string(),
connection.clone(),
);

let mut client_paths = self.get_client_connections(&connection.client_id).await?;
client_paths.paths.push(connection_id.clone());
self.put_client_connection(&connection.client_id, client_paths);

let counter = self
.get_connection_counter()
.await
Expand All @@ -41,13 +55,9 @@ pub trait StateWriteExt: StateWrite {

fn update_connection(&mut self, connection_id: &ConnectionId, connection: ConnectionEnd) {
self.put(
state_key::connections::by_connection_id(connection_id),
ConnectionPath::new(connection_id).to_string(),
connection.clone(),
);
self.put(
state_key::connections::by_client_id(&connection.client_id, connection_id),
connection,
);
}
}

Expand All @@ -56,14 +66,20 @@ impl<T: StateWrite> StateWriteExt for T {}
#[async_trait]
pub trait StateReadExt: StateRead {
async fn get_connection_counter(&self) -> Result<ConnectionCounter> {
self.get(state_key::connections::counter())
self.get(state_key::counter())
.await
.map(|counter| counter.unwrap_or(ConnectionCounter(0)))
}

async fn get_connection(&self, connection_id: &ConnectionId) -> Result<Option<ConnectionEnd>> {
self.get(&state_key::connections::by_connection_id(connection_id))
self.get(&ConnectionPath::new(connection_id).to_string())
.await
}

async fn get_client_connections(&self, client_id: &ClientId) -> Result<ClientPaths> {
self.get(&ClientConnectionPath::new(client_id).to_string())
.await
.map(|paths| paths.unwrap_or(ClientPaths { paths: vec![] }))
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,13 @@ use ibc_proto::ibc::core::connection::v1::{
};

use ibc_types::core::connection::{ConnectionId, IdentifiedConnectionEnd};
use ibc_types::path::ConnectionPath;
use ibc_types::DomainType;
use penumbra_chain::component::AppHashRead;
use prost::Message;
use std::str::FromStr;

use crate::component::{state_key, ConnectionStateReadExt};
use crate::component::ConnectionStateReadExt;

use super::IbcQuery;

Expand All @@ -33,7 +34,8 @@ impl ConnectionQuery for IbcQuery {

let (conn, proof) = snapshot
.get_with_proof_to_apphash(
state_key::connections::by_connection_id(connection_id)
ConnectionPath::new(connection_id)
.to_string()
.as_bytes()
.to_vec(),
)
Expand Down
39 changes: 6 additions & 33 deletions crates/core/component/ibc/src/component/state_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,44 +8,17 @@ pub fn ibc_params() -> &'static str {
"ibc/params"
}

// TODO (ava): move these to ibc-types eventually
// 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}")
Copy link
Member

Choose a reason for hiding this comment

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

if these aren't to be used by counterparties, and are component-internal, should we be prefixing them like the other component keys?

}

pub mod connections {
use ibc_types::core::client::ClientId;
use ibc_types::core::connection::ConnectionId;

use std::string::String;

// This is part of the ICS-3 spec but not exposed yet:
// https://github.com/cosmos/ibc/tree/main/spec/core/ics-003-connection-semantics
#[allow(dead_code)]
pub fn by_client_id_list(client_id: &ClientId) -> String {
format!("ibc/clients/{client_id}/connections/")
}

pub fn by_client_id(client_id: &ClientId, connection_id: &ConnectionId) -> String {
format!(
"ibc/clients/{}/connections/{}",
client_id,
connection_id.as_str()
)
}

pub fn by_connection_id(connection_id: &ConnectionId) -> String {
format!("ibc/connections/{}", connection_id.as_str())
}

pub fn counter() -> &'static str {
"ibc/ics03-connection/connection_counter"
}
pub fn counter() -> &'static str {
"ibc/connection_counter"
}

pub fn ics20_value_balance(channel_id: &ChannelId, asset_id: &asset::Id) -> String {
format!("ibc/ics20-value-balance/{channel_id}/{asset_id}")
}
2 changes: 1 addition & 1 deletion crates/core/transaction/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ penumbra-asset = { path = "../asset", default-features = false }
penumbra-keys = { path = "../keys", default-features = false }

# Git deps
ibc-types = { version = "0.6.0", default-features = false }
ibc-types = { version = "0.6.3", default-features = false }

# Crates.io deps
ibc-proto = { version = "0.33.0", default-features = false }
Expand Down
2 changes: 1 addition & 1 deletion crates/proto/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ anyhow = "1.0"
subtle-encoding = "0.5"
bech32 = "0.8"
penumbra-storage = { path = "../storage", optional = true }
ibc-types = { version = "0.6.0", features = ["std"] }
ibc-types = { version = "0.6.3", features = ["std"]}
pin-project = "1"
async-trait = "0.1.52"
async-stream = "0.2.0"
Expand Down
9 changes: 8 additions & 1 deletion crates/proto/src/protobuf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,13 +182,20 @@ extern crate ibc_types;
use ibc_proto::google::protobuf::Any;
use ibc_proto::ibc::core::channel::v1::Channel as RawChannel;
use ibc_proto::ibc::core::client::v1::Height as RawHeight;
use ibc_proto::ibc::core::connection::v1::ClientPaths as RawClientPaths;
use ibc_proto::ibc::core::connection::v1::ConnectionEnd as RawConnectionEnd;

use ibc_types::core::channel::ChannelEnd;
use ibc_types::core::client::Height;
use ibc_types::core::connection::ConnectionEnd;
use ibc_types::core::connection::{ClientPaths, ConnectionEnd};
use ibc_types::lightclients::tendermint::client_state::ClientState;

impl TypeUrl for ClientPaths {
const TYPE_URL: &'static str = "/ibc.core.connection.v1.ClientPaths";
}
impl DomainType for ClientPaths {
type Proto = RawClientPaths;
}
impl TypeUrl for ConnectionEnd {
const TYPE_URL: &'static str = "/ibc.core.connection.v1.ConnectionEnd";
}
Expand Down
2 changes: 1 addition & 1 deletion crates/view/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ penumbra-compact-block = { path = "../core/component/compact-block", default-fea
penumbra-app = { path = "../core/app" }
penumbra-transaction = { path = "../core/transaction" }

ibc-types = { version = "0.6.0", default-features = false }
ibc-types = { version = "0.6.3", default-features = false }

ark-std = { version = "0.4", default-features = false }
decaf377 = { version = "0.5", features = ["r1cs"] }
Expand Down