Fix cw20 ics20 packets

contracts/cw20-ics20/src/contract.rs

@@ -13,7 +13,7 @@ use cw_storage_plus::Bound;
 use crate::amount::Amount;
 use crate::error::ContractError;
 use crate::ibc::Ics20Packet;
-use crate::migrations::v1;
+use crate::migrations::{v1, v2};
 use crate::msg::{
     AllowMsg, AllowedInfo, AllowedResponse, ChannelResponse, ConfigResponse, ExecuteMsg, InitMsg,
     ListAllowedResponse, ListChannelsResponse, MigrateMsg, PortResponse, QueryMsg, TransferMsg,
@@ -199,9 +199,10 @@ pub fn execute_allow(
 
 const MIGRATE_MIN_VERSION: &str = "0.11.1";
 const MIGRATE_VERSION_2: &str = "0.12.0-alpha1";
+const MIGRATE_VERSION_3: &str = "0.13.1";
 
 #[cfg_attr(not(feature = "library"), entry_point)]
-pub fn migrate(mut deps: DepsMut, _env: Env, _msg: MigrateMsg) -> Result<Response, ContractError> {
+pub fn migrate(mut deps: DepsMut, env: Env, _msg: MigrateMsg) -> Result<Response, ContractError> {
     let version: Version = CONTRACT_VERSION.parse().map_err(from_semver)?;
     let stored = get_contract_version(deps.storage)?;
     let storage_version: Version = stored.version.parse().map_err(from_semver)?;
@@ -235,6 +236,10 @@ pub fn migrate(mut deps: DepsMut, _env: Env, _msg: MigrateMsg) -> Result<Respons
         };
         CONFIG.save(deps.storage, &config)?;
     }
+    // run the v2->v3 converstion if we are v2 style
+    if storage_version <= MIGRATE_VERSION_3.parse().map_err(from_semver)? {
+        v2::update_balances(deps.branch(), &env)?;
+    }
     // otherwise no migration (yet) - add them here
 
     // we don't need to save anything if migrating from the same version
@@ -360,9 +365,10 @@ mod test {
     use super::*;
     use crate::test_helpers::*;
 
-    use cosmwasm_std::testing::{mock_env, mock_info};
+    use cosmwasm_std::testing::{mock_env, mock_info, MOCK_CONTRACT_ADDR};
     use cosmwasm_std::{coin, coins, CosmosMsg, IbcMsg, StdError, Uint128};
 
+    use crate::state::ChannelState;
     use cw_utils::PaymentError;
 
     #[test]
@@ -527,4 +533,36 @@ mod test {
         let err = execute(deps.as_mut(), mock_env(), info, msg).unwrap_err();
         assert_eq!(err, ContractError::NotOnAllowList);
     }
+
+    #[test]
+    fn v3_migration_works() {
+        // basic state with one channel
+        let send_channel = "channel-15";
+        let cw20_addr = "my-token";
+        let native = "ucosm";
+        let mut deps = setup(&[send_channel], &[(cw20_addr, 123456)]);
+
+        // mock that we sent some tokens in both native and cw20 (TODO: cw20)
+        // balances set high
+        deps.querier
+            .update_balance(MOCK_CONTRACT_ADDR, coins(50000, native));
+
+        // channel state a bit lower (some in-flight acks)
+        let state = ChannelState {
+            // 14000 not accounted for (in-flight)
+            outstanding: Uint128::new(36000),
+            total_sent: Uint128::new(100000),
+        };
+        CHANNEL_STATE
+            .save(deps.as_mut().storage, (send_channel, native), &state)
+            .unwrap();
+
+        // run migration
+        migrate(deps.as_mut(), mock_env(), MigrateMsg {}).unwrap();
+
+        // check new channel state
+        let chan = query_channel(deps.as_ref(), send_channel.into()).unwrap();
+        assert_eq!(chan.balances, vec![Amount::native(50000, native)]);
+        assert_eq!(chan.total_sent, vec![Amount::native(114000, native)]);
+    }
 }

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;
 
@@ -32,20 +32,15 @@ pub struct Ics20Packet {
     pub receiver: String,
     /// the sender address
     pub sender: String,
-    /// used only by us to control ack handling
-    pub v: Option<u32>,
 }
 
-const V2: u32 = 2;
-
 impl Ics20Packet {
     pub fn new<T: Into<String>>(amount: Uint128, denom: T, sender: &str, receiver: &str) -> Self {
         Ics20Packet {
             denom: denom.into(),
             amount,
             sender: sender.to_string(),
             receiver: receiver.to_string(),
-            v: Some(V2),
         }
     }
 
@@ -317,15 +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<IbcBasicResponse, ContractError> {
+fn on_packet_success(_deps: DepsMut, packet: IbcPacket) -> Result<IbcBasicResponse, ContractError> {
     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"),
@@ -347,10 +336,8 @@ fn on_packet_failure(
 ) -> Result<IbcBasicResponse, ContractError> {
     let msg: Ics20Packet = from_binary(&packet.data)?;
 
-    // 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)?;
-    }
+    // undo the balance update on failure (as we pre-emptively added it on send)
+    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)?;
@@ -426,7 +413,7 @@ mod test {
             "wasm1fucynrfkrt684pm8jrt8la5h2csvs5cnldcgqc",
         );
         // Example message generated from the SDK
-        let expected = r#"{"amount":"12345","denom":"ucosm","receiver":"wasm1fucynrfkrt684pm8jrt8la5h2csvs5cnldcgqc","sender":"cosmos1zedxv25ah8fksmg2lzrndrpkvsjqgk4zt5ff7n","v":2}"#;
+        let expected = r#"{"amount":"12345","denom":"ucosm","receiver":"wasm1fucynrfkrt684pm8jrt8la5h2csvs5cnldcgqc","sender":"cosmos1zedxv25ah8fksmg2lzrndrpkvsjqgk4zt5ff7n"}"#;
 
         let encdoded = String::from_utf8(to_vec(&packet).unwrap()).unwrap();
         assert_eq!(expected, encdoded.as_str());
@@ -474,7 +461,6 @@ mod test {
             amount: amount.into(),
             sender: "remote-sender".to_string(),
             receiver: receiver.to_string(),
-            v: Some(V2),
         };
         print!("Packet denom: {}", &data.denom);
         IbcPacket::new(
@@ -535,7 +521,6 @@ 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!(

contracts/cw20-ics20/src/migrations.rs

@@ -14,3 +14,74 @@ pub mod v1 {
 
     pub const CONFIG: Item<Config> = Item::new("ics20_config");
 }
+
+// v2 format is anything older than 0.13.1 when we only updated the internal balances on success ack
+pub mod v2 {
+    use crate::amount::Amount;
+    use crate::state::{ChannelState, CHANNEL_INFO, CHANNEL_STATE};
+    use crate::ContractError;
+    use cosmwasm_std::{to_binary, Addr, DepsMut, Env, Order, StdResult, WasmQuery};
+    use cw20::{BalanceResponse, Cw20QueryMsg};
+
+    pub fn update_balances(mut deps: DepsMut, env: &Env) -> Result<(), ContractError> {
+        let channels = CHANNEL_INFO
+            .keys(deps.storage, None, None, Order::Ascending)
+            .collect::<StdResult<Vec<_>>>()?;
+        match channels.len() {
+            0 => Ok(()),
+            1 => {
+                let channel = &channels[0];
+                let addr = &env.contract.address;
+                let states = CHANNEL_STATE
+                    .prefix(channel)
+                    .range(deps.storage, None, None, Order::Ascending)
+                    .collect::<StdResult<Vec<_>>>()?;
+                for (denom, state) in states.into_iter() {
+                    update_denom(deps.branch(), addr, channel, denom, state)?;
+                }
+                Ok(())
+            }
+            _ => Err(ContractError::CannotMigrate {
+                previous_contract: "multiple channels open".into(),
+            }),
+        }
+    }
+
+    fn update_denom(
+        deps: DepsMut,
+        contract: &Addr,
+        channel: &str,
+        denom: String,
+        mut state: ChannelState,
+    ) -> StdResult<()> {
+        // handle this for both native and cw20
+        let balance = match Amount::from_parts(denom.clone(), state.outstanding) {
+            Amount::Native(coin) => deps.querier.query_balance(contract, coin.denom)?.amount,
+            Amount::Cw20(coin) => {
+                // FIXME: we should be able to do this with the following line, but QuerierWrapper doesn't play
+                // with the Querier generics
+                // `Cw20Contract(contract.clone()).balance(&deps.querier, contract)?`
+                let query = WasmQuery::Smart {
+                    contract_addr: coin.address,
+                    msg: to_binary(&Cw20QueryMsg::Balance {
+                        address: contract.into(),
+                    })?,
+                };
+                let res: BalanceResponse = deps.querier.query(&query.into())?;
+                res.balance
+            }
+        };
+
+        // this checks if we have received some coins that are "in flight" and not yet accounted in the state
+        let diff = balance - state.outstanding;
+        // if they are in flight, we add them to the internal state now, as if we added them when sent (not when acked)
+        // to match the current logic
+        if !diff.is_zero() {
+            state.outstanding += diff;
+            state.total_sent += diff;
+            CHANNEL_STATE.save(deps.storage, (channel, &denom), &state)?;
+        }
+
+        Ok(())
+    }
+}