solana-ibc: introduce ChannelIdx type for channel identification

solana/solana-ibc/programs/solana-ibc/src/execution_context.rs

@@ -200,32 +200,13 @@ impl ExecutionContext for IbcStorage<'_, '_> {
         commitment: PacketCommitment,
     ) -> Result {
         msg!("store_packet_commitment({}, {:?})", path, commitment);
-        let mut store = self.borrow_mut();
-        let trie_key = TrieKey::from(path);
-        // PacketCommitment is always 32-byte long.
-        let commitment = <&CryptoHash>::try_from(commitment.as_ref()).unwrap();
-        store.provable.set(&trie_key, commitment).unwrap();
-        record_packet_sequence(
-            &mut store.private.packet_commitment_sequence_sets,
-            &path.port_id,
-            &path.channel_id,
-            path.sequence,
-        );
-        Ok(())
+        // Note: PacketCommitment is always 32-byte long.
+        self.store_commitment(TrieKey::try_from(path)?, commitment.as_ref())
     }
 
     fn delete_packet_commitment(&mut self, path: &CommitmentPath) -> Result {
         msg!("delete_packet_commitment({})", path);
-        let mut store = self.borrow_mut();
-        let trie_key = TrieKey::from(path);
-        store.provable.del(&trie_key).unwrap();
-        delete_packet_sequence(
-            &mut store.private.packet_commitment_sequence_sets,
-            &path.port_id,
-            &path.channel_id,
-            path.sequence,
-        );
-        Ok(())
+        self.delete_commitment(TrieKey::try_from(path)?)
     }
 
     fn store_packet_receipt(
@@ -234,10 +215,7 @@ impl ExecutionContext for IbcStorage<'_, '_> {
         Receipt::Ok: Receipt,
     ) -> Result {
         msg!("store_packet_receipt({}, Ok)", path);
-        let mut store = self.borrow_mut();
-        let trie_key = TrieKey::from(path);
-        store.provable.set_and_seal(&trie_key, &CryptoHash::DEFAULT).unwrap();
-        Ok(())
+        self.store_commitment(TrieKey::try_from(path)?, &[0; 32][..])
     }
 
     fn store_packet_acknowledgement(
@@ -246,32 +224,13 @@ impl ExecutionContext for IbcStorage<'_, '_> {
         commitment: AcknowledgementCommitment,
     ) -> Result {
         msg!("store_packet_acknowledgement({}, {:?})", path, commitment);
-        let mut store = self.borrow_mut();
-        let trie_key = TrieKey::from(path);
-        // AcknowledgementCommitment is always 32-byte long.
-        let commitment = <&CryptoHash>::try_from(commitment.as_ref()).unwrap();
-        store.provable.set(&trie_key, commitment).unwrap();
-        record_packet_sequence(
-            &mut store.private.packet_acknowledgement_sequence_sets,
-            &path.port_id,
-            &path.channel_id,
-            path.sequence,
-        );
-        Ok(())
+        // Note: AcknowledgementCommitment is always 32-byte long.
+        self.store_commitment(TrieKey::try_from(path)?, commitment.as_ref())
     }
 
     fn delete_packet_acknowledgement(&mut self, path: &AckPath) -> Result {
         msg!("delete_packet_acknowledgement({})", path);
-        let mut store = self.borrow_mut();
-        let trie_key = TrieKey::from(path);
-        store.provable.del(&trie_key).unwrap();
-        delete_packet_sequence(
-            &mut store.private.packet_acknowledgement_sequence_sets,
-            &path.port_id,
-            &path.channel_id,
-            path.sequence,
-        );
-        Ok(())
+        self.delete_commitment(TrieKey::try_from(path)?)
     }
 
     fn store_channel(
@@ -280,10 +239,12 @@ impl ExecutionContext for IbcStorage<'_, '_> {
         channel_end: ChannelEnd,
     ) -> Result {
         msg!("store_channel({}, {:?})", path, channel_end);
+        let port_channel = ids::PortChannelPK::try_from(&path.0, &path.1)?;
+        let trie_key = TrieKey::for_channel_end(&port_channel);
         self.borrow_mut().store_serialised_proof(
             |private| &mut private.channel_ends,
-            (path.0.to_string(), path.1.to_string()),
-            &TrieKey::from(path),
+            port_channel,
+            &trie_key,
             &channel_end,
         )
     }
@@ -294,7 +255,7 @@ impl ExecutionContext for IbcStorage<'_, '_> {
         seq: Sequence,
     ) -> Result {
         msg!("store_next_sequence_send: path: {}, seq: {}", path, seq);
-        self.borrow_mut().store_next_sequence(
+        self.store_next_sequence(
             path.into(),
             storage::SequenceTripleIdx::Send,
             seq,
@@ -307,7 +268,7 @@ impl ExecutionContext for IbcStorage<'_, '_> {
         seq: Sequence,
     ) -> Result {
         msg!("store_next_sequence_recv: path: {}, seq: {}", path, seq);
-        self.borrow_mut().store_next_sequence(
+        self.store_next_sequence(
             path.into(),
             storage::SequenceTripleIdx::Recv,
             seq,
@@ -320,7 +281,7 @@ impl ExecutionContext for IbcStorage<'_, '_> {
         seq: Sequence,
     ) -> Result {
         msg!("store_next_sequence_ack: path: {}, seq: {}", path, seq);
-        self.borrow_mut().store_next_sequence(
+        self.store_next_sequence(
             path.into(),
             storage::SequenceTripleIdx::Ack,
             seq,
@@ -349,25 +310,36 @@ impl ExecutionContext for IbcStorage<'_, '_> {
     fn get_client_execution_context(&mut self) -> &mut Self::E { self }
 }
 
-impl storage::IbcStorageInner<'_, '_> {
+impl storage::IbcStorage<'_, '_> {
+    fn store_commitment(&mut self, key: TrieKey, commitment: &[u8]) -> Result {
+        // Caller promises that commitment is always 32 bytes.
+        let commitment = <&CryptoHash>::try_from(commitment).unwrap();
+        self.borrow_mut().provable.set(&key, commitment).map_err(error)
+    }
+
+    fn delete_commitment(&mut self, key: TrieKey) -> Result {
+        self.borrow_mut().provable.del(&key).map(|_| ()).map_err(error)
+    }
+
     fn store_next_sequence(
         &mut self,
         path: storage::trie_key::SequencePath<'_>,
         index: storage::SequenceTripleIdx,
         seq: Sequence,
     ) -> Result {
-        let trie = &mut self.provable;
-        let next_seq = &mut self.private.next_sequence;
-        let map_key = (path.port_id.to_string(), path.channel_id.to_string());
-        let triple = next_seq.entry(map_key).or_default();
-        triple.set(index, seq);
-
-        let trie_key = TrieKey::from(path);
-        trie.set(&trie_key, &triple.to_hash()).unwrap();
-
-        Ok(())
+        let key = ids::PortChannelPK::try_from(path.port_id, path.channel_id)?;
+        let trie_key = TrieKey::for_next_sequence(&key);
+        let mut store = self.borrow_mut();
+        let hash = {
+            let triple = store.private.next_sequence.entry(key).or_default();
+            triple.set(index, seq);
+            triple.to_hash()
+        };
+        store.provable.set(&trie_key, &hash).map_err(error)
     }
+}
 
+impl storage::IbcStorageInner<'_, '_> {
     /// Serialises `value` and stores it in private storage along with its
     /// commitment in provable storage.
     ///
@@ -391,41 +363,6 @@ impl storage::IbcStorageInner<'_, '_> {
     }
 }
 
-type SequencesMap =
-    BTreeMap<(storage::InnerPortId, storage::InnerChannelId), Vec<Sequence>>;
-
-/// Adds sequence to given per-channel set.
-fn record_packet_sequence(
-    map: &mut SequencesMap,
-    port_id: &ibc::core::ics24_host::identifier::PortId,
-    channel_id: &ibc::core::ics24_host::identifier::ChannelId,
-    sequence: Sequence,
-) {
-    let key = (port_id.to_string(), channel_id.to_string());
-    map.entry(key).or_default().push(sequence);
-}
-
-/// Removes sequence from given per-channel set.
-fn delete_packet_sequence(
-    map: &mut SequencesMap,
-    port_id: &ibc::core::ics24_host::identifier::PortId,
-    channel_id: &ibc::core::ics24_host::identifier::ChannelId,
-    sequence: Sequence,
-) {
-    use alloc::collections::btree_map::Entry;
-
-    let key = (port_id.to_string(), channel_id.to_string());
-    if let Entry::Occupied(mut entry) = map.entry(key) {
-        let set = entry.get_mut();
-        if let Some(pos) = set.iter().position(|s| *s == sequence) {
-            set.swap_remove(pos);
-            if set.is_empty() {
-                entry.remove();
-            }
-        }
-    }
-}
-
 fn error(description: impl ToString) -> ContextError {
     ClientError::Other { description: description.to_string() }.into()
 }

solana/solana-ibc/programs/solana-ibc/src/storage.rs

@@ -16,18 +16,20 @@ mod ibc {
     pub use ibc::core::ics03_connection::connection::ConnectionEnd;
     pub use ibc::core::ics03_connection::error::ConnectionError;
     pub use ibc::core::ics04_channel::channel::ChannelEnd;
+    pub use ibc::core::ics04_channel::error::ChannelError;
     pub use ibc::core::ics04_channel::msgs::PacketMsg;
     pub use ibc::core::ics04_channel::packet::Sequence;
-    pub use ibc::core::ics24_host::identifier::{ClientId, ConnectionId};
+    pub use ibc::core::ics24_host::identifier::{
+        ChannelId, ClientId, ConnectionId, PortId,
+    };
+    pub use ibc::core::ics24_host::path;
     pub use ibc::Height;
 }
 
 pub(crate) mod ids;
 pub(crate) mod trie_key;
 
 pub(crate) type SolanaTimestamp = u64;
-pub(crate) type InnerPortId = String;
-pub(crate) type InnerChannelId = String;
 
 /// A triple of send, receive and acknowledge sequences.
 #[derive(
@@ -155,23 +157,15 @@ pub(crate) struct PrivateStorage {
     /// `connection-<N>`.
     pub connections: Vec<Serialised<ibc::ConnectionEnd>>,
 
-    pub channel_ends:
-        BTreeMap<(InnerPortId, InnerChannelId), Serialised<ibc::ChannelEnd>>,
-    pub channel_counter: u64,
-
-    /// The sequence numbers of the packet commitments.
-    pub packet_commitment_sequence_sets:
-        BTreeMap<(InnerPortId, InnerChannelId), Vec<ibc::Sequence>>,
-    /// The sequence numbers of the packet acknowledgements.
-    pub packet_acknowledgement_sequence_sets:
-        BTreeMap<(InnerPortId, InnerChannelId), Vec<ibc::Sequence>>,
+    pub channel_ends: BTreeMap<ids::PortChannelPK, Serialised<ibc::ChannelEnd>>,
+    pub channel_counter: u32,
 
     /// Next send, receive and ack sequence for given (port, channel).
     ///
     /// We’re storing all three sequences in a single object to reduce amount of
     /// different maps we need to maintain.  This saves us on the amount of
     /// trie nodes we need to maintain.
-    pub next_sequence: BTreeMap<(InnerPortId, InnerChannelId), SequenceTriple>,
+    pub next_sequence: BTreeMap<ids::PortChannelPK, SequenceTriple>,
 }
 
 impl PrivateStorage {

solana/solana-ibc/programs/solana-ibc/src/storage/ids.rs

@@ -5,14 +5,6 @@ use super::ibc;
 type Result<T, E = ibc::ClientError> = core::result::Result<T, E>;
 
 
-/// Prefix of IBC channel ids.
-///
-/// Note: We’re not using ChannelId::prefix() because it returns the prefix
-/// without trailing `-` which we want included to simplify stripping of the
-/// prefix.
-pub(super) const CHANNEL_ID_PREFIX: &str = "channel-";
-
-
 /// An index used as unique identifier for a client.
 ///
 /// IBC client id uses `<client-type>-<counter>` format.  This index is
@@ -113,6 +105,71 @@ impl TryFrom<&ibc::ConnectionId> for ConnectionIdx {
 }
 
 
+/// An internal port-channel identifier; that is, it combines IBC port and
+/// channel identifier into a single primary key type.
+///
+/// Currently port identifier is represented as a string.
+///
+/// Meanwhile, the channel identifier is build from IBC identifiers which are of
+/// the form `channel-<number>`.  Rather than treating the identifier as
+/// a string, we’re parsing the number out and keep only that.
+#[derive(
+    Clone,
+    Debug,
+    PartialEq,
+    Eq,
+    PartialOrd,
+    Ord,
+    Hash,
+    borsh::BorshSerialize,
+    borsh::BorshDeserialize,
+)]
+pub struct PortChannelPK {
+    pub(super) port_id: ibc::PortId,
+    pub(super) channel_idx: u32,
+}
+
+impl PortChannelPK {
+    /// Prefix of IBC channel ids.
+    ///
+    /// Note: We’re not using ChannelId::prefix() because it returns the
+    /// prefix without trailing `-` which we want included to simplify stripping
+    /// of the prefix.
+    const CHANNEL_IBC_PREFIX: &'static str = "channel-";
+
+    pub fn try_from(
+        port_id: impl MaybeOwned<ibc::PortId>,
+        channel_id: impl MaybeOwned<ibc::ChannelId>,
+    ) -> Result<Self, ibc::ChannelError> {
+        let channel_str = channel_id.as_ref().as_str();
+        match parse_sans_prefix(Self::CHANNEL_IBC_PREFIX, channel_str) {
+            Some(channel_idx) => {
+                Ok(Self { port_id: port_id.into_owned(), channel_idx })
+            }
+            None => Err(ibc::ChannelError::ChannelNotFound {
+                port_id: port_id.into_owned(),
+                channel_id: channel_id.into_owned(),
+            }),
+        }
+    }
+}
+
+pub trait MaybeOwned<T> {
+    fn as_ref(&self) -> &T;
+    fn into_owned(self) -> T;
+}
+
+impl<T: Clone> MaybeOwned<T> for &T {
+    fn as_ref(&self) -> &T { self }
+    fn into_owned(self) -> T { (*self).clone() }
+}
+
+impl<T> MaybeOwned<T> for T {
+    fn as_ref(&self) -> &T { self }
+    fn into_owned(self) -> T { self }
+}
+
+
 /// Strips `prefix` from `data` and parses it to get `u32`.  Panics if data
 /// doesn’t start with the prefix or parsing fails.
 fn parse_sans_prefix(prefix: &'static str, data: &str) -> Option<u32> {