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

Signer parsing from context #1393

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
2 changes: 2 additions & 0 deletions .changelog/unreleased/features/1392-optional-ack.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Support asynchronous packet acknowledgements.
([\#1392](https://github.com/cosmos/ibc-rs/pull/1392))
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
- Replace the `TryFrom<Signer>` bound on `AccountId` with new context
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Replace the `TryFrom<Signer>` bound on `AccountId` with new context
- [ibc-apps] Replace the `TryFrom<Signer>` bound on `AccountId` with new context

Copy link
Member

Choose a reason for hiding this comment

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

Let's put this under the /unreleased/breaking-changes/ directory.

methods, with the aim of contextually parsing `Signer` instances.
([\#1393](https://github.com/cosmos/ibc-rs/pull/1393))
9 changes: 8 additions & 1 deletion ibc-apps/ics20-transfer/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,14 @@ use ibc_core::primitives::Signer;

/// Methods required in token transfer validation, to be implemented by the host
pub trait TokenTransferValidationContext {
type AccountId: TryFrom<Signer>;
/// Native chain account id.
type AccountId;

/// Attempt to convert a [`Signer`] to a native chain sender account.
fn sender_account_from_signer(&self, signer: &Signer) -> Option<Self::AccountId>;
Copy link
Member

Choose a reason for hiding this comment

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

I suggest renaming these methods to sender_account and receiver_account. The Signer type is currently used for account-like fields everywhere in ibc-rs, but it actually need improvement later since it can be confusing. As you can see, the sender and receiver aren’t actually signers here. So, I'm suggesting to avoid breaking the API naming in the future. Even the args can be updated to sender and receiver.


/// Attempt to convert a [`Signer`] to a native chain receiver account.
fn receiver_account_from_signer(&self, signer: &Signer) -> Option<Self::AccountId>;
Comment on lines +14 to +18
Copy link
Member

Choose a reason for hiding this comment

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

Can you update the return type to Result<Self::AccountId, HostError>? It’d keep it consistent with our other APIs. Since the account ID isn’t always derivable from the signer, and ibc-rs typically propagates errors originating from the host boundary as HostError, it makes more sense to classify it this way. This also gives hosts the flexibility to include contextual information in the error, which ibc-rs can then pass along.
(This PR seems like a good move from this perspective too.)


/// get_port returns the portID for the transfer module.
fn get_port(&self) -> Result<PortId, HostError>;
Expand Down
16 changes: 6 additions & 10 deletions ibc-apps/ics20-transfer/src/handler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,9 @@
packet: &Packet,
data: &PacketData,
) -> Result<(), TokenTransferError> {
let sender = data
.sender
.clone()
.try_into()
.map_err(|_| TokenTransferError::FailedToParseAccount)?;
let sender = ctx_a
.sender_account_from_signer(&data.sender)
.ok_or(TokenTransferError::FailedToParseAccount)?;

Check warning on line 22 in ibc-apps/ics20-transfer/src/handler/mod.rs

View check run for this annotation

Codecov / codecov/patch

ibc-apps/ics20-transfer/src/handler/mod.rs#L20-L22

Added lines #L20 - L22 were not covered by tests

if is_sender_chain_source(
packet.port_id_on_a.clone(),
Expand All @@ -48,11 +46,9 @@
packet: &Packet,
data: &PacketData,
) -> Result<(), TokenTransferError> {
let sender = data
.sender
.clone()
.try_into()
.map_err(|_| TokenTransferError::FailedToParseAccount)?;
let sender = ctx_a
.sender_account_from_signer(&data.sender)
.ok_or(TokenTransferError::FailedToParseAccount)?;

Check warning on line 51 in ibc-apps/ics20-transfer/src/handler/mod.rs

View check run for this annotation

Codecov / codecov/patch

ibc-apps/ics20-transfer/src/handler/mod.rs#L49-L51

Added lines #L49 - L51 were not covered by tests

if is_sender_chain_source(
packet.port_id_on_a.clone(),
Expand Down
14 changes: 8 additions & 6 deletions ibc-apps/ics20-transfer/src/handler/on_recv_packet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,14 @@
.can_receive_coins()
.map_err(|err| (ModuleExtras::empty(), err.into()))?;

let receiver_account = data.receiver.clone().try_into().map_err(|_| {
(
ModuleExtras::empty(),
TokenTransferError::FailedToParseAccount,
)
})?;
let receiver_account = ctx_b
.receiver_account_from_signer(&data.receiver)
.ok_or_else(|| {
(
ModuleExtras::empty(),
TokenTransferError::FailedToParseAccount,
)
})?;

Check warning on line 33 in ibc-apps/ics20-transfer/src/handler/on_recv_packet.rs

View check run for this annotation

Codecov / codecov/patch

ibc-apps/ics20-transfer/src/handler/on_recv_packet.rs#L26-L33

Added lines #L26 - L33 were not covered by tests

let extras = if is_receiver_chain_source(
packet.port_id_on_a.clone(),
Expand Down
18 changes: 6 additions & 12 deletions ibc-apps/ics20-transfer/src/handler/send_transfer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,9 @@ where

let token = &msg.packet_data.token;

let sender: TokenCtx::AccountId = msg
.packet_data
.sender
.clone()
.try_into()
.map_err(|_| TokenTransferError::FailedToParseAccount)?;
let sender = token_ctx_a
.sender_account_from_signer(&msg.packet_data.sender)
.ok_or(TokenTransferError::FailedToParseAccount)?;
Copy link
Member

@Farhad-Shabani Farhad-Shabani Jan 17, 2025

Choose a reason for hiding this comment

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

With the update requested in the other comment, this FailedToParseAccount variant can completely be removed.


if is_sender_chain_source(
msg.port_id_on_a.clone(),
Expand Down Expand Up @@ -129,12 +126,9 @@ where

let token = &msg.packet_data.token;

let sender = msg
.packet_data
.sender
.clone()
.try_into()
.map_err(|_| TokenTransferError::FailedToParseAccount)?;
let sender = token_ctx_a
.sender_account_from_signer(&msg.packet_data.sender)
.ok_or(TokenTransferError::FailedToParseAccount)?;

if is_sender_chain_source(
msg.port_id_on_a.clone(),
Expand Down
8 changes: 4 additions & 4 deletions ibc-core/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,11 @@ asynchronously](https://github.com/cosmos/ibc/tree/main/spec/core/ics-004-channe
This allows modules to receive the packet, but only applying the changes at a
later time (after which they would write the acknowledgement).

We currently force applications to process the packets as part of
`onRecvPacket()`. If you need asynchronous acknowledgements for your
application, please open an issue.
Our implementation supports the same semantics, as part of the `on_recv_packet_execute`
method of [`Module`]. The documentation of this method explains how packets should be
acknowledged out of sync.

Note that this still makes us 100% compatible with `ibc-go`.
[`Module`]: https://github.com/cosmos/ibc-rs/blob/main/ibc-core/ics26-routing/src/module.rs

## Contributing

Expand Down
106 changes: 103 additions & 3 deletions ibc-core/ics04-channel/src/handler/acknowledgement.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,119 @@
use ibc_core_channel_types::channel::{Counterparty, Order, State as ChannelState};
use ibc_core_channel_types::acknowledgement::Acknowledgement;
use ibc_core_channel_types::channel::{ChannelEnd, Counterparty, Order, State as ChannelState};
use ibc_core_channel_types::commitment::{compute_ack_commitment, compute_packet_commitment};
use ibc_core_channel_types::error::ChannelError;
use ibc_core_channel_types::events::AcknowledgePacket;
use ibc_core_channel_types::events::{AcknowledgePacket, WriteAcknowledgement};
use ibc_core_channel_types::msgs::MsgAcknowledgement;
use ibc_core_channel_types::packet::{Packet, Receipt};
use ibc_core_client::context::prelude::*;
use ibc_core_connection::delay::verify_conn_delay_passed;
use ibc_core_connection::types::State as ConnectionState;
use ibc_core_handler_types::events::{IbcEvent, MessageEvent};
use ibc_core_host::types::path::{
AckPath, ChannelEndPath, ClientConsensusStatePath, CommitmentPath, Path, SeqAckPath,
AckPath, ChannelEndPath, ClientConsensusStatePath, CommitmentPath, Path, ReceiptPath,
SeqAckPath, SeqRecvPath,
};
use ibc_core_host::{ExecutionContext, ValidationContext};
use ibc_core_router::module::Module;
use ibc_primitives::prelude::*;

pub fn commit_packet_sequence_number_with_chan_end<ExecCtx>(
ctx_b: &mut ExecCtx,
chan_end_on_b: &ChannelEnd,
packet: &Packet,
) -> Result<(), ChannelError>
where
ExecCtx: ExecutionContext,
{
// `recvPacket` core handler state changes
match chan_end_on_b.ordering {
Order::Unordered => {
let receipt_path_on_b = ReceiptPath {
port_id: packet.port_id_on_b.clone(),
channel_id: packet.chan_id_on_b.clone(),
sequence: packet.seq_on_a,
};

ctx_b.store_packet_receipt(&receipt_path_on_b, Receipt::Ok)?;
}
Order::Ordered => {
let seq_recv_path_on_b = SeqRecvPath::new(&packet.port_id_on_b, &packet.chan_id_on_b);
let next_seq_recv = ctx_b.get_next_sequence_recv(&seq_recv_path_on_b)?;
ctx_b.store_next_sequence_recv(&seq_recv_path_on_b, next_seq_recv.increment())?;
}
_ => {}

Check warning on line 44 in ibc-core/ics04-channel/src/handler/acknowledgement.rs

View check run for this annotation

Codecov / codecov/patch

ibc-core/ics04-channel/src/handler/acknowledgement.rs#L44

Added line #L44 was not covered by tests
}

Ok(())
}

pub fn commit_packet_sequence_number<ExecCtx>(
ctx_b: &mut ExecCtx,
packet: &Packet,
) -> Result<(), ChannelError>
where
ExecCtx: ExecutionContext,
{
let chan_end_path_on_b = ChannelEndPath::new(&packet.port_id_on_b, &packet.chan_id_on_b);
let chan_end_on_b = ctx_b.channel_end(&chan_end_path_on_b)?;

Check warning on line 58 in ibc-core/ics04-channel/src/handler/acknowledgement.rs

View check run for this annotation

Codecov / codecov/patch

ibc-core/ics04-channel/src/handler/acknowledgement.rs#L50-L58

Added lines #L50 - L58 were not covered by tests

commit_packet_sequence_number_with_chan_end(ctx_b, &chan_end_on_b, packet)
}

Check warning on line 61 in ibc-core/ics04-channel/src/handler/acknowledgement.rs

View check run for this annotation

Codecov / codecov/patch

ibc-core/ics04-channel/src/handler/acknowledgement.rs#L60-L61

Added lines #L60 - L61 were not covered by tests

pub fn commit_packet_acknowledgment<ExecCtx>(
ctx_b: &mut ExecCtx,
packet: &Packet,
acknowledgement: &Acknowledgement,
) -> Result<(), ChannelError>
where
ExecCtx: ExecutionContext,
{
let ack_path_on_b = AckPath::new(&packet.port_id_on_b, &packet.chan_id_on_b, packet.seq_on_a);

// `writeAcknowledgement` handler state changes
ctx_b.store_packet_acknowledgement(&ack_path_on_b, compute_ack_commitment(acknowledgement))?;

Ok(())
}

pub fn emit_packet_acknowledgement_event_with_chan_end<ExecCtx>(
ctx_b: &mut ExecCtx,
chan_end_on_b: &ChannelEnd,
packet: Packet,
acknowledgement: Acknowledgement,
) -> Result<(), ChannelError>
where
ExecCtx: ExecutionContext,
{
let conn_id_on_b = &chan_end_on_b.connection_hops()[0];

ctx_b.log_message("success: packet write acknowledgement".to_string())?;

let event = IbcEvent::WriteAcknowledgement(WriteAcknowledgement::new(
packet,
acknowledgement,
conn_id_on_b.clone(),
));
ctx_b.emit_ibc_event(IbcEvent::Message(MessageEvent::Channel))?;
ctx_b.emit_ibc_event(event)?;

Ok(())
}

pub fn emit_packet_acknowledgement_event<ExecCtx>(
ctx_b: &mut ExecCtx,
packet: Packet,
acknowledgement: Acknowledgement,
) -> Result<(), ChannelError>
where
ExecCtx: ExecutionContext,
{
let chan_end_path_on_b = ChannelEndPath::new(&packet.port_id_on_b, &packet.chan_id_on_b);
let chan_end_on_b = ctx_b.channel_end(&chan_end_path_on_b)?;

Check warning on line 112 in ibc-core/ics04-channel/src/handler/acknowledgement.rs

View check run for this annotation

Codecov / codecov/patch

ibc-core/ics04-channel/src/handler/acknowledgement.rs#L103-L112

Added lines #L103 - L112 were not covered by tests

emit_packet_acknowledgement_event_with_chan_end(ctx_b, &chan_end_on_b, packet, acknowledgement)
}

Check warning on line 115 in ibc-core/ics04-channel/src/handler/acknowledgement.rs

View check run for this annotation

Codecov / codecov/patch

ibc-core/ics04-channel/src/handler/acknowledgement.rs#L114-L115

Added lines #L114 - L115 were not covered by tests

pub fn acknowledgement_packet_validate<ValCtx>(
ctx_a: &ValCtx,
module: &dyn Module,
Expand Down
62 changes: 22 additions & 40 deletions ibc-core/ics04-channel/src/handler/recv_packet.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
use ibc_core_channel_types::channel::{Counterparty, Order, State as ChannelState};
use ibc_core_channel_types::commitment::{compute_ack_commitment, compute_packet_commitment};
use ibc_core_channel_types::commitment::compute_packet_commitment;
use ibc_core_channel_types::error::ChannelError;
use ibc_core_channel_types::events::{ReceivePacket, WriteAcknowledgement};
use ibc_core_channel_types::events::ReceivePacket;
use ibc_core_channel_types::msgs::MsgRecvPacket;
use ibc_core_channel_types::packet::Receipt;
use ibc_core_client::context::prelude::*;
use ibc_core_connection::delay::verify_conn_delay_passed;
use ibc_core_connection::types::State as ConnectionState;
Expand All @@ -16,6 +15,11 @@
use ibc_core_router::module::Module;
use ibc_primitives::prelude::*;

use super::acknowledgement::{
commit_packet_acknowledgment, commit_packet_sequence_number_with_chan_end,
emit_packet_acknowledgement_event_with_chan_end,
};

pub fn recv_packet_validate<ValCtx>(ctx_b: &ValCtx, msg: MsgRecvPacket) -> Result<(), ChannelError>
where
ValCtx: ValidationContext,
Expand Down Expand Up @@ -70,42 +74,16 @@
let (extras, acknowledgement) = module.on_recv_packet_execute(&msg.packet, &msg.signer);

// state changes
{
// `recvPacket` core handler state changes
match chan_end_on_b.ordering {
Order::Unordered => {
let receipt_path_on_b = ReceiptPath {
port_id: msg.packet.port_id_on_b.clone(),
channel_id: msg.packet.chan_id_on_b.clone(),
sequence: msg.packet.seq_on_a,
};
commit_packet_sequence_number_with_chan_end(ctx_b, &chan_end_on_b, &msg.packet)?;

ctx_b.store_packet_receipt(&receipt_path_on_b, Receipt::Ok)?;
}
Order::Ordered => {
let seq_recv_path_on_b =
SeqRecvPath::new(&msg.packet.port_id_on_b, &msg.packet.chan_id_on_b);
let next_seq_recv = ctx_b.get_next_sequence_recv(&seq_recv_path_on_b)?;
ctx_b.store_next_sequence_recv(&seq_recv_path_on_b, next_seq_recv.increment())?;
}
_ => {}
}
let ack_path_on_b = AckPath::new(
&msg.packet.port_id_on_b,
&msg.packet.chan_id_on_b,
msg.packet.seq_on_a,
);
// `writeAcknowledgement` handler state changes
ctx_b.store_packet_acknowledgement(
&ack_path_on_b,
compute_ack_commitment(&acknowledgement),
)?;
if let Some(acknowledgement) = acknowledgement.as_ref() {
commit_packet_acknowledgment(ctx_b, &msg.packet, acknowledgement)?;
}

// emit events and logs
{
// receive packet events/logs
ctx_b.log_message("success: packet receive".to_string())?;
ctx_b.log_message("success: packet write acknowledgement".to_string())?;

let conn_id_on_b = &chan_end_on_b.connection_hops()[0];
let event = IbcEvent::ReceivePacket(ReceivePacket::new(
Expand All @@ -115,14 +93,18 @@
));
ctx_b.emit_ibc_event(IbcEvent::Message(MessageEvent::Channel))?;
ctx_b.emit_ibc_event(event)?;
let event = IbcEvent::WriteAcknowledgement(WriteAcknowledgement::new(
msg.packet,
acknowledgement,
conn_id_on_b.clone(),
));
ctx_b.emit_ibc_event(IbcEvent::Message(MessageEvent::Channel))?;
ctx_b.emit_ibc_event(event)?;

// write ack events/logs
if let Some(acknowledgement) = acknowledgement {
emit_packet_acknowledgement_event_with_chan_end(
ctx_b,
&chan_end_on_b,
msg.packet,
acknowledgement,
)?;
}

Check warning on line 105 in ibc-core/ics04-channel/src/handler/recv_packet.rs

View check run for this annotation

Codecov / codecov/patch

ibc-core/ics04-channel/src/handler/recv_packet.rs#L105

Added line #L105 was not covered by tests

// module specific events/logs
for module_event in extras.events {
ctx_b.emit_ibc_event(IbcEvent::Module(module_event))?;
}
Expand Down
24 changes: 23 additions & 1 deletion ibc-core/ics26-routing/src/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,33 @@ pub trait Module: Debug {
// if any error occurs, than an "error acknowledgement"
// must be returned

/// ICS-26 `onRecvPacket` callback implementation.
///
/// # Note on optional acknowledgements
///
/// Acknowledgements can be committed asynchronously, hence
/// the `Option` type. In general, acknowledgements should
/// be committed to storage, accompanied by an ack event,
/// as soon as a packet is received. This will be done
/// automatically as long as `Some(ack)` is returned from
/// this callback. However, in some cases, such as when
/// implementing a multiple hop packet delivery protocol,
/// a packet can only be acknowledged after it has reached
/// the last hop.
///
/// ## Committing a packet asynchronously
///
/// Event emission and state updates for packet acknowledgements
/// can be performed asynchronously using [`emit_packet_acknowledgement_event`]
/// and [`commit_packet_acknowledgment`], respectively.
///
/// [`commit_packet_acknowledgment`]: ../../channel/handler/fn.commit_packet_acknowledgment.html
/// [`emit_packet_acknowledgement_event`]: ../../channel/handler/fn.emit_packet_acknowledgement_event.html
fn on_recv_packet_execute(
&mut self,
packet: &Packet,
relayer: &Signer,
) -> (ModuleExtras, Acknowledgement);
) -> (ModuleExtras, Option<Acknowledgement>);

fn on_acknowledgement_packet_validate(
&self,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,10 @@
&mut self,
_packet: &Packet,
_relayer: &Signer,
) -> (ModuleExtras, Acknowledgement) {
) -> (ModuleExtras, Option<Acknowledgement>) {

Check warning on line 67 in ibc-testkit/src/testapp/ibc/applications/nft_transfer/module.rs

View check run for this annotation

Codecov / codecov/patch

ibc-testkit/src/testapp/ibc/applications/nft_transfer/module.rs#L67

Added line #L67 was not covered by tests
(
ModuleExtras::empty(),
Acknowledgement::try_from(vec![1u8]).expect("Never fails"),
Some(Acknowledgement::try_from(vec![1u8]).expect("Never fails")),

Check warning on line 70 in ibc-testkit/src/testapp/ibc/applications/nft_transfer/module.rs

View check run for this annotation

Codecov / codecov/patch

ibc-testkit/src/testapp/ibc/applications/nft_transfer/module.rs#L70

Added line #L70 was not covered by tests
)
}

Expand Down
Loading
Loading