From af19599fa38760431ffbbcfb43e8399d842415c9 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Thu, 24 Mar 2022 13:15:46 +0100 Subject: [PATCH 1/6] Remove extra "v" field, act is if always set --- contracts/cw20-ics20/src/ibc.rs | 27 ++++++--------------------- 1 file changed, 6 insertions(+), 21 deletions(-) diff --git a/contracts/cw20-ics20/src/ibc.rs b/contracts/cw20-ics20/src/ibc.rs index add61ac5f..d46121f65 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; @@ -32,12 +32,8 @@ 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 { @@ -45,7 +41,6 @@ impl Ics20Packet { 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 { +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"), @@ -347,10 +336,8 @@ fn on_packet_failure( ) -> Result { 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!( From 2aa334ab20d080135b09c0e4837d35b2e1ed4e01 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Thu, 24 Mar 2022 13:34:07 +0100 Subject: [PATCH 2/6] Add new migration path for old contracts to the new logic --- contracts/cw20-ics20/src/contract.rs | 9 ++++-- contracts/cw20-ics20/src/migrations.rs | 40 ++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/contracts/cw20-ics20/src/contract.rs b/contracts/cw20-ics20/src/contract.rs index 706a16073..e04f4eb6e 100644 --- a/contracts/cw20-ics20/src/contract.rs +++ b/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 { +pub fn migrate(mut deps: DepsMut, env: Env, _msg: MigrateMsg) -> Result { 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) -> Resultv3 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 diff --git a/contracts/cw20-ics20/src/migrations.rs b/contracts/cw20-ics20/src/migrations.rs index e874569a0..4c79b6ffb 100644 --- a/contracts/cw20-ics20/src/migrations.rs +++ b/contracts/cw20-ics20/src/migrations.rs @@ -14,3 +14,43 @@ pub mod v1 { pub const CONFIG: Item = 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::state::{CHANNEL_INFO, CHANNEL_STATE}; + use crate::ContractError; + use cosmwasm_std::{Coin, DepsMut, Env, Order, StdResult}; + + pub fn update_balances(deps: DepsMut, env: &Env) -> Result<(), ContractError> { + let channels = CHANNEL_INFO + .keys(deps.storage, None, None, Order::Ascending) + .collect::>>()?; + 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::>>()?; + for (denom, mut state) in states.into_iter() { + // this checks if we have received some coins that are "in flight" and not yet accounted in the state + let Coin { amount, .. } = deps.querier.query_balance(addr, &denom)?; + let diff = state.outstanding - amount; + // 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(()) + } + _ => Err(ContractError::CannotMigrate { + previous_contract: "multiple channels open".into(), + }), + } + } +} From 514a203d5a28136fb32074715c530c0172bd1859 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Thu, 24 Mar 2022 17:44:44 +0100 Subject: [PATCH 3/6] Test migration --- contracts/cw20-ics20/src/contract.rs | 35 +++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/contracts/cw20-ics20/src/contract.rs b/contracts/cw20-ics20/src/contract.rs index e04f4eb6e..3c8b25aa9 100644 --- a/contracts/cw20-ics20/src/contract.rs +++ b/contracts/cw20-ics20/src/contract.rs @@ -365,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] @@ -532,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)]); + } } From f0c32d32febdb3b50ed0de13dbf88806e87dae30 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Thu, 24 Mar 2022 17:45:25 +0100 Subject: [PATCH 4/6] Fix subtration in diff --- contracts/cw20-ics20/src/migrations.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/cw20-ics20/src/migrations.rs b/contracts/cw20-ics20/src/migrations.rs index 4c79b6ffb..c48e7f71a 100644 --- a/contracts/cw20-ics20/src/migrations.rs +++ b/contracts/cw20-ics20/src/migrations.rs @@ -37,7 +37,7 @@ pub mod v2 { for (denom, mut state) in states.into_iter() { // this checks if we have received some coins that are "in flight" and not yet accounted in the state let Coin { amount, .. } = deps.querier.query_balance(addr, &denom)?; - let diff = state.outstanding - amount; + let diff = amount - 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() { From 9ad5fef463ad7ff4ffd187e1b23ec2c03e0758ef Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Thu, 24 Mar 2022 18:09:40 +0100 Subject: [PATCH 5/6] Add (untested) support for migrating cw20 --- contracts/cw20-ics20/src/migrations.rs | 61 ++++++++++++++++++++------ 1 file changed, 47 insertions(+), 14 deletions(-) diff --git a/contracts/cw20-ics20/src/migrations.rs b/contracts/cw20-ics20/src/migrations.rs index c48e7f71a..536b4da96 100644 --- a/contracts/cw20-ics20/src/migrations.rs +++ b/contracts/cw20-ics20/src/migrations.rs @@ -17,11 +17,13 @@ pub mod v1 { // v2 format is anything older than 0.13.1 when we only updated the internal balances on success ack pub mod v2 { - use crate::state::{CHANNEL_INFO, CHANNEL_STATE}; + use crate::amount::Amount; + use crate::state::{ChannelState, CHANNEL_INFO, CHANNEL_STATE}; use crate::ContractError; - use cosmwasm_std::{Coin, DepsMut, Env, Order, StdResult}; + use cosmwasm_std::{to_binary, Addr, DepsMut, Env, Order, StdResult, WasmQuery}; + use cw20::{BalanceResponse, Cw20QueryMsg}; - pub fn update_balances(deps: DepsMut, env: &Env) -> Result<(), ContractError> { + pub fn update_balances(mut deps: DepsMut, env: &Env) -> Result<(), ContractError> { let channels = CHANNEL_INFO .keys(deps.storage, None, None, Order::Ascending) .collect::>>()?; @@ -34,17 +36,8 @@ pub mod v2 { .prefix(channel) .range(deps.storage, None, None, Order::Ascending) .collect::>>()?; - for (denom, mut state) in states.into_iter() { - // this checks if we have received some coins that are "in flight" and not yet accounted in the state - let Coin { amount, .. } = deps.querier.query_balance(addr, &denom)?; - let diff = amount - 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)?; - } + for (denom, state) in states.into_iter() { + update_denom(deps.branch(), addr, channel, denom, state)?; } Ok(()) } @@ -53,4 +46,44 @@ pub mod v2 { }), } } + + 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 msg = Cw20QueryMsg::Balance { + address: contract.into(), + }; + let query = WasmQuery::Smart { + contract_addr: coin.address, + msg: to_binary(&msg)?, + } + .into(); + let res: BalanceResponse = deps.querier.query(&query)?; + 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(()) + } } From cd6b88c5e9bf123e6263728012951eba9da54649 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Thu, 24 Mar 2022 19:15:32 +0100 Subject: [PATCH 6/6] Cleanup --- contracts/cw20-ics20/src/migrations.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/contracts/cw20-ics20/src/migrations.rs b/contracts/cw20-ics20/src/migrations.rs index 536b4da96..8c67b6b49 100644 --- a/contracts/cw20-ics20/src/migrations.rs +++ b/contracts/cw20-ics20/src/migrations.rs @@ -60,16 +60,14 @@ pub mod v2 { 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 msg = Cw20QueryMsg::Balance { - address: contract.into(), - }; + // `Cw20Contract(contract.clone()).balance(&deps.querier, contract)?` let query = WasmQuery::Smart { contract_addr: coin.address, - msg: to_binary(&msg)?, - } - .into(); - let res: BalanceResponse = deps.querier.query(&query)?; + msg: to_binary(&Cw20QueryMsg::Balance { + address: contract.into(), + })?, + }; + let res: BalanceResponse = deps.querier.query(&query.into())?; res.balance } };