Merge pull request #684 from CosmWasm/fix-cw20-ics20-packets

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(())
+ }
+}