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

imp: refactor send-coins-*() and add memo field to escrow-* and burn-* methods #836

Merged
merged 31 commits into from
Dec 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
6f1a5a6
split send_coins_*() functions
plafer Aug 22, 2023
3336f13
arg name change
plafer Aug 22, 2023
625efb3
add error variant
plafer Aug 22, 2023
4ade607
add error
plafer Aug 22, 2023
0a4b42b
add `Other` variant to `TokenTransferError`
plafer Aug 23, 2023
d144b48
fmt
plafer Aug 23, 2023
d3e497e
fix: restore `no_std` support for `serde` feature (#790)
Farhad-Shabani Aug 23, 2023
3bd0bc9
bump ibc-proto to v0.34.1 and borsh to v0.10 (#844)
plafer Aug 29, 2023
869d3cb
add borsh derive for `MsgTransfer`
plafer Aug 30, 2023
e618b31
derive JsonSchema for MsgTransfer
plafer Aug 30, 2023
a0ef6de
fix JsonSchema derive for MsgTransfer
plafer Aug 30, 2023
476f6da
Merge branch 'main' into plafer/transfer-extra-data
plafer Aug 31, 2023
1ed7d2e
Merge branch 'main' into plafer/transfer-extra-data
plafer Sep 1, 2023
7c49ec1
Merge branch 'main' into plafer/transfer-extra-data
plafer Sep 1, 2023
419a28f
Merge branch 'main' into plafer/transfer-extra-data
plafer Sep 5, 2023
9fd3deb
Merge branch 'main' into plafer/transfer-extra-data
plafer Sep 5, 2023
c126e7f
Merge branch 'main' into plafer/transfer-extra-data
plafer Sep 7, 2023
fffcf22
execute() methods must have &mut self
plafer Sep 14, 2023
8041a87
Merge branch 'main' into plafer/transfer-extra-data
Farhad-Shabani Sep 22, 2023
ca1c673
Merge branch 'main' into plafer/transfer-extra-data
Farhad-Shabani Oct 4, 2023
bc09e28
Merge branch 'main' into plafer/transfer-extra-data
Farhad-Shabani Oct 6, 2023
b7dd19d
Merge branch 'main' into plafer/transfer-extra-data
Farhad-Shabani Oct 14, 2023
5279297
Merge branch 'main' into plafer/transfer-extra-data
Farhad-Shabani Nov 27, 2023
8e9213d
nit: spell checker
Farhad-Shabani Nov 27, 2023
9b6d226
imp: simplify escrow/unescrow abstraction + using memo instead
Farhad-Shabani Dec 8, 2023
58d8c9c
chore: dress up with unclogs
Farhad-Shabani Dec 8, 2023
6592e86
Merge branch 'main' into plafer/transfer-extra-data
Farhad-Shabani Dec 8, 2023
cbdcacb
nit: adjust unlogs
Farhad-Shabani Dec 8, 2023
743a3ff
nit: remove confusing InternalTransferFailed error variant
Farhad-Shabani Dec 8, 2023
e56a1c1
imp: add memo field to burn-coins-*
Farhad-Shabani Dec 9, 2023
5a16481
docs: adjust & add missing docstrings
Farhad-Shabani Dec 11, 2023
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: 0 additions & 2 deletions .changelog/unreleased/993-bump-ibc-proto-rs.md

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
- `[ibc-app-transfer]` Refactor `send-coins-*()` methods by breaking them down
into distinct escrow and unescrow methods, enhancing both clarity and
specificity in functionality.
([\#837](https://github.com/cosmos/ibc-rs/issues/837))

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
- `[ibc-app-transfer]` Add `memo` field to `escrow-coins-*()` and
`burn-coins-*()` methods, allowing implementors to pass in arbitrary data
necessary for their use case.
([\#839](https://github.com/cosmos/ibc-rs/issues/837))

Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
- Optimize `IdentifierError` variants and make them mutually exclusive.
([\#978](https://github.com/cosmos/ibc-rs/issues/978))
- `[ibc-core-host-type]` Optimize `IdentifierError` variants and make them
mutually exclusive. ([\#978](https://github.com/cosmos/ibc-rs/issues/978))
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- `[ibc-data-types]` Bump ibc-proto-rs dependency to v0.39.1.
([\#993](https://github.com/cosmos/ibc-rs/issues/993))
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
- [`ibc`] Minimize `prost` dependency by introducing `ToVec` trait
- `[ibc]` Minimize `prost` dependency by introducing `ToVec` trait
- Now `prost` is only imported in `ibc-primitives` crate
- Remove error variants originating from `prost` (Breaking change)
- Eliminate the need for the `bytes` dependency
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
- [`cw-check`] More rigorous CosmWasm check by upgrading dependencies and
- `[cw-check]` More rigorous CosmWasm check by upgrading dependencies and
including `std` and `schema` features for `ibc-core`.
([\#992](https://github.com/cosmos/ibc-rs/pull/992))
61 changes: 45 additions & 16 deletions ibc-apps/ics20-transfer/src/context.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! Defines the main context traits and IBC module callbacks

use ibc_app_transfer_types::error::TokenTransferError;
use ibc_app_transfer_types::{PrefixedCoin, PrefixedDenom};
use ibc_app_transfer_types::{Memo, PrefixedCoin, PrefixedDenom};
use ibc_core::host::types::identifiers::{ChannelId, PortId};
use ibc_core::primitives::prelude::*;
use ibc_core::primitives::Signer;
Expand All @@ -13,24 +13,31 @@ pub trait TokenTransferValidationContext {
/// get_port returns the portID for the transfer module.
fn get_port(&self) -> Result<PortId, TokenTransferError>;

/// Returns the escrow account id for a port and channel combination
fn get_escrow_account(
&self,
port_id: &PortId,
channel_id: &ChannelId,
) -> Result<Self::AccountId, TokenTransferError>;

/// Returns Ok() if the host chain supports sending coins.
fn can_send_coins(&self) -> Result<(), TokenTransferError>;

/// Returns Ok() if the host chain supports receiving coins.
fn can_receive_coins(&self) -> Result<(), TokenTransferError>;

/// Validates the sender and receiver accounts and the coin inputs
fn send_coins_validate(
/// Validates that the tokens can be escrowed successfully.
///
/// `memo` field allows to incorporate additional contextual details in the
/// escrow validation.
fn escrow_coins_validate(
&self,
from_account: &Self::AccountId,
port_id: &PortId,
channel_id: &ChannelId,
coin: &PrefixedCoin,
memo: &Memo,
) -> Result<(), TokenTransferError>;

/// Validates that the tokens can be unescrowed successfully.
fn unescrow_coins_validate(
&self,
to_account: &Self::AccountId,
port_id: &PortId,
channel_id: &ChannelId,
coin: &PrefixedCoin,
) -> Result<(), TokenTransferError>;

Expand All @@ -41,11 +48,15 @@ pub trait TokenTransferValidationContext {
coin: &PrefixedCoin,
) -> Result<(), TokenTransferError>;

/// Validates the sender account and the coin input
/// Validates the sender account and the coin input before burning.
///
/// `memo` field allows to incorporate additional contextual details in the
/// burn validation.
fn burn_coins_validate(
&self,
account: &Self::AccountId,
coin: &PrefixedCoin,
memo: &Memo,
) -> Result<(), TokenTransferError>;

/// Returns a hash of the prefixed denom.
Expand All @@ -55,27 +66,45 @@ pub trait TokenTransferValidationContext {
}
}

/// Methods required in token transfer execution, to be implemented by the host
/// Methods required in token transfer execution, to be implemented by the host.
pub trait TokenTransferExecutionContext: TokenTransferValidationContext {
/// This function should enable sending ibc fungible tokens from one account to another
fn send_coins_execute(
/// Executes the escrow of the tokens in a user account.
///
/// `memo` field allows to incorporate additional contextual details in the
/// escrow execution.
fn escrow_coins_execute(
&mut self,
from_account: &Self::AccountId,
port_id: &PortId,
channel_id: &ChannelId,
coin: &PrefixedCoin,
memo: &Memo,
) -> Result<(), TokenTransferError>;

/// Executes the unescrow of the tokens in a user account.
fn unescrow_coins_execute(
&mut self,
to_account: &Self::AccountId,
port_id: &PortId,
channel_id: &ChannelId,
coin: &PrefixedCoin,
) -> Result<(), TokenTransferError>;

/// This function to enable minting ibc tokens to a user account
/// Executes minting of the tokens in a user account.
fn mint_coins_execute(
&mut self,
account: &Self::AccountId,
coin: &PrefixedCoin,
) -> Result<(), TokenTransferError>;

/// This function should enable burning of minted tokens in a user account
/// Executes burning of the tokens in a user account.
///
/// `memo` field allows to incorporate additional contextual details in the
/// burn execution.
fn burn_coins_execute(
&mut self,
account: &Self::AccountId,
coin: &PrefixedCoin,
memo: &Memo,
) -> Result<(), TokenTransferError>;
}
21 changes: 12 additions & 9 deletions ibc-apps/ics20-transfer/src/handler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,12 @@
packet.chan_id_on_a.clone(),
&data.token.denom,
) {
// unescrow tokens back to sender
let escrow_address =
ctx_a.get_escrow_account(&packet.port_id_on_a, &packet.chan_id_on_a)?;

ctx_a.send_coins_execute(&escrow_address, &sender, &data.token)
ctx_a.unescrow_coins_execute(
&sender,
&packet.port_id_on_a,
&packet.chan_id_on_a,
&data.token,
)

Check warning on line 35 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#L30-L35

Added lines #L30 - L35 were not covered by tests
}
// mint vouchers back to sender
else {
Expand All @@ -55,10 +56,12 @@
packet.chan_id_on_a.clone(),
&data.token.denom,
) {
let escrow_address =
ctx_a.get_escrow_account(&packet.port_id_on_a, &packet.chan_id_on_a)?;

ctx_a.send_coins_validate(&escrow_address, &sender, &data.token)
ctx_a.unescrow_coins_validate(
&sender,
&packet.port_id_on_a,
&packet.chan_id_on_a,
&data.token,
)

Check warning on line 64 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#L59-L64

Added lines #L59 - L64 were not covered by tests
} else {
ctx_a.mint_coins_validate(&sender, &data.token)
}
Expand Down
19 changes: 12 additions & 7 deletions ibc-apps/ics20-transfer/src/handler/on_recv_packet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,6 @@
c
};

let escrow_address = ctx_b
.get_escrow_account(&packet.port_id_on_b, &packet.chan_id_on_b)
.map_err(|token_err| (ModuleExtras::empty(), token_err))?;

// Note: it is correct to do the validation here because `recv_packet()`
// works slightly differently. We do not have a
// `on_recv_packet_validate()` callback because regardless of whether or
Expand All @@ -58,11 +54,20 @@
// gets relayed back to the sender so that the escrowed tokens
// can be refunded.
ctx_b
.send_coins_validate(&escrow_address, &receiver_account, &coin)
.unescrow_coins_validate(
&receiver_account,
&packet.port_id_on_b,
&packet.chan_id_on_b,
&coin,
)

Check warning on line 62 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#L57-L62

Added lines #L57 - L62 were not covered by tests
.map_err(|token_err| (ModuleExtras::empty(), token_err))?;

ctx_b
.send_coins_execute(&escrow_address, &receiver_account, &coin)
.unescrow_coins_execute(
&receiver_account,
&packet.port_id_on_b,
&packet.chan_id_on_b,
&coin,
)

Check warning on line 70 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#L65-L70

Added lines #L65 - L70 were not covered by tests
.map_err(|token_err| (ModuleExtras::empty(), token_err))?;

ModuleExtras::empty()
Expand Down
24 changes: 16 additions & 8 deletions ibc-apps/ics20-transfer/src/handler/send_transfer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,15 @@
msg.chan_id_on_a.clone(),
&token.denom,
) {
let escrow_address =
token_ctx_a.get_escrow_account(&msg.port_id_on_a, &msg.chan_id_on_a)?;
token_ctx_a.send_coins_validate(&sender, &escrow_address, token)?;
token_ctx_a.escrow_coins_validate(
&sender,
&msg.port_id_on_a,
&msg.chan_id_on_a,
token,
&msg.packet_data.memo,
)?;
} else {
token_ctx_a.burn_coins_validate(&sender, token)?;
token_ctx_a.burn_coins_validate(&sender, token, &msg.packet_data.memo)?;

Check warning on line 79 in ibc-apps/ics20-transfer/src/handler/send_transfer.rs

View check run for this annotation

Codecov / codecov/patch

ibc-apps/ics20-transfer/src/handler/send_transfer.rs#L79

Added line #L79 was not covered by tests
}

let packet = {
Expand Down Expand Up @@ -137,11 +141,15 @@
msg.chan_id_on_a.clone(),
&token.denom,
) {
let escrow_address =
token_ctx_a.get_escrow_account(&msg.port_id_on_a, &msg.chan_id_on_a)?;
token_ctx_a.send_coins_execute(&sender, &escrow_address, token)?;
token_ctx_a.escrow_coins_execute(
&sender,
&msg.port_id_on_a,
&msg.chan_id_on_a,
token,
&msg.packet_data.memo,
)?;
} else {
token_ctx_a.burn_coins_execute(&sender, token)?;
token_ctx_a.burn_coins_execute(&sender, token, &msg.packet_data.memo)?;

Check warning on line 152 in ibc-apps/ics20-transfer/src/handler/send_transfer.rs

View check run for this annotation

Codecov / codecov/patch

ibc-apps/ics20-transfer/src/handler/send_transfer.rs#L152

Added line #L152 was not covered by tests
}

let packet = {
Expand Down
9 changes: 9 additions & 0 deletions ibc-apps/ics20-transfer/types/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,19 @@ use ibc_core::host::types::identifiers::{ChannelId, PortId};
use ibc_core::primitives::prelude::*;
use uint::FromDecStrErr;

use crate::Amount;

#[derive(Display, Debug)]
pub enum TokenTransferError {
/// context error: `{0}`
ContextError(ContextError),
/// invalid identifier: `{0}`
InvalidIdentifier(IdentifierError),
/// insufficient funds: tried to send `{send_attempt}`, sender only has `{available_funds}`
InsufficientFunds {
send_attempt: Amount,
available_funds: Amount,
},
/// destination channel not found in the counterparty of port_id `{port_id}` and channel_id `{channel_id}`
DestinationChannelNotFound {
port_id: PortId,
Expand Down Expand Up @@ -70,6 +77,8 @@ pub enum TokenTransferError {
InvalidCoin { coin: String },
/// decoding raw bytes as UTF8 string error: `{0}`
Utf8Decode(Utf8Error),
/// other error: `{0}`
Other(String),
}

#[cfg(feature = "std")]
Expand Down
1 change: 1 addition & 0 deletions ibc-apps/ics20-transfer/types/src/msgs/transfer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
feature = "borsh",
derive(borsh::BorshSerialize, borsh::BorshDeserialize)
)]
#[cfg_attr(feature = "schema", derive(schemars::JsonSchema))]

Check warning on line 35 in ibc-apps/ics20-transfer/types/src/msgs/transfer.rs

View check run for this annotation

Codecov / codecov/patch

ibc-apps/ics20-transfer/types/src/msgs/transfer.rs#L35

Added line #L35 was not covered by tests
pub struct MsgTransfer {
/// the port on which the packet will be sent
pub port_id_on_a: PortId,
Expand Down
44 changes: 29 additions & 15 deletions ibc-testkit/src/testapp/ibc/applications/transfer/context.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
use ibc::apps::transfer::context::{TokenTransferExecutionContext, TokenTransferValidationContext};
use ibc::apps::transfer::types::error::TokenTransferError;
use ibc::apps::transfer::types::PrefixedCoin;
use ibc::apps::transfer::types::{Memo, PrefixedCoin};
use ibc::core::host::types::identifiers::{ChannelId, PortId};
use ibc::core::primitives::Signer;
use ibc::cosmos_host::utils::cosmos_adr028_escrow_address;
use subtle_encoding::bech32;

use super::types::DummyTransferModule;

Expand All @@ -15,27 +13,29 @@
Ok(PortId::transfer())
}

fn get_escrow_account(
&self,
port_id: &PortId,
channel_id: &ChannelId,
) -> Result<Self::AccountId, TokenTransferError> {
let addr = cosmos_adr028_escrow_address(port_id, channel_id);
Ok(bech32::encode("cosmos", addr).into())
}

fn can_send_coins(&self) -> Result<(), TokenTransferError> {
Ok(())
}

fn can_receive_coins(&self) -> Result<(), TokenTransferError> {
Ok(())
}

fn send_coins_validate(
fn escrow_coins_validate(
&self,
_from_account: &Self::AccountId,
_port_id: &PortId,
_channel_id: &ChannelId,
_coin: &PrefixedCoin,
_memo: &Memo,
) -> Result<(), TokenTransferError> {
Ok(())
}

fn unescrow_coins_validate(
&self,

Check warning on line 35 in ibc-testkit/src/testapp/ibc/applications/transfer/context.rs

View check run for this annotation

Codecov / codecov/patch

ibc-testkit/src/testapp/ibc/applications/transfer/context.rs#L34-L35

Added lines #L34 - L35 were not covered by tests
_to_account: &Self::AccountId,
_port_id: &PortId,
_channel_id: &ChannelId,

Check warning on line 38 in ibc-testkit/src/testapp/ibc/applications/transfer/context.rs

View check run for this annotation

Codecov / codecov/patch

ibc-testkit/src/testapp/ibc/applications/transfer/context.rs#L37-L38

Added lines #L37 - L38 were not covered by tests
_coin: &PrefixedCoin,
) -> Result<(), TokenTransferError> {
Ok(())
Expand All @@ -53,16 +53,29 @@
&self,
_account: &Self::AccountId,
_coin: &PrefixedCoin,
_memo: &Memo,

Check warning on line 56 in ibc-testkit/src/testapp/ibc/applications/transfer/context.rs

View check run for this annotation

Codecov / codecov/patch

ibc-testkit/src/testapp/ibc/applications/transfer/context.rs#L56

Added line #L56 was not covered by tests
) -> Result<(), TokenTransferError> {
Ok(())
}
}

impl TokenTransferExecutionContext for DummyTransferModule {
fn send_coins_execute(
fn escrow_coins_execute(
&mut self,
_from_account: &Self::AccountId,
_port_id: &PortId,
_channel_id: &ChannelId,
_coin: &PrefixedCoin,
_memo: &Memo,
) -> Result<(), TokenTransferError> {
Ok(())
}

fn unescrow_coins_execute(
&mut self,

Check warning on line 75 in ibc-testkit/src/testapp/ibc/applications/transfer/context.rs

View check run for this annotation

Codecov / codecov/patch

ibc-testkit/src/testapp/ibc/applications/transfer/context.rs#L74-L75

Added lines #L74 - L75 were not covered by tests
_to_account: &Self::AccountId,
_port_id: &PortId,
_channel_id: &ChannelId,

Check warning on line 78 in ibc-testkit/src/testapp/ibc/applications/transfer/context.rs

View check run for this annotation

Codecov / codecov/patch

ibc-testkit/src/testapp/ibc/applications/transfer/context.rs#L77-L78

Added lines #L77 - L78 were not covered by tests
_coin: &PrefixedCoin,
) -> Result<(), TokenTransferError> {
Ok(())
Expand All @@ -80,6 +93,7 @@
&mut self,
_account: &Self::AccountId,
_coin: &PrefixedCoin,
_memo: &Memo,

Check warning on line 96 in ibc-testkit/src/testapp/ibc/applications/transfer/context.rs

View check run for this annotation

Codecov / codecov/patch

ibc-testkit/src/testapp/ibc/applications/transfer/context.rs#L96

Added line #L96 was not covered by tests
) -> Result<(), TokenTransferError> {
Ok(())
}
Expand Down
Loading