address Osmosis comments

contracts/pair_pcl/src/contract.rs

@@ -230,7 +230,7 @@ pub fn execute(
             max_spread,
             to,
             ..
-        } => execute_swap(deps, info, offer_asset, belief_price, max_spread, to),
+        } => execute_swap(deps, env, info, offer_asset, belief_price, max_spread, to),
         ExecuteMsg::WithdrawLiquidity { assets } => withdraw_liquidity(deps, env, info, assets),
         ExecuteMsg::UpdateConfig { params } => update_config(deps, env, info, params),
         ExecuteMsg::ProposeNewOwner { owner, expires_in } => {
@@ -604,6 +604,7 @@ fn withdraw_liquidity(
 
 fn execute_swap(
     deps: DepsMut,
+    env: Env,
     info: MessageInfo,
     offer_asset: Asset,
     belief_price: Option<Decimal>,
@@ -622,11 +623,12 @@ fn execute_swap(
         &SwapParams {
             belief_price,
             max_spread,
+            sender: info.sender,
             to: addr_opt_validate(deps.api, &to)?,
         },
     )?;
     let dispatch_swap_msg = MsgSwapExactAmountIn {
-        sender: info.sender.to_string(),
+        sender: env.contract.address.to_string(),
         routes: vec![SwapAmountInRoute {
             // If for some reason pool id was not set on instantiation any swap will fail which is totally safe.
             // Pool contract must know its pool id in Osmosis DEX module.

contracts/pair_pcl/src/queries.rs

@@ -151,8 +151,8 @@ pub fn query(deps: Deps, env: Env, msg: QueryMsg) -> StdResult<Binary> {
                 spot_price: spot_price.into(),
             })
         }
-        // TODO: PCL pools have dynamic fees which also depend on a swap size. Not sure whether we need to handle this query.
-        // TODO: However, Osmosis does this query prior calling sudo swap endpoint so I propose to leave default here.
+        // It was needed due to Osmosis legacy multi hop osmo swap fee reduction where it needs swap fee to pass into the swap interface.
+        // Osmosis confirmed we can safely set 0% here
         QueryMsg::GetSwapFee {} => to_binary(&GetSwapFeeResponse::default()),
         // TODO: there is no clear documentation how does it work
         QueryMsg::IsActive {} => to_binary(&IsActiveResponse { is_active: true }),

contracts/pair_pcl/src/state.rs

@@ -12,6 +12,7 @@ use cw_storage_plus::{Item, SnapshotMap};
 pub struct SwapParams {
     pub belief_price: Option<Decimal>,
     pub max_spread: Option<Decimal>,
+    pub sender: Addr,
     pub to: Option<Addr>,
 }
 

contracts/pair_pcl/src/sudo.rs

@@ -18,25 +18,29 @@ pub fn sudo(deps: DepsMut, env: Env, msg: SudoMessage) -> Result<Response, Contr
             token_out_min_amount,
             ..
         } => {
-            let sender = deps.api.addr_validate(&sender)?;
+            let mut sender = deps.api.addr_validate(&sender)?;
             let offer_asset = native_asset_info(token_in.denom).with_balance(token_in.amount);
 
             let mut belief_price = Some(Decimal::from_ratio(token_in.amount, token_out_min_amount));
-            // TODO: I assume Osmosis applies slippage on frontend side hence we won't disrupt
-            // TODO: this logic with our additional default 0.02% slippage tolerance
+            // Osmosis applies slippage on their frontend side hence we won't disrupt
+            // this logic with our additional default 0.02% slippage tolerance.
             let mut max_spread = Some(Decimal::zero());
             let mut to = None;
             // If swap was dispatched from Astroport pair it must have SWAP_PARAMS in the storage
             if let Some(swap_params) = SWAP_PARAMS.may_load(deps.storage)? {
                 belief_price = swap_params.belief_price;
                 max_spread = swap_params.max_spread;
+                sender = swap_params.sender;
                 to = swap_params.to;
-                // Remove params so they won't be used if SwapExactAmountIn is called directly from DEX module
+
+                // Remove params so they won't be used if SwapExactAmountIn is called directly from the DEX module
                 SWAP_PARAMS.remove(deps.storage);
             }
 
             let response_data = to_binary(&SwapExactAmountInResponseData {
-                token_out_amount: 1u8.into(), // TODO: does it matter if we send tokens ourselves?
+                // TODO: extract amount from the response. Osmosis: "It’s needed in case of multi pool swap routing"
+                // https://github.com/osmosis-labs/osmosis/blob/294302637a47ffec5cafc0c1953e88a54390b20e/x/poolmanager/router.go#L102C3-L113
+                token_out_amount: 1u8.into(),
             })?;
             internal_swap(deps, env, sender, offer_asset, belief_price, max_spread, to).map(|res| {
                 res.add_attribute("method", "swap_exact_amount_in")
@@ -45,6 +49,7 @@ pub fn sudo(deps: DepsMut, env: Env, msg: SudoMessage) -> Result<Response, Contr
         }
         SudoMessage::SwapExactAmountOut { .. } => {
             todo!("Unsafe function! Osmosis doesn't pull out expected coins from sender balance!")
+            // TODO: implement according to internal Osmosis logic described here https://github.com/osmosis-labs/osmosis/blob/294302637a47ffec5cafc0c1953e88a54390b20e/x/cosmwasmpool/pool_module.go#L272-L324
             /*let sender = deps.api.addr_validate(&sender)?;
             let ask_asset = native_asset_info(token_out.denom).with_balance(token_out.amount);
 

e2e_tests/tests/e2e_testing.rs

@@ -129,6 +129,17 @@ fn dex_swap_test() {
     assert_eq!(bar_bal, 0);
     assert_eq!(foo_bal, 994806);
 
+    // Direct swap via pair contract FOO -> BAR (which essentially proxies it to DEX module)
+    let asset = foo.with_balance(foo_bal);
+    helper
+        .swap_on_pair(&user, &pair_addr, &asset, None)
+        .unwrap();
+
+    let foo_bal = helper.coin_balance(&user.address().to_string(), &foo_denom);
+    let bar_bal = helper.coin_balance(&user.address().to_string(), &bar_denom);
+    assert_eq!(foo_bal, 0);
+    assert_eq!(bar_bal, 1984437);
+
     // TODO: this DEX endpoint is harmful!
     // Reverse swap via DEX: FOO -> BAR
     // let asset = foo.with_balance(1_000000u128);
@@ -143,18 +154,6 @@ fn dex_swap_test() {
     // let foo_bal = helper.coin_balance(&user2.address().to_string(), &foo_denom);
     // let bar_bal = helper.coin_balance(&user2.address().to_string(), &bar_denom);
     // dbg!(foo_bal, bar_bal);
-
-    // Direct swap via pair contract FOO -> BAR (which essentially proxies it to DEX module)
-    // TODO: currently throws an error "failed to execute message; message index: 0: dispatch: submessages: contract doesn't have permission: unauthorized"
-    // let asset = foo.with_balance(foo_bal);
-    // helper
-    //     .swap_on_pair(&user, &pair_addr, &asset, None)
-    //     .unwrap();
-    //
-    // let foo_bal = helper.coin_balance(&user.address().to_string(), &foo_denom);
-    // let bar_bal = helper.coin_balance(&user.address().to_string(), &bar_denom);
-    // assert_eq!(foo_bal, 0);
-    // assert_eq!(bar_bal, 1984437);
 }
 
 #[test]