Merge pull request #6 from oraichain/single-step

fixed single-step refund using shared amount bug

Author: ducphamle2

contracts/cw-ics20-latest/src/ibc.rs

@@ -13,8 +13,8 @@ use oraiswap::router::{SimulateSwapOperationsResponse, SwapOperation};
 use crate::error::{ContractError, Never};
 use crate::state::{
     get_key_ics20_ibc_denom, ics20_denoms, increase_channel_balance, reduce_channel_balance,
-    undo_increase_channel_balance, undo_reduce_channel_balance, ChannelInfo, MappingMetadata,
-    ReplyArgs, SingleStepReplyArgs, ALLOW_LIST, CHANNEL_INFO, CONFIG, REPLY_ARGS,
+    undo_increase_channel_balance, undo_reduce_channel_balance, ChannelInfo, IbcSingleStepData,
+    MappingMetadata, ReplyArgs, SingleStepReplyArgs, ALLOW_LIST, CHANNEL_INFO, CONFIG, REPLY_ARGS,
     SINGLE_STEP_REPLY_ARGS,
 };
 use cw20::{Cw20ExecuteMsg, Cw20QueryMsg, TokenInfoResponse};
@@ -618,9 +618,9 @@ pub fn build_ibc_msg(
     let mut reply_args = SingleStepReplyArgs {
         channel: destination.destination_channel.clone(),
         refund_asset_info: receiver_asset_info.clone(),
-        ibc_denom: None,
+        ibc_data: None,
         receiver: local_receiver.to_string(),
-        amount,
+        local_amount: amount,
     };
     let (is_evm_based, destination) = destination.is_receiver_evm_based();
     if is_evm_based {
@@ -642,15 +642,15 @@ pub fn build_ibc_msg(
             .into_iter()
             .find(|(key, _)| key.contains(&destination.destination_channel));
         if let Some(pair_mapping) = pair_mappings {
-            let amount = convert_local_to_remote(
+            let remote_amount = convert_local_to_remote(
                 amount,
                 pair_mapping.1.remote_decimals,
                 pair_mapping.1.asset_info_decimals,
             )?;
 
             // build ics20 packet
             let packet = Ics20Packet::new(
-                amount.clone(),
+                remote_amount.clone(),
                 pair_mapping.0.clone(), // we use ibc denom in form <transfer>/<channel>/<denom> so that when it is sent back to remote chain, it gets parsed correctly and burned
                 env.contract.address.as_str(),
                 &remote_address,
@@ -661,12 +661,15 @@ pub fn build_ibc_msg(
                 storage,
                 &local_channel_id.clone(),
                 &pair_mapping.0.clone(),
-                amount,
+                remote_amount,
                 false,
             )
             .map_err(|err| StdError::generic_err(err.to_string()))?;
             reply_args.channel = local_channel_id.to_string();
-            reply_args.ibc_denom = Some(pair_mapping.0);
+            reply_args.ibc_data = Some(IbcSingleStepData {
+                ibc_denom: pair_mapping.0,
+                remote_amount,
+            });
             // keep track of the reply. We need to keep a seperate value because if using REPLY, it could be overriden by the channel increase later on
             SINGLE_STEP_REPLY_ARGS.save(storage, &reply_args)?;
 
@@ -697,22 +700,27 @@ pub fn handle_follow_up_failure(
     err: String,
 ) -> Result<Response, ContractError> {
     // if there's an error but no ibc msg aka no channel balance reduce => wont undo reduce
-    if let Some(ibc_denom) = reply_args.ibc_denom {
+    let mut response: Response = Response::new();
+    if let Some(ibc_data) = reply_args.ibc_data {
         undo_reduce_channel_balance(
             storage,
             &reply_args.channel,
-            &ibc_denom,
-            reply_args.amount,
+            &ibc_data.ibc_denom,
+            ibc_data.remote_amount,
             false,
         )?;
+        response = response.add_attributes(vec![
+            attr("ibc_denom_undo_channel_balance", ibc_data.ibc_denom),
+            attr("remote_amount_undo_channel_balance", ibc_data.remote_amount),
+        ]);
     }
     let refund_amount = Amount::from_parts(
         parse_asset_info_denom(reply_args.refund_asset_info.clone()),
-        reply_args.amount,
+        reply_args.local_amount,
     );
     // we send refund to the local receiver of the single-step tx because the funds are currently in this contract
     let send = send_amount(refund_amount, reply_args.receiver, None);
-    Ok(Response::new()
+    response = response
         .add_submessage(SubMsg::reply_on_error(send, REFUND_FAILURE_ID))
         .set_data(ack_fail(err.clone()))
         .add_attributes(vec![
@@ -721,8 +729,9 @@ pub fn handle_follow_up_failure(
                 "attempt_refund_denom",
                 reply_args.refund_asset_info.to_string(),
             ),
-            attr("attempt_refund_amount", reply_args.amount),
-        ]))
+            attr("attempt_refund_amount", reply_args.local_amount),
+        ]);
+    Ok(response)
 }
 
 pub fn check_gas_limit(deps: Deps, amount: &Amount) -> Result<Option<u64>, ContractError> {

contracts/cw-ics20-latest/src/ibc_tests.rs

@@ -19,11 +19,11 @@ mod test {
 
     use crate::error::ContractError;
     use crate::state::{
-        get_key_ics20_ibc_denom, increase_channel_balance, ChannelState, SingleStepReplyArgs,
-        CHANNEL_REVERSE_STATE, SINGLE_STEP_REPLY_ARGS,
+        get_key_ics20_ibc_denom, increase_channel_balance, ChannelState, IbcSingleStepData,
+        SingleStepReplyArgs, CHANNEL_REVERSE_STATE, SINGLE_STEP_REPLY_ARGS,
     };
     use cw20::{Cw20Coin, Cw20ExecuteMsg};
-    use cw20_ics20_msg::amount::Amount;
+    use cw20_ics20_msg::amount::{convert_local_to_remote, Amount};
 
     use crate::contract::{execute, migrate, query_channel};
     use crate::msg::{ExecuteMsg, MigrateMsg, TransferMsg, UpdatePairMsg};
@@ -638,6 +638,10 @@ mod test {
             denom: "orai".to_string(),
         };
         let amount = Uint128::from(10u128);
+        let remote_decimals = 18;
+        let asset_info_decimals = 6;
+        let remote_amount =
+            convert_local_to_remote(amount, remote_decimals, asset_info_decimals).unwrap();
         let remote_address = "eth-mainnet0x1235";
         let mut env = mock_env();
         env.contract.address = Addr::unchecked("addr");
@@ -714,8 +718,8 @@ mod test {
             local_channel_id: "mars-channel".to_string(),
             denom: "trx-mainnet".to_string(),
             asset_info: receiver_asset_info.clone(),
-            remote_decimals: 18,
-            asset_info_decimals: 18,
+            remote_decimals,
+            asset_info_decimals,
         };
 
         // works with proper funds
@@ -731,7 +735,7 @@ mod test {
             deps.as_mut().storage,
             receive_channel,
             pair_mapping_key.as_str(),
-            amount.clone(),
+            remote_amount.clone(),
             false,
         )
         .unwrap();
@@ -755,7 +759,7 @@ mod test {
             CosmosMsg::Ibc(IbcMsg::SendPacket {
                 channel_id: receive_channel.to_string(),
                 data: to_binary(&Ics20Packet::new(
-                    amount.clone(),
+                    remote_amount.clone(),
                     pair_mapping_key.clone(),
                     env.contract.address.as_str(),
                     &remote_address,
@@ -766,9 +770,11 @@ mod test {
             })
         );
         let reply_args = SINGLE_STEP_REPLY_ARGS.load(deps.as_mut().storage).unwrap();
-        assert_eq!(reply_args.amount, amount);
+        let ibc_data = reply_args.ibc_data.unwrap();
+        assert_eq!(ibc_data.remote_amount, remote_amount);
+        assert_eq!(reply_args.local_amount, amount);
         assert_eq!(reply_args.channel, receive_channel);
-        assert_eq!(reply_args.ibc_denom, Some(pair_mapping_key));
+        assert_eq!(ibc_data.ibc_denom, pair_mapping_key);
         assert_eq!(reply_args.receiver, local_receiver.to_string());
         assert_eq!(reply_args.refund_asset_info, receiver_asset_info)
     }
@@ -866,8 +872,8 @@ mod test {
         let mut single_step_reply_args = SingleStepReplyArgs {
             channel: local_channel_id.to_string(),
             refund_asset_info: refund_asset_info.clone(),
-            ibc_denom: None,
-            amount: amount.clone(),
+            ibc_data: None,
+            local_amount: amount,
             receiver: receiver.to_string(),
         };
         let result = handle_follow_up_failure(
@@ -894,12 +900,16 @@ mod test {
                         "attempt_refund_denom",
                         single_step_reply_args.refund_asset_info.to_string(),
                     ),
-                    attr("attempt_refund_amount", single_step_reply_args.amount),
+                    attr("attempt_refund_amount", single_step_reply_args.local_amount),
                 ])
         );
 
         let ibc_denom = "ibc_denom";
-        single_step_reply_args.ibc_denom = Some(ibc_denom.to_string());
+        let remote_amount = convert_local_to_remote(amount, 18, 6).unwrap();
+        single_step_reply_args.ibc_data = Some(IbcSingleStepData {
+            ibc_denom: ibc_denom.to_string(),
+            remote_amount: remote_amount.clone(),
+        });
         // if has ibc denom then it's evm based, need to undo reducing balance
         CHANNEL_REVERSE_STATE
             .save(
@@ -921,6 +931,6 @@ mod test {
             .load(deps.as_mut().storage, (local_channel_id, ibc_denom))
             .unwrap();
         // should undo reduce channel state
-        assert_eq!(channel_state.outstanding, amount)
+        assert_eq!(channel_state.outstanding, remote_amount)
     }
 }

contracts/cw-ics20-latest/src/state.rs

@@ -99,12 +99,18 @@ pub struct ReplyArgs {
     pub amount: Uint128,
 }
 
+#[cw_serde]
+pub struct IbcSingleStepData {
+    pub ibc_denom: String,
+    pub remote_amount: Uint128,
+}
+
 #[cw_serde]
 pub struct SingleStepReplyArgs {
     pub channel: String,
     pub refund_asset_info: AssetInfo,
-    pub ibc_denom: Option<String>,
-    pub amount: Uint128,
+    pub ibc_data: Option<IbcSingleStepData>,
+    pub local_amount: Uint128,
     pub receiver: String,
 }