From 87adc8d6b15f6081c1adf169daed4ca8873bd9f6 Mon Sep 17 00:00:00 2001 From: noot <36753753+noot@users.noreply.github.com> Date: Wed, 21 Aug 2024 14:38:15 -0400 Subject: [PATCH] fix penumbra-specific timestamp reads in ibc module (#4822) ## Describe your changes fix penumbra-specific timestamp reads in ibc module by using `HostInterface` instead. - `Ics20WithdrawalWithHandler` was created analogously to `IbcRelayWithHandler`, which attaches a `HostInterface` to be used in `check_and_execute` to get the current block timestamp - the rpc method `client_state` was also updated to use `HostInterface` ## Issue ticket number and link closes #4812 ## Checklist before requesting a review - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > the timestamp reads are unchanged for penumbra, as the `PenumbraHost` is used, which calls the same methods as before --------- Co-authored-by: turbocrime --- crates/core/app/src/action_handler/actions.rs | 24 ++++++++++++-- crates/core/app/src/penumbra_host_chain.rs | 2 +- .../component/ibc/src/component/client.rs | 15 ++++----- .../component/ibc/src/component/packet.rs | 9 ++++-- .../ibc/src/component/rpc/client_query.rs | 6 ++-- .../component/shielded-pool/src/component.rs | 2 ++ .../action_handler/ics20_withdrawal.rs | 29 ++++++++--------- .../ics20_withdrawal_with_handler.rs | 31 +++++++++++++++++++ .../shielded-pool/src/component/transfer.rs | 9 ++++-- 9 files changed, 91 insertions(+), 36 deletions(-) create mode 100644 crates/core/component/shielded-pool/src/component/ics20_withdrawal_with_handler.rs diff --git a/crates/core/app/src/action_handler/actions.rs b/crates/core/app/src/action_handler/actions.rs index 27773aeaac..52b3465772 100644 --- a/crates/core/app/src/action_handler/actions.rs +++ b/crates/core/app/src/action_handler/actions.rs @@ -43,7 +43,13 @@ impl AppActionHandler for Action { .check_stateless(()) .await } - Action::Ics20Withdrawal(action) => action.check_stateless(()).await, + Action::Ics20Withdrawal(action) => { + action + .clone() + .with_handler::() + .check_stateless(()) + .await + } Action::CommunityPoolSpend(action) => action.check_stateless(()).await, Action::CommunityPoolOutput(action) => action.check_stateless(()).await, Action::CommunityPoolDeposit(action) => action.check_stateless(()).await, @@ -78,7 +84,13 @@ impl AppActionHandler for Action { .check_historical(state) .await } - Action::Ics20Withdrawal(action) => action.check_historical(state).await, + Action::Ics20Withdrawal(action) => { + action + .clone() + .with_handler::() + .check_historical(state) + .await + } Action::CommunityPoolSpend(action) => action.check_historical(state).await, Action::CommunityPoolOutput(action) => action.check_historical(state).await, Action::CommunityPoolDeposit(action) => action.check_historical(state).await, @@ -113,7 +125,13 @@ impl AppActionHandler for Action { .check_and_execute(state) .await } - Action::Ics20Withdrawal(action) => action.check_and_execute(state).await, + Action::Ics20Withdrawal(action) => { + action + .clone() + .with_handler::() + .check_and_execute(state) + .await + } Action::CommunityPoolSpend(action) => action.check_and_execute(state).await, Action::CommunityPoolOutput(action) => action.check_and_execute(state).await, Action::CommunityPoolDeposit(action) => action.check_and_execute(state).await, diff --git a/crates/core/app/src/penumbra_host_chain.rs b/crates/core/app/src/penumbra_host_chain.rs index f394c06edf..0336762576 100644 --- a/crates/core/app/src/penumbra_host_chain.rs +++ b/crates/core/app/src/penumbra_host_chain.rs @@ -4,7 +4,7 @@ use penumbra_sct::component::clock::EpochRead; use crate::app::StateReadExt; -/// The implementation of [`penumbr_ibc::component::HostInterface`] for Penumbra. +/// The implementation of [`penumbra_ibc::component::HostInterface`] for Penumbra. /// It encapsulates all of the chain-specific data that the ibc implementation needs. #[derive(Clone)] pub struct PenumbraHost {} diff --git a/crates/core/component/ibc/src/component/client.rs b/crates/core/component/ibc/src/component/client.rs index b6f6ffd327..aa1ce26c4f 100644 --- a/crates/core/component/ibc/src/component/client.rs +++ b/crates/core/component/ibc/src/component/client.rs @@ -240,7 +240,7 @@ pub trait StateWriteExt: StateWrite + StateReadExt { impl StateWriteExt for T {} #[async_trait] -pub trait StateReadExt: StateRead + penumbra_sct::component::clock::EpochRead { +pub trait StateReadExt: StateRead { async fn client_counter(&self) -> Result { self.get("ibc_client_counter") .await @@ -266,7 +266,11 @@ pub trait StateReadExt: StateRead + penumbra_sct::component::clock::EpochRead { client_state.context(format!("could not find client state for {client_id}")) } - async fn get_client_status(&self, client_id: &ClientId) -> ClientStatus { + async fn get_client_status( + &self, + client_id: &ClientId, + current_block_time: tendermint::Time, + ) -> ClientStatus { let client_type = self.get_client_type(client_id).await; if client_type.is_err() { @@ -303,13 +307,6 @@ pub trait StateReadExt: StateRead + penumbra_sct::component::clock::EpochRead { let latest_consensus_state = latest_consensus_state.expect("latest consensus state is Ok"); - let current_block_time = self.get_current_block_timestamp().await; - - if current_block_time.is_err() { - return ClientStatus::Unknown; - } - - let current_block_time = current_block_time.expect("current block time is Ok"); let time_elapsed = current_block_time.duration_since(latest_consensus_state.timestamp); if time_elapsed.is_err() { return ClientStatus::Unknown; diff --git a/crates/core/component/ibc/src/component/packet.rs b/crates/core/component/ibc/src/component/packet.rs index 032986f079..5a5fa3c329 100644 --- a/crates/core/component/ibc/src/component/packet.rs +++ b/crates/core/component/ibc/src/component/packet.rs @@ -5,7 +5,7 @@ use ibc_types::core::{ channel::{channel::State as ChannelState, events, ChannelId, Packet, PortId}, client::Height, }; -use penumbra_sct::component::clock::EpochRead; +use tendermint::Time; use crate::component::{ channel::{StateReadExt as _, StateWriteExt as _}, @@ -96,7 +96,11 @@ impl IBCPacket { #[async_trait] pub trait SendPacketRead: StateRead { /// send_packet_check verifies that a packet can be sent using the provided parameters. - async fn send_packet_check(&self, packet: IBCPacket) -> Result> { + async fn send_packet_check( + &self, + packet: IBCPacket, + current_block_time: Time, + ) -> Result> { let channel = self .get_channel(&packet.source_channel, &packet.source_port) .await? @@ -134,7 +138,6 @@ pub trait SendPacketRead: StateRead { .get_verified_consensus_state(&client_state.latest_height(), &connection.client_id) .await?; - let current_block_time = self.get_current_block_timestamp().await?; let time_elapsed = current_block_time.duration_since(latest_consensus_state.timestamp)?; if client_state.expired(time_elapsed) { diff --git a/crates/core/component/ibc/src/component/rpc/client_query.rs b/crates/core/component/ibc/src/component/rpc/client_query.rs index d8b23d020f..9b52f35097 100644 --- a/crates/core/component/ibc/src/component/rpc/client_query.rs +++ b/crates/core/component/ibc/src/component/rpc/client_query.rs @@ -228,8 +228,10 @@ impl ClientQuery for IbcQuery { let snapshot = self.storage.latest_snapshot(); let client_id = ClientId::from_str(&request.get_ref().client_id) .map_err(|e| tonic::Status::invalid_argument(format!("invalid client id: {e}")))?; - - let client_status = snapshot.get_client_status(&client_id).await; + let timestamp = HI::get_block_timestamp(snapshot.clone()) + .await + .map_err(|e| tonic::Status::aborted(format!("couldn't get block timestamp: {e}")))?; + let client_status = snapshot.get_client_status(&client_id, timestamp).await; let resp = QueryClientStatusResponse { status: client_status.to_string(), }; diff --git a/crates/core/component/shielded-pool/src/component.rs b/crates/core/component/shielded-pool/src/component.rs index 2e89b08526..747dc98f77 100644 --- a/crates/core/component/shielded-pool/src/component.rs +++ b/crates/core/component/shielded-pool/src/component.rs @@ -3,6 +3,7 @@ mod action_handler; mod assets; mod fmd; +mod ics20_withdrawal_with_handler; mod metrics; mod note_manager; mod shielded_pool; @@ -11,6 +12,7 @@ mod transfer; pub use self::metrics::register_metrics; pub use assets::{AssetRegistry, AssetRegistryRead}; pub use fmd::ClueManager; +pub use ics20_withdrawal_with_handler::Ics20WithdrawalWithHandler; pub use note_manager::NoteManager; pub use shielded_pool::{ShieldedPool, StateReadExt, StateWriteExt}; pub use transfer::Ics20Transfer; diff --git a/crates/core/component/shielded-pool/src/component/action_handler/ics20_withdrawal.rs b/crates/core/component/shielded-pool/src/component/action_handler/ics20_withdrawal.rs index 77657dd099..ddc3f7d12c 100644 --- a/crates/core/component/shielded-pool/src/component/action_handler/ics20_withdrawal.rs +++ b/crates/core/component/shielded-pool/src/component/action_handler/ics20_withdrawal.rs @@ -1,24 +1,18 @@ use std::sync::Arc; use anyhow::{ensure, Result}; -use async_trait::async_trait; use cnidarium::{StateRead, StateWrite}; -use cnidarium_component::ActionHandler; -use penumbra_ibc::StateReadExt as _; +use penumbra_ibc::{component::HostInterface, StateReadExt as _}; -use crate::{ - component::transfer::{Ics20TransferReadExt as _, Ics20TransferWriteExt as _}, - Ics20Withdrawal, -}; +use crate::component::transfer::{Ics20TransferReadExt as _, Ics20TransferWriteExt as _}; +use crate::component::Ics20WithdrawalWithHandler; -#[async_trait] -impl ActionHandler for Ics20Withdrawal { - type CheckStatelessContext = (); - async fn check_stateless(&self, _context: ()) -> Result<()> { - self.validate() +impl Ics20WithdrawalWithHandler { + pub async fn check_stateless(&self, _context: ()) -> Result<()> { + self.action().validate() } - async fn check_historical(&self, state: Arc) -> Result<()> { + pub async fn check_historical(&self, state: Arc) -> Result<()> { ensure!( state .get_ibc_params() @@ -29,8 +23,11 @@ impl ActionHandler for Ics20Withdrawal { Ok(()) } - async fn check_and_execute(&self, mut state: S) -> Result<()> { - state.withdrawal_check(self).await?; - state.withdrawal_execute(self).await + pub async fn check_and_execute(&self, mut state: S) -> Result<()> { + let current_block_time = HI::get_block_timestamp(&state).await?; + state + .withdrawal_check(self.action(), current_block_time) + .await?; + state.withdrawal_execute(self.action()).await } } diff --git a/crates/core/component/shielded-pool/src/component/ics20_withdrawal_with_handler.rs b/crates/core/component/shielded-pool/src/component/ics20_withdrawal_with_handler.rs new file mode 100644 index 0000000000..cee833da45 --- /dev/null +++ b/crates/core/component/shielded-pool/src/component/ics20_withdrawal_with_handler.rs @@ -0,0 +1,31 @@ +use crate::Ics20Withdrawal; +use penumbra_ibc::component::HostInterface; +use std::marker::PhantomData; + +pub struct Ics20WithdrawalWithHandler(Ics20Withdrawal, PhantomData); + +impl Ics20WithdrawalWithHandler { + pub fn new(action: Ics20Withdrawal) -> Self { + Self(action, PhantomData) + } + + pub fn action(&self) -> &Ics20Withdrawal { + &self.0 + } + + pub fn into_inner(self) -> Ics20Withdrawal { + self.0 + } +} + +impl From> for Ics20Withdrawal { + fn from(value: Ics20WithdrawalWithHandler) -> Self { + value.0 + } +} + +impl Ics20Withdrawal { + pub fn with_handler(self) -> Ics20WithdrawalWithHandler { + Ics20WithdrawalWithHandler::new(self) + } +} diff --git a/crates/core/component/shielded-pool/src/component/transfer.rs b/crates/core/component/shielded-pool/src/component/transfer.rs index 702c0c6065..d75df18540 100644 --- a/crates/core/component/shielded-pool/src/component/transfer.rs +++ b/crates/core/component/shielded-pool/src/component/transfer.rs @@ -35,6 +35,7 @@ use penumbra_ibc::component::{ }, state_key, }; +use tendermint::Time; // returns a bool indicating if the provided denom was issued locally or if it was bridged in. // this logic is a bit tricky, and adapted from https://github.com/cosmos/ibc/tree/main/spec/app/ics-020-fungible-token-transfer (sendFungibleTokens). @@ -68,12 +69,16 @@ pub struct Ics20Transfer {} #[async_trait] pub trait Ics20TransferReadExt: StateRead { - async fn withdrawal_check(&self, withdrawal: &Ics20Withdrawal) -> Result<()> { + async fn withdrawal_check( + &self, + withdrawal: &Ics20Withdrawal, + current_block_time: Time, + ) -> Result<()> { // create packet let packet: IBCPacket = withdrawal.clone().into(); // send packet - self.send_packet_check(packet).await?; + self.send_packet_check(packet, current_block_time).await?; Ok(()) }