From 1d7a0a17cd82c03c0c03ff27ef737de8a3583005 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Sat, 15 Jan 2022 23:40:46 +0100 Subject: [PATCH 1/5] Start handling rollback on submessage error --- contracts/cw20-ics20/src/ibc.rs | 41 +++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/contracts/cw20-ics20/src/ibc.rs b/contracts/cw20-ics20/src/ibc.rs index 136337456..de633b9fb 100644 --- a/contracts/cw20-ics20/src/ibc.rs +++ b/contracts/cw20-ics20/src/ibc.rs @@ -72,21 +72,25 @@ fn ack_fail(err: String) -> Binary { to_binary(&res).unwrap() } -const SEND_TOKEN_ID: u64 = 1337; +const RECEIVE_ID: u64 = 1337; +const ACK_FAILURE_ID: u64 = 0xfa17; #[cfg_attr(not(feature = "library"), entry_point)] pub fn reply(_deps: DepsMut, _env: Env, reply: Reply) -> Result { - if reply.id != SEND_TOKEN_ID { - return Err(ContractError::UnknownReplyId { id: reply.id }); - } - let res = match reply.result { - ContractResult::Ok(_) => Response::new(), + match reply.result { + ContractResult::Ok(_) => Ok(Response::new()), ContractResult::Err(err) => { - // encode an acknowledgement error - Response::new().set_data(ack_fail(err)) + let res = Response::new().set_data(ack_fail(err)); + match reply.id { + RECEIVE_ID => { + // TODO: revert state change + Ok(res) + } + ACK_FAILURE_ID => Ok(res), + _ => Err(ContractError::UnknownReplyId { id: reply.id }), + } } - }; - Ok(res) + } } #[cfg_attr(not(feature = "library"), entry_point)] @@ -212,6 +216,9 @@ fn do_ibc_packet_receive( // If it originated on our chain, it looks like "port/channel/ucosm". let denom = parse_voucher_denom(&msg.denom, &packet.src)?; + // Important design note: with ibcv2 and wasmd 0.22 we should return error so this gets reverted + // If we need compatibility with Juno (Jan 2022), we need to ensure that this state change gets reverted + // in the case of the submessage (transfer) erroring CHANNEL_STATE.update( deps.storage, (&channel, denom), @@ -228,7 +235,7 @@ fn do_ibc_packet_receive( let to_send = Amount::from_parts(denom.to_string(), msg.amount); let gas_limit = check_gas_limit(deps.as_ref(), &to_send)?; - let send = send_amount(to_send, msg.receiver.clone(), gas_limit); + let send = send_amount(to_send, msg.receiver.clone(), gas_limit, RECEIVE_ID); let res = IbcReceiveResponse::new() .set_ack(ack_success()) @@ -320,7 +327,7 @@ fn on_packet_failure( 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(), gas_limit); + let send = send_amount(to_send, msg.sender.clone(), gas_limit, ACK_FAILURE_ID); // similar event messages like ibctransfer module let res = IbcBasicResponse::new() @@ -336,14 +343,14 @@ fn on_packet_failure( Ok(res) } -fn send_amount(amount: Amount, recipient: String, gas_limit: Option) -> SubMsg { +fn send_amount(amount: Amount, recipient: String, gas_limit: Option, reply_id: u64) -> SubMsg { match amount { Amount::Native(coin) => SubMsg::reply_on_error( BankMsg::Send { to_address: recipient, amount: vec![coin], }, - SEND_TOKEN_ID, + reply_id, ), Amount::Cw20(coin) => { let msg = Cw20ExecuteMsg::Transfer { @@ -355,7 +362,7 @@ fn send_amount(amount: Amount, recipient: String, gas_limit: Option) -> Sub msg: to_binary(&msg).unwrap(), funds: vec![], }; - let mut sub = SubMsg::reply_on_error(exec, SEND_TOKEN_ID); + let mut sub = SubMsg::reply_on_error(exec, reply_id); sub.gas_limit = gas_limit; sub } @@ -413,7 +420,7 @@ mod test { msg: to_binary(&msg).unwrap(), funds: vec![], }; - let mut msg = SubMsg::reply_on_error(exec, SEND_TOKEN_ID); + let mut msg = SubMsg::reply_on_error(exec, RECEIVE_ID); msg.gas_limit = gas_limit; msg } @@ -424,7 +431,7 @@ mod test { to_address: recipient.into(), amount: coins(amount, denom), }, - SEND_TOKEN_ID, + RECEIVE_ID, ) } From 2803a9806b26df651f390d9462f5decb877f8352 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Sun, 16 Jan 2022 00:10:59 +0100 Subject: [PATCH 2/5] Only make state changes when submessages pass --- contracts/cw20-ics20/src/ibc.rs | 132 ++++++++++++++++-------------- contracts/cw20-ics20/src/state.rs | 50 ++++++++++- 2 files changed, 119 insertions(+), 63 deletions(-) diff --git a/contracts/cw20-ics20/src/ibc.rs b/contracts/cw20-ics20/src/ibc.rs index de633b9fb..cf69993b7 100644 --- a/contracts/cw20-ics20/src/ibc.rs +++ b/contracts/cw20-ics20/src/ibc.rs @@ -2,15 +2,18 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use cosmwasm_std::{ - attr, entry_point, from_binary, to_binary, BankMsg, Binary, ContractResult, Deps, DepsMut, Env, - IbcBasicResponse, IbcChannel, IbcChannelCloseMsg, IbcChannelConnectMsg, IbcChannelOpenMsg, - IbcEndpoint, IbcOrder, IbcPacket, IbcPacketAckMsg, IbcPacketReceiveMsg, IbcPacketTimeoutMsg, - IbcReceiveResponse, Reply, Response, StdResult, SubMsg, Uint128, WasmMsg, + attr, entry_point, from_binary, to_binary, BankMsg, Binary, ContractResult, CosmosMsg, Deps, + DepsMut, Env, IbcBasicResponse, IbcChannel, IbcChannelCloseMsg, IbcChannelConnectMsg, + IbcChannelOpenMsg, IbcEndpoint, IbcOrder, IbcPacket, IbcPacketAckMsg, IbcPacketReceiveMsg, + IbcPacketTimeoutMsg, IbcReceiveResponse, Reply, Response, SubMsg, Uint128, WasmMsg, }; use crate::amount::Amount; use crate::error::{ContractError, Never}; -use crate::state::{ChannelInfo, ALLOW_LIST, CHANNEL_INFO, CHANNEL_STATE}; +use crate::state::{ + increase_channel_balance, reduce_channel_balance, ChannelInfo, ReplyArgs, ALLOW_LIST, + CHANNEL_INFO, REPLY_ARGS, +}; use cw20::Cw20ExecuteMsg; pub const ICS20_VERSION: &str = "ics20-1"; @@ -76,20 +79,39 @@ const RECEIVE_ID: u64 = 1337; const ACK_FAILURE_ID: u64 = 0xfa17; #[cfg_attr(not(feature = "library"), entry_point)] -pub fn reply(_deps: DepsMut, _env: Env, reply: Reply) -> Result { - match reply.result { - ContractResult::Ok(_) => Ok(Response::new()), - ContractResult::Err(err) => { - let res = Response::new().set_data(ack_fail(err)); - match reply.id { - RECEIVE_ID => { - // TODO: revert state change - Ok(res) - } - ACK_FAILURE_ID => Ok(res), - _ => Err(ContractError::UnknownReplyId { id: reply.id }), +pub fn reply(deps: DepsMut, _env: Env, reply: Reply) -> Result { + match reply.id { + RECEIVE_ID => match reply.result { + ContractResult::Ok(_) => { + // Important design note: with ibcv2 and wasmd 0.22 we can implement this all much easier. + // No reply needed... the receive function and submessage should return error on failure and all + // state gets reverted with a proper app-level message auto-generated + + // Since we need compatibility with Juno (Jan 2022), we need to ensure that no state gets written + // if the receive processing succeeds, but the submesage fails, so we can only write after we are + // sure all submessages succeeded. + + // However, this requires passing some state between the ibc_packet_receive function and + // the reply handler. We do this with a singleton, with is "okay" for IBC as there is no + // reentrancy on these functions (cannot be called by another contract). This pattern + // should not be used for ExecuteMsg handlers + let reply_args = REPLY_ARGS.load(deps.storage)?; + reduce_channel_balance( + deps.storage, + &reply_args.channel, + &reply_args.denom, + reply_args.amount, + )?; + + Ok(Response::new()) } - } + ContractResult::Err(err) => Ok(Response::new().set_data(ack_fail(err))), + }, + ACK_FAILURE_ID => match reply.result { + ContractResult::Ok(_) => Ok(Response::new()), + ContractResult::Err(err) => Ok(Response::new().set_data(ack_fail(err))), + }, + _ => Err(ContractError::UnknownReplyId { id: reply.id }), } } @@ -216,30 +238,23 @@ fn do_ibc_packet_receive( // If it originated on our chain, it looks like "port/channel/ucosm". let denom = parse_voucher_denom(&msg.denom, &packet.src)?; - // Important design note: with ibcv2 and wasmd 0.22 we should return error so this gets reverted - // If we need compatibility with Juno (Jan 2022), we need to ensure that this state change gets reverted - // in the case of the submessage (transfer) erroring - CHANNEL_STATE.update( - deps.storage, - (&channel, denom), - |orig| -> Result<_, ContractError> { - // this will return error if we don't have the funds there to cover the request (or no denom registered) - let mut cur = orig.ok_or(ContractError::InsufficientFunds {})?; - cur.outstanding = cur - .outstanding - .checked_sub(msg.amount) - .or(Err(ContractError::InsufficientFunds {}))?; - Ok(cur) - }, - )?; + // we need to save the data to update the balances in reply + let reply_args = ReplyArgs { + channel, + denom: denom.to_string(), + amount: msg.amount, + }; + REPLY_ARGS.save(deps.storage, &reply_args)?; let to_send = Amount::from_parts(denom.to_string(), msg.amount); let gas_limit = check_gas_limit(deps.as_ref(), &to_send)?; - let send = send_amount(to_send, msg.receiver.clone(), gas_limit, RECEIVE_ID); + let send = send_amount(to_send, msg.receiver.clone()); + let mut submsg = SubMsg::reply_always(send, RECEIVE_ID); + submsg.gas_limit = gas_limit; let res = IbcReceiveResponse::new() .set_ack(ack_success()) - .add_submessage(send) + .add_submessage(submsg) .add_attribute("action", "receive") .add_attribute("sender", msg.sender) .add_attribute("receiver", msg.receiver) @@ -271,7 +286,9 @@ pub fn ibc_packet_ack( _env: Env, msg: IbcPacketAckMsg, ) -> Result { - // TODO: trap error like in receive? + // Design decision: should we trap error like in receive? + // TODO: unsure... as it is now a failed ack handling would revert the tx and would be + // retried again and again. is that good? let ics20msg: Ics20Ack = from_binary(&msg.acknowledgement.data)?; match ics20msg { Ics20Ack::Result(_) => on_packet_success(deps, msg.original_packet), @@ -286,7 +303,7 @@ pub fn ibc_packet_timeout( _env: Env, msg: IbcPacketTimeoutMsg, ) -> Result { - // TODO: trap error like in receive? + // TODO: trap error like in receive? (same question as ack above) let packet = msg.packet; on_packet_failure(deps, packet, "timeout".to_string()) } @@ -304,15 +321,8 @@ fn on_packet_success(deps: DepsMut, packet: IbcPacket) -> Result StdResult<_> { - let mut state = orig.unwrap_or_default(); - state.outstanding += amount; - state.total_sent += amount; - Ok(state) - })?; + // we have made a proper transfer, record this + increase_channel_balance(deps.storage, &packet.src.channel_id, &msg.denom, msg.amount)?; Ok(IbcBasicResponse::new().add_attributes(attributes)) } @@ -327,11 +337,13 @@ fn on_packet_failure( 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(), gas_limit, ACK_FAILURE_ID); + let send = send_amount(to_send, msg.sender.clone()); + let mut submsg = SubMsg::reply_on_error(send, ACK_FAILURE_ID); + submsg.gas_limit = gas_limit; // similar event messages like ibctransfer module let res = IbcBasicResponse::new() - .add_submessage(send) + .add_submessage(submsg) .add_attribute("action", "acknowledge") .add_attribute("sender", msg.sender) .add_attribute("receiver", msg.receiver) @@ -343,28 +355,24 @@ fn on_packet_failure( Ok(res) } -fn send_amount(amount: Amount, recipient: String, gas_limit: Option, reply_id: u64) -> SubMsg { +fn send_amount(amount: Amount, recipient: String) -> CosmosMsg { match amount { - Amount::Native(coin) => SubMsg::reply_on_error( - BankMsg::Send { - to_address: recipient, - amount: vec![coin], - }, - reply_id, - ), + Amount::Native(coin) => BankMsg::Send { + to_address: recipient, + amount: vec![coin], + } + .into(), Amount::Cw20(coin) => { let msg = Cw20ExecuteMsg::Transfer { recipient, amount: coin.amount, }; - let exec = WasmMsg::Execute { + WasmMsg::Execute { contract_addr: coin.address, msg: to_binary(&msg).unwrap(), funds: vec![], - }; - let mut sub = SubMsg::reply_on_error(exec, reply_id); - sub.gas_limit = gas_limit; - sub + } + .into() } } } diff --git a/contracts/cw20-ics20/src/state.rs b/contracts/cw20-ics20/src/state.rs index b87307a65..37e8874b3 100644 --- a/contracts/cw20-ics20/src/state.rs +++ b/contracts/cw20-ics20/src/state.rs @@ -1,11 +1,15 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use cosmwasm_std::{Addr, IbcEndpoint, Uint128}; +use crate::ContractError; +use cosmwasm_std::{Addr, IbcEndpoint, StdResult, Storage, Uint128}; use cw_storage_plus::{Item, Map}; pub const CONFIG: Item = Item::new("ics20_config"); +// Used to pass info from the ibc_packet_receive to the reply handler +pub const REPLY_ARGS: Item = Item::new("reply_args"); + /// static info on one channel that doesn't change pub const CHANNEL_INFO: Map<&str, ChannelInfo> = Map::new("channel_info"); @@ -41,3 +45,47 @@ pub struct ChannelInfo { pub struct AllowInfo { pub gas_limit: Option, } + +#[derive(Serialize, Deserialize, Clone, PartialEq, JsonSchema, Debug)] +pub struct ReplyArgs { + pub channel: String, + pub denom: String, + pub amount: Uint128, +} + +pub fn reduce_channel_balance( + storage: &mut dyn Storage, + channel: &str, + denom: &str, + amount: Uint128, +) -> Result<(), ContractError> { + CHANNEL_STATE.update( + storage, + (channel, denom), + |orig| -> Result<_, ContractError> { + // this will return error if we don't have the funds there to cover the request (or no denom registered) + let mut cur = orig.ok_or(ContractError::InsufficientFunds {})?; + cur.outstanding = cur + .outstanding + .checked_sub(amount) + .or(Err(ContractError::InsufficientFunds {}))?; + Ok(cur) + }, + )?; + Ok(()) +} + +pub fn increase_channel_balance( + storage: &mut dyn Storage, + channel: &str, + denom: &str, + amount: Uint128, +) -> Result<(), ContractError> { + CHANNEL_STATE.update(storage, (channel, denom), |orig| -> StdResult<_> { + let mut state = orig.unwrap_or_default(); + state.outstanding += amount; + state.total_sent += amount; + Ok(state) + })?; + Ok(()) +} From 978afcd88ccc25d9c35eee140722d54c4d0e3d5d Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Sun, 16 Jan 2022 00:18:52 +0100 Subject: [PATCH 3/5] Logic should be properly implemented, need to call reply in mock tests --- contracts/cw20-ics20/src/ibc.rs | 15 +++++++++++---- contracts/cw20-ics20/src/state.rs | 17 +++++++++++++++++ 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/contracts/cw20-ics20/src/ibc.rs b/contracts/cw20-ics20/src/ibc.rs index cf69993b7..fc993beff 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, ChannelInfo, ReplyArgs, ALLOW_LIST, - CHANNEL_INFO, REPLY_ARGS, + ensure_channel_balance, increase_channel_balance, reduce_channel_balance, ChannelInfo, + ReplyArgs, ALLOW_LIST, CHANNEL_INFO, REPLY_ARGS, }; use cw20::Cw20ExecuteMsg; @@ -238,6 +238,9 @@ fn do_ibc_packet_receive( // If it originated on our chain, it looks like "port/channel/ucosm". let denom = parse_voucher_denom(&msg.denom, &packet.src)?; + // make sure we have enough balance for this (that is, the later reduction in the reply handler should succeed) + ensure_channel_balance(deps.storage, &channel, denom, msg.amount)?; + // we need to save the data to update the balances in reply let reply_args = ReplyArgs { channel, @@ -428,13 +431,13 @@ mod test { msg: to_binary(&msg).unwrap(), funds: vec![], }; - let mut msg = SubMsg::reply_on_error(exec, RECEIVE_ID); + let mut msg = SubMsg::reply_always(exec, RECEIVE_ID); msg.gas_limit = gas_limit; msg } fn native_payment(amount: u128, denom: &str, recipient: &str) -> SubMsg { - SubMsg::reply_on_error( + SubMsg::reply_always( BankMsg::Send { to_address: recipient.into(), amount: coins(amount, denom), @@ -546,6 +549,8 @@ mod test { let ack: Ics20Ack = from_binary(&res.acknowledgement).unwrap(); matches!(ack, Ics20Ack::Result(_)); + // TODO: we need to call the reply block + // query channel state let state = query_channel(deps.as_ref(), send_channel.to_string()).unwrap(); assert_eq!(state.balances, vec![Amount::cw20(111111111, cw20_addr)]); @@ -600,6 +605,8 @@ mod test { let ack: Ics20Ack = from_binary(&res.acknowledgement).unwrap(); matches!(ack, Ics20Ack::Result(_)); + // TODO: we must call the reply block + // query channel state let state = query_channel(deps.as_ref(), send_channel.to_string()).unwrap(); assert_eq!(state.balances, vec![Amount::native(111111111, denom)]); diff --git a/contracts/cw20-ics20/src/state.rs b/contracts/cw20-ics20/src/state.rs index 37e8874b3..7f455beca 100644 --- a/contracts/cw20-ics20/src/state.rs +++ b/contracts/cw20-ics20/src/state.rs @@ -53,6 +53,23 @@ pub struct ReplyArgs { pub amount: Uint128, } +// this is like reduce_channel_balance, but doesn't change the state +// it returns an error IFF reduce_channel_balance would return an error +pub fn ensure_channel_balance( + storage: &dyn Storage, + channel: &str, + denom: &str, + amount: Uint128, +) -> Result<(), ContractError> { + CHANNEL_STATE + .may_load(storage, (channel, denom))? + .ok_or(ContractError::InsufficientFunds {})? + .outstanding + .checked_sub(amount) + .map_err(|_| ContractError::InsufficientFunds {})?; + Ok(()) +} + pub fn reduce_channel_balance( storage: &mut dyn Storage, channel: &str, From 315be1af1e8da84692fec49e5cfaa9fc4f4e9c1e Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Thu, 27 Jan 2022 20:35:53 +0100 Subject: [PATCH 4/5] Update state on receive, undo on submessage error --- contracts/cw20-ics20/src/ibc.rs | 18 +++++++++--------- contracts/cw20-ics20/src/state.rs | 23 +++++++++++------------ 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/contracts/cw20-ics20/src/ibc.rs b/contracts/cw20-ics20/src/ibc.rs index fc993beff..a05fc0023 100644 --- a/contracts/cw20-ics20/src/ibc.rs +++ b/contracts/cw20-ics20/src/ibc.rs @@ -11,7 +11,7 @@ use cosmwasm_std::{ use crate::amount::Amount; use crate::error::{ContractError, Never}; use crate::state::{ - ensure_channel_balance, increase_channel_balance, reduce_channel_balance, ChannelInfo, + increase_channel_balance, reduce_channel_balance, undo_reduce_channel_balance, ChannelInfo, ReplyArgs, ALLOW_LIST, CHANNEL_INFO, REPLY_ARGS, }; use cw20::Cw20ExecuteMsg; @@ -82,30 +82,30 @@ const ACK_FAILURE_ID: u64 = 0xfa17; pub fn reply(deps: DepsMut, _env: Env, reply: Reply) -> Result { match reply.id { RECEIVE_ID => match reply.result { - ContractResult::Ok(_) => { + ContractResult::Ok(_) => Ok(Response::new()), + ContractResult::Err(err) => { // Important design note: with ibcv2 and wasmd 0.22 we can implement this all much easier. // No reply needed... the receive function and submessage should return error on failure and all // state gets reverted with a proper app-level message auto-generated - // Since we need compatibility with Juno (Jan 2022), we need to ensure that no state gets written - // if the receive processing succeeds, but the submesage fails, so we can only write after we are - // sure all submessages succeeded. + // Since we need compatibility with Juno (Jan 2022), we need to ensure that optimisitic + // state updates in ibc_packet_receive get reverted in the (unlikely) chance of an + // error while sending the token // However, this requires passing some state between the ibc_packet_receive function and // the reply handler. We do this with a singleton, with is "okay" for IBC as there is no // reentrancy on these functions (cannot be called by another contract). This pattern // should not be used for ExecuteMsg handlers let reply_args = REPLY_ARGS.load(deps.storage)?; - reduce_channel_balance( + undo_reduce_channel_balance( deps.storage, &reply_args.channel, &reply_args.denom, reply_args.amount, )?; - Ok(Response::new()) + Ok(Response::new().set_data(ack_fail(err))) } - ContractResult::Err(err) => Ok(Response::new().set_data(ack_fail(err))), }, ACK_FAILURE_ID => match reply.result { ContractResult::Ok(_) => Ok(Response::new()), @@ -239,7 +239,7 @@ fn do_ibc_packet_receive( let denom = parse_voucher_denom(&msg.denom, &packet.src)?; // make sure we have enough balance for this (that is, the later reduction in the reply handler should succeed) - ensure_channel_balance(deps.storage, &channel, denom, msg.amount)?; + reduce_channel_balance(deps.storage, &channel, denom, msg.amount)?; // we need to save the data to update the balances in reply let reply_args = ReplyArgs { diff --git a/contracts/cw20-ics20/src/state.rs b/contracts/cw20-ics20/src/state.rs index 7f455beca..79c127d8c 100644 --- a/contracts/cw20-ics20/src/state.rs +++ b/contracts/cw20-ics20/src/state.rs @@ -53,20 +53,18 @@ pub struct ReplyArgs { pub amount: Uint128, } -// this is like reduce_channel_balance, but doesn't change the state -// it returns an error IFF reduce_channel_balance would return an error -pub fn ensure_channel_balance( - storage: &dyn Storage, +pub fn increase_channel_balance( + storage: &mut dyn Storage, channel: &str, denom: &str, amount: Uint128, ) -> Result<(), ContractError> { - CHANNEL_STATE - .may_load(storage, (channel, denom))? - .ok_or(ContractError::InsufficientFunds {})? - .outstanding - .checked_sub(amount) - .map_err(|_| ContractError::InsufficientFunds {})?; + CHANNEL_STATE.update(storage, (channel, denom), |orig| -> StdResult<_> { + let mut state = orig.unwrap_or_default(); + state.outstanding += amount; + state.total_sent += amount; + Ok(state) + })?; Ok(()) } @@ -92,7 +90,9 @@ pub fn reduce_channel_balance( Ok(()) } -pub fn increase_channel_balance( +// this is like increase, but it only "un-subtracts" (= adds) outstanding, not total_sent +// calling `reduce_channel_balance` and then `undo_reduce_channel_balance` should leave state unchanged. +pub fn undo_reduce_channel_balance( storage: &mut dyn Storage, channel: &str, denom: &str, @@ -101,7 +101,6 @@ pub fn increase_channel_balance( CHANNEL_STATE.update(storage, (channel, denom), |orig| -> StdResult<_> { let mut state = orig.unwrap_or_default(); state.outstanding += amount; - state.total_sent += amount; Ok(state) })?; Ok(()) From a0ca1dcc4bb8d8a06a9e4b788a30f261a659b16b Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Thu, 27 Jan 2022 20:40:43 +0100 Subject: [PATCH 5/5] reply_on_error not always --- contracts/cw20-ics20/src/ibc.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/cw20-ics20/src/ibc.rs b/contracts/cw20-ics20/src/ibc.rs index a05fc0023..ebc3139cb 100644 --- a/contracts/cw20-ics20/src/ibc.rs +++ b/contracts/cw20-ics20/src/ibc.rs @@ -238,7 +238,7 @@ fn do_ibc_packet_receive( // If it originated on our chain, it looks like "port/channel/ucosm". let denom = parse_voucher_denom(&msg.denom, &packet.src)?; - // make sure we have enough balance for this (that is, the later reduction in the reply handler should succeed) + // make sure we have enough balance for this reduce_channel_balance(deps.storage, &channel, denom, msg.amount)?; // we need to save the data to update the balances in reply @@ -252,7 +252,7 @@ fn do_ibc_packet_receive( let to_send = Amount::from_parts(denom.to_string(), msg.amount); let gas_limit = check_gas_limit(deps.as_ref(), &to_send)?; let send = send_amount(to_send, msg.receiver.clone()); - let mut submsg = SubMsg::reply_always(send, RECEIVE_ID); + let mut submsg = SubMsg::reply_on_error(send, RECEIVE_ID); submsg.gas_limit = gas_limit; let res = IbcReceiveResponse::new() @@ -431,13 +431,13 @@ mod test { msg: to_binary(&msg).unwrap(), funds: vec![], }; - let mut msg = SubMsg::reply_always(exec, RECEIVE_ID); + let mut msg = SubMsg::reply_on_error(exec, RECEIVE_ID); msg.gas_limit = gas_limit; msg } fn native_payment(amount: u128, denom: &str, recipient: &str) -> SubMsg { - SubMsg::reply_always( + SubMsg::reply_on_error( BankMsg::Send { to_address: recipient.into(), amount: coins(amount, denom),