From 13464aa289c688bb1710f8526dbc2ec3f4a29275 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Tue, 8 Feb 2022 21:15:20 +0100 Subject: [PATCH 1/4] Update balance in transfer, reduce on_packet_failure, like ibctransfer --- contracts/cw20-ics20/src/contract.rs | 13 +++++++++---- contracts/cw20-ics20/src/ibc.rs | 13 +++++++------ 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/contracts/cw20-ics20/src/contract.rs b/contracts/cw20-ics20/src/contract.rs index 35f946fd4..706a16073 100644 --- a/contracts/cw20-ics20/src/contract.rs +++ b/contracts/cw20-ics20/src/contract.rs @@ -18,7 +18,10 @@ use crate::msg::{ AllowMsg, AllowedInfo, AllowedResponse, ChannelResponse, ConfigResponse, ExecuteMsg, InitMsg, ListAllowedResponse, ListChannelsResponse, MigrateMsg, PortResponse, QueryMsg, TransferMsg, }; -use crate::state::{AllowInfo, Config, ADMIN, ALLOW_LIST, CHANNEL_INFO, CHANNEL_STATE, CONFIG}; +use crate::state::{ + increase_channel_balance, AllowInfo, Config, ADMIN, ALLOW_LIST, CHANNEL_INFO, CHANNEL_STATE, + CONFIG, +}; use cw_utils::{maybe_addr, nonpayable, one_coin}; // version info for migration info @@ -130,6 +133,11 @@ pub fn execute_transfer( ); packet.validate()?; + // Update the balance now (optimistically) like ibctransfer modules. + // In on_packet_failure (ack with error message or a timeout), we reduce the balance appropriately. + // This means the channel works fine if success acks are not relayed. + increase_channel_balance(deps.storage, &msg.channel, &amount.denom(), amount.amount())?; + // prepare ibc message let msg = IbcMsg::SendPacket { channel_id: msg.channel, @@ -137,9 +145,6 @@ pub fn execute_transfer( timeout: timeout.into(), }; - // Note: we update local state when we get ack - do not count this transfer towards anything until acked - // similar event messages like ibctransfer module - // send response let res = Response::new() .add_message(msg) diff --git a/contracts/cw20-ics20/src/ibc.rs b/contracts/cw20-ics20/src/ibc.rs index ebc3139cb..d1a4da259 100644 --- a/contracts/cw20-ics20/src/ibc.rs +++ b/contracts/cw20-ics20/src/ibc.rs @@ -11,8 +11,8 @@ use cosmwasm_std::{ use crate::amount::Amount; use crate::error::{ContractError, Never}; use crate::state::{ - increase_channel_balance, reduce_channel_balance, undo_reduce_channel_balance, ChannelInfo, - ReplyArgs, ALLOW_LIST, CHANNEL_INFO, REPLY_ARGS, + reduce_channel_balance, undo_reduce_channel_balance, ChannelInfo, ReplyArgs, ALLOW_LIST, + CHANNEL_INFO, REPLY_ARGS, }; use cw20::Cw20ExecuteMsg; @@ -312,8 +312,9 @@ pub fn ibc_packet_timeout( } // update the balance stored on this (channel, denom) index -fn on_packet_success(deps: DepsMut, packet: IbcPacket) -> Result { +fn on_packet_success(_deps: DepsMut, packet: IbcPacket) -> Result { let msg: Ics20Packet = from_binary(&packet.data)?; + // similar event messages like ibctransfer module let attributes = vec![ attr("action", "acknowledge"), @@ -324,9 +325,6 @@ fn on_packet_success(deps: DepsMut, packet: IbcPacket) -> Result Result { let msg: Ics20Packet = from_binary(&packet.data)?; + // undo the balance update + reduce_channel_balance(deps.storage, &packet.src.channel_id, &msg.denom, msg.amount)?; + let to_send = Amount::from_parts(msg.denom.clone(), msg.amount); let gas_limit = check_gas_limit(deps.as_ref(), &to_send)?; let send = send_amount(to_send, msg.sender.clone()); From 3696e600716e131ca01cd14cabbf14bdc28ed3f6 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Tue, 8 Feb 2022 21:26:26 +0100 Subject: [PATCH 2/4] Update tests --- contracts/cw20-ics20/src/ibc.rs | 45 ++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/contracts/cw20-ics20/src/ibc.rs b/contracts/cw20-ics20/src/ibc.rs index d1a4da259..f7e6abb3f 100644 --- a/contracts/cw20-ics20/src/ibc.rs +++ b/contracts/cw20-ics20/src/ibc.rs @@ -386,9 +386,11 @@ mod test { use super::*; use crate::test_helpers::*; - use crate::contract::query_channel; - use cosmwasm_std::testing::mock_env; - use cosmwasm_std::{coins, to_vec, IbcAcknowledgement, IbcEndpoint, IbcTimeout, Timestamp}; + use crate::contract::{execute, query_channel}; + use crate::msg::{ExecuteMsg, TransferMsg}; + use cosmwasm_std::testing::{mock_env, mock_info}; + use cosmwasm_std::{coins, to_vec, IbcEndpoint, IbcTimeout, Timestamp}; + use cw20::Cw20ReceiveMsg; #[test] fn check_ack_json() { @@ -447,6 +449,7 @@ mod test { ) } + #[allow(dead_code)] fn mock_sent_packet(my_channel: &str, amount: u128, denom: &str, sender: &str) -> IbcPacket { let data = Ics20Packet { denom: denom.into(), @@ -468,6 +471,7 @@ mod test { IbcTimeout::with_timestamp(Timestamp::from_seconds(1665321069)), ) } + fn mock_receive_packet( my_channel: &str, amount: u128, @@ -509,23 +513,31 @@ mod test { ); // prepare some mock packets - let sent_packet = mock_sent_packet(send_channel, 987654321, cw20_denom, "local-sender"); let recv_packet = mock_receive_packet(send_channel, 876543210, cw20_denom, "local-rcpt"); let recv_high_packet = mock_receive_packet(send_channel, 1876543210, cw20_denom, "local-rcpt"); - let msg = IbcPacketReceiveMsg::new(recv_packet.clone()); // cannot receive this denom yet + let msg = IbcPacketReceiveMsg::new(recv_packet.clone()); let res = ibc_packet_receive(deps.as_mut(), mock_env(), msg).unwrap(); assert!(res.messages.is_empty()); let ack: Ics20Ack = from_binary(&res.acknowledgement).unwrap(); let no_funds = Ics20Ack::Error(ContractError::InsufficientFunds {}.to_string()); assert_eq!(ack, no_funds); - // we get a success cache (ack) for a send - let msg = IbcPacketAckMsg::new(IbcAcknowledgement::new(ack_success()), sent_packet); - let res = ibc_packet_ack(deps.as_mut(), mock_env(), msg).unwrap(); - assert_eq!(0, res.messages.len()); + // we send some cw20 tokens over + let transfer = TransferMsg { + channel: send_channel.to_string(), + remote_address: "remote-rcpt".to_string(), + timeout: None, + }; + let msg = ExecuteMsg::Receive(Cw20ReceiveMsg { + sender: "local-sender".to_string(), + amount: Uint128::new(987654321), + msg: to_binary(&transfer).unwrap(), + }); + let info = mock_info(cw20_addr, &[]); + execute(deps.as_mut(), mock_env(), info, msg).unwrap(); // query channel state|_| let state = query_channel(deps.as_ref(), send_channel.to_string()).unwrap(); @@ -566,7 +578,6 @@ mod test { let denom = "uatom"; // prepare some mock packets - let sent_packet = mock_sent_packet(send_channel, 987654321, denom, "local-sender"); let recv_packet = mock_receive_packet(send_channel, 876543210, denom, "local-rcpt"); let recv_high_packet = mock_receive_packet(send_channel, 1876543210, denom, "local-rcpt"); @@ -578,10 +589,14 @@ mod test { let no_funds = Ics20Ack::Error(ContractError::InsufficientFunds {}.to_string()); assert_eq!(ack, no_funds); - // we get a success cache (ack) for a send - let msg = IbcPacketAckMsg::new(IbcAcknowledgement::new(ack_success()), sent_packet); - let res = ibc_packet_ack(deps.as_mut(), mock_env(), msg).unwrap(); - assert_eq!(0, res.messages.len()); + // we transfer some tokens + let msg = ExecuteMsg::Transfer(TransferMsg { + channel: send_channel.to_string(), + remote_address: "my-remote-address".to_string(), + timeout: None, + }); + let info = mock_info("local-sender", &coins(987654321, denom)); + execute(deps.as_mut(), mock_env(), info, msg).unwrap(); // query channel state|_| let state = query_channel(deps.as_ref(), send_channel.to_string()).unwrap(); @@ -606,7 +621,7 @@ mod test { let ack: Ics20Ack = from_binary(&res.acknowledgement).unwrap(); matches!(ack, Ics20Ack::Result(_)); - // TODO: we must call the reply block + // only need to call reply block on error case // query channel state let state = query_channel(deps.as_ref(), send_channel.to_string()).unwrap(); From 82094c82274e399f47792042c64f37672e38ea2c Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Tue, 8 Feb 2022 21:45:13 +0100 Subject: [PATCH 3/4] Check sent ibc packet in tests --- contracts/cw20-ics20/src/ibc.rs | 43 ++++++++++++++------------------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/contracts/cw20-ics20/src/ibc.rs b/contracts/cw20-ics20/src/ibc.rs index f7e6abb3f..7538d84bd 100644 --- a/contracts/cw20-ics20/src/ibc.rs +++ b/contracts/cw20-ics20/src/ibc.rs @@ -389,7 +389,7 @@ mod test { use crate::contract::{execute, query_channel}; use crate::msg::{ExecuteMsg, TransferMsg}; use cosmwasm_std::testing::{mock_env, mock_info}; - use cosmwasm_std::{coins, to_vec, IbcEndpoint, IbcTimeout, Timestamp}; + use cosmwasm_std::{coins, to_vec, IbcEndpoint, IbcMsg, IbcTimeout, Timestamp}; use cw20::Cw20ReceiveMsg; #[test] @@ -449,29 +449,6 @@ mod test { ) } - #[allow(dead_code)] - fn mock_sent_packet(my_channel: &str, amount: u128, denom: &str, sender: &str) -> IbcPacket { - let data = Ics20Packet { - denom: denom.into(), - amount: amount.into(), - sender: sender.to_string(), - receiver: "remote-rcpt".to_string(), - }; - IbcPacket::new( - to_binary(&data).unwrap(), - IbcEndpoint { - port_id: CONTRACT_PORT.to_string(), - channel_id: my_channel.to_string(), - }, - IbcEndpoint { - port_id: REMOTE_PORT.to_string(), - channel_id: "channel-1234".to_string(), - }, - 2, - IbcTimeout::with_timestamp(Timestamp::from_seconds(1665321069)), - ) - } - fn mock_receive_packet( my_channel: &str, amount: u128, @@ -537,7 +514,23 @@ mod test { msg: to_binary(&transfer).unwrap(), }); let info = mock_info(cw20_addr, &[]); - execute(deps.as_mut(), mock_env(), info, msg).unwrap(); + let res = execute(deps.as_mut(), mock_env(), info, msg).unwrap(); + assert_eq!(1, res.messages.len()); + let expected = Ics20Packet { + denom: cw20_denom.into(), + amount: Uint128::new(987654321), + sender: "local-sender".to_string(), + receiver: "remote-rcpt".to_string(), + }; + let timeout = mock_env().block.time.plus_seconds(DEFAULT_TIMEOUT); + assert_eq!( + &res.messages[0], + &SubMsg::new(IbcMsg::SendPacket { + channel_id: send_channel.to_string(), + data: to_binary(&expected).unwrap(), + timeout: IbcTimeout::with_timestamp(timeout), + }) + ); // query channel state|_| let state = query_channel(deps.as_ref(), send_channel.to_string()).unwrap(); From 2ea606be2e9af33a4f23baf24506a798711cb2d7 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Wed, 9 Feb 2022 12:35:41 +0100 Subject: [PATCH 4/4] Add version on packet, so we can use old ack handling for in-flight packets --- contracts/cw20-ics20/src/ibc.rs | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/contracts/cw20-ics20/src/ibc.rs b/contracts/cw20-ics20/src/ibc.rs index 7538d84bd..f666db322 100644 --- a/contracts/cw20-ics20/src/ibc.rs +++ b/contracts/cw20-ics20/src/ibc.rs @@ -11,8 +11,8 @@ use cosmwasm_std::{ use crate::amount::Amount; use crate::error::{ContractError, Never}; use crate::state::{ - reduce_channel_balance, undo_reduce_channel_balance, ChannelInfo, ReplyArgs, ALLOW_LIST, - CHANNEL_INFO, REPLY_ARGS, + increase_channel_balance, reduce_channel_balance, undo_reduce_channel_balance, ChannelInfo, + ReplyArgs, ALLOW_LIST, CHANNEL_INFO, REPLY_ARGS, }; use cw20::Cw20ExecuteMsg; @@ -32,8 +32,12 @@ pub struct Ics20Packet { pub receiver: String, /// the sender address pub sender: String, + /// used only by us to control ack handling + pub v: Option, } +const V2: u32 = 2; + impl Ics20Packet { pub fn new>(amount: Uint128, denom: T, sender: &str, receiver: &str) -> Self { Ics20Packet { @@ -41,6 +45,7 @@ impl Ics20Packet { amount, sender: sender.to_string(), receiver: receiver.to_string(), + v: Some(V2), } } @@ -312,9 +317,15 @@ pub fn ibc_packet_timeout( } // update the balance stored on this (channel, denom) index -fn on_packet_success(_deps: DepsMut, packet: IbcPacket) -> Result { +fn on_packet_success(deps: DepsMut, packet: IbcPacket) -> Result { let msg: Ics20Packet = from_binary(&packet.data)?; + // if this was for an older (pre-v2) packet we send continue with old behavior + // (this is needed for transitioning on a system with pending packet) + if msg.v.is_none() { + increase_channel_balance(deps.storage, &packet.src.channel_id, &msg.denom, msg.amount)?; + } + // similar event messages like ibctransfer module let attributes = vec![ attr("action", "acknowledge"), @@ -336,8 +347,10 @@ fn on_packet_failure( ) -> Result { let msg: Ics20Packet = from_binary(&packet.data)?; - // undo the balance update - reduce_channel_balance(deps.storage, &packet.src.channel_id, &msg.denom, msg.amount)?; + // undo the balance update (but not for pre-v2/None packets which didn't add before sending) + if msg.v.is_some() { + reduce_channel_balance(deps.storage, &packet.src.channel_id, &msg.denom, msg.amount)?; + } let to_send = Amount::from_parts(msg.denom.clone(), msg.amount); let gas_limit = check_gas_limit(deps.as_ref(), &to_send)?; @@ -413,7 +426,7 @@ mod test { "wasm1fucynrfkrt684pm8jrt8la5h2csvs5cnldcgqc", ); // Example message generated from the SDK - let expected = r#"{"amount":"12345","denom":"ucosm","receiver":"wasm1fucynrfkrt684pm8jrt8la5h2csvs5cnldcgqc","sender":"cosmos1zedxv25ah8fksmg2lzrndrpkvsjqgk4zt5ff7n"}"#; + let expected = r#"{"amount":"12345","denom":"ucosm","receiver":"wasm1fucynrfkrt684pm8jrt8la5h2csvs5cnldcgqc","sender":"cosmos1zedxv25ah8fksmg2lzrndrpkvsjqgk4zt5ff7n","v":2}"#; let encdoded = String::from_utf8(to_vec(&packet).unwrap()).unwrap(); assert_eq!(expected, encdoded.as_str()); @@ -461,6 +474,7 @@ mod test { amount: amount.into(), sender: "remote-sender".to_string(), receiver: receiver.to_string(), + v: Some(V2), }; print!("Packet denom: {}", &data.denom); IbcPacket::new( @@ -521,6 +535,7 @@ mod test { amount: Uint128::new(987654321), sender: "local-sender".to_string(), receiver: "remote-rcpt".to_string(), + v: Some(V2), }; let timeout = mock_env().block.time.plus_seconds(DEFAULT_TIMEOUT); assert_eq!(