full feature review #1

czarcas7ic · 2023-09-14T02:51:48Z

No description provided.

czarcas7ic · 2023-09-14T02:54:11Z

src/execute.rs

+fn get_next_sqrt_price_from_amount0_in_round_up(
+    liquidity: Decimal256,
+    sqrt_price_current: Decimal256,
+    token_in: Decimal256,
+) -> Decimal256 {
+    let numerator = liquidity * sqrt_price_current;
+    let denominator = liquidity + (token_in * sqrt_price_current);
+    numerator / denominator
+}
+
+fn get_next_sqrt_price_from_amount1_in_round_down(
+    liquidity: Decimal256,
+    sqrt_price_current: Decimal256,
+    token_in: Decimal256,
+) -> Decimal256 {
+    sqrt_price_current + (token_in / liquidity)
+}


Note to reviewer: I don't know how to properly round Uint256 to mirror the logic in the osmosis codebase, so I left rounding logic off of these methods

Discussed that it's probably fine for v1 version of the contract. v2 can use similar tricks to uniswap math if ever needed

czarcas7ic · 2023-09-14T02:55:17Z

src/execute.rs

+    return sqrt_price * sqrt_price;
+}
+
+// fn iterative_naive_approach_new(


Note to reviewer: This was an initial attempt at iterating to get the perfect ratio, but after discussing with Dev I scrapped this for now

czarcas7ic · 2023-09-14T02:56:47Z

src/test_tube/mod.rs

+pub const UFOO: &'static str =
+    "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7";
+pub const UBAR: &'static str =
+    "ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2";


These denoms are used since they are authorized quote assets so I didn't need to fight with adding ufoo and ubar to the params. I did not use uosmo because we pay tx fees with uosmo, so it makes it simpler to calculate how much was actually used in the swap and position creation.

czarcas7ic · 2023-09-14T02:57:39Z

src/test_tube/tests/swap_and_join.rs

+    use prost::Message;
+
+    #[test]
+    fn test_single_sided_swap_and_join_amt_0_in() {


These tests do a good job to show how close we get with this naive approach. They aren't actually checking any inequalities, but print the resulting values so you can see how much is left over after all is done.

czarcas7ic · 2023-09-14T02:58:16Z

src/tick.rs

+pub const MIN_INITIALIZED_TICK: i64 = -108000000;
+pub const MAX_TICK: i128 = 342000000;
+
+// The methods in this file are copied from the Quasar cl vault contract.


This whole file was taken from Quasar's vault contract. That's not to say it shouldn't be reviewed, but wanted to highlight this.

czarcas7ic · 2023-09-14T02:59:45Z

src/contract.rs

+pub fn instantiate(
+    deps: DepsMut,
+    _env: Env,
+    _info: MessageInfo,
+    msg: InstantiateMsg,
+) -> Result<Response, ContractError> {
+    set_contract_version(deps.storage, CONTRACT_NAME, CONTRACT_VERSION)?;
+
+    let owner = deps.api.addr_validate(&msg.owner)?;
+    let state = Config { owner };
+
+    CONFIG.save(deps.storage, &state)?;
+    Ok(Response::new().add_attribute("method", "instantiate"))
+}


Currently contract owner doesn't have any special powers. I do think we should add the min tick, max tick, min spot price, max spot price constants as store values that can be changed by the contract owner, but that can be in the next iteration.

czarcas7ic · 2023-09-14T03:00:10Z

src/error.rs

@@ -0,0 +1,114 @@
+use cosmwasm_std::{


Most of these errors can be removed, I just started with a base one and worked off of it.

p0mvn · 2023-09-14T14:00:21Z

src/execute.rs

+    let original = pool.current_sqrt_price.to_string();
+    let parts: Vec<&str> = original.split('.').collect();
+    let integer_part = parts[0];
+    let fractional_part = &parts[1][0..18]; // Take only first 18 digits after the decimal point


We should make sure that we handle the min spot price lowering where it is allowed to have a spot price lower than 10^-18

I think it's fine to just fail in such a case since this is primarily for vaults. If they have a future need to support a pair with low spot price, we can refactor and upgrade the contract.

p0mvn · 2023-09-14T14:01:51Z

src/execute.rs

+    let fractional_part = &parts[1][0..18]; // Take only first 18 digits after the decimal point
+    let rounded = format!("{}.{}", integer_part, fractional_part);
+
+    let pool_cur_sqrt_price = Decimal256::from_str(&rounded).unwrap();


maybe we check to make sure it is not zero and fail if so to handle my comment above

p0mvn · 2023-09-15T01:00:28Z

src/execute.rs

+    if token_provided.denom == pool.token0.clone() {
+        if pool.current_tick > upper_tick {
+            // If the current tick is greater than the upper tick, we will swap the entire amount of token0 for token1
+            token_provided_swap_amount = token_provided.amount;
+        } else {
+            // Otherwise, we utilize the ratio of assets calculated above to determine how much of token0 to swap for token1
+            let token_provided_dec =
+                Decimal256::from_str(token_provided.amount.to_string().as_str())?;
+            token_provided_swap_amount = token_provided_dec
+                .checked_mul(asset_1_ratio.checked_add(spread_factor)?)?
+                .to_uint_floor()
+                .try_into()?;
+        }
+        token_out_denom = pool.token1.clone();
+        token_in = Coin {
+            denom: pool.token0.clone(),
+            amount: token_provided_swap_amount,
+        };
+    } else if token_provided.denom == pool.token1.clone() {
+        if pool.current_tick < lower_tick {
+            // If the current tick is less than the lower tick, we will swap the entire amount of token1 for token0
+            token_provided_swap_amount = token_provided.amount;
+        } else {
+            // Otherwise, we utilize the ratio of assets calculated above to determine how much of token1 to swap for token0
+            let token_provided_dec =
+                Decimal256::from_str(token_provided.amount.to_string().as_str())?;
+            token_provided_swap_amount = token_provided_dec
+                .checked_mul(asset_0_ratio.checked_add(spread_factor)?)?
+                .to_uint_floor()
+                .try_into()?;
+        }
+        token_out_denom = pool.token0.clone();
+        token_in = Coin {
+            denom: pool.token1.clone(),
+            amount: token_provided_swap_amount,
+        };


nit: we could probably reduce code duplication here by extracting a helper. Something like

pub fn helper(all_consumedl: bool, asset_ratio: type) -> ... if all_consumed{ token_provided_swap_amount = token_provided.amount; } else { for token0 let token_provided_dec = Decimal256::from_str(token_provided.amount.to_string().as_str())?; token_provided_swap_amount = token_provided_dec .checked_mul(asset_ratio.checked_add(spread_factor)?)? .to_uint_floor() .try_into()?; } token_out_denom = pool.token0.clone(); token_in = Coin { denom: pool.token1.clone(), amount: token_provided_swap_amount, }; // return }

p0mvn · 2023-09-15T01:04:33Z

src/execute.rs

+fn get_next_sqrt_price_from_amount0_in_round_up(
+    liquidity: Decimal256,
+    sqrt_price_current: Decimal256,
+    token_in: Decimal256,
+) -> Decimal256 {
+    let numerator = liquidity * sqrt_price_current;
+    let denominator = liquidity + (token_in * sqrt_price_current);
+    numerator / denominator
+}
+
+fn get_next_sqrt_price_from_amount1_in_round_down(
+    liquidity: Decimal256,
+    sqrt_price_current: Decimal256,
+    token_in: Decimal256,
+) -> Decimal256 {
+    sqrt_price_current + (token_in / liquidity)
+}


Discussed that it's probably fine for v1 version of the contract. v2 can use similar tricks to uniswap math if ever needed

p0mvn · 2023-09-15T01:06:20Z

src/execute.rs

+    Ok(delta_y)
+}
+
+pub fn calc_asset_ratio_from_ticks(


nit: comment would be helpful

p0mvn · 2023-09-15T01:39:04Z

src/execute.rs

+    }
+}
+
+fn iterative_naive_approach(


nit: instead of checking "if token_0_in" twice, suggest doing that once and extracting the common logic into a separate helper. That would help with readability

p0mvn · 2023-09-15T15:03:21Z

src/execute.rs

+    if token_0_in {
+        sqrt_price_next = get_next_sqrt_price_from_amount0_in_round_up(
+            Decimal256::from_str(pool.current_tick_liquidity.as_str()).unwrap(),
+            pool_cur_sqrt_price,
+            Decimal256::from_str(token_to_swap.amount.to_string().as_str()).unwrap(),
+        );
+    } else {
+        sqrt_price_next = get_next_sqrt_price_from_amount1_in_round_down(
+            Decimal256::from_str(pool.current_tick_liquidity.as_str()).unwrap(),
+            pool_cur_sqrt_price,
+            Decimal256::from_str(token_to_swap.amount.to_string().as_str()).unwrap(),
+        );
+    }


What if there are multiple initialized ticks over the range in the direction of the swap?

Then, we use the current tick liquidity to apply to them all but each bucket has different liquidity value.

I think we must use a liquidity net in direction query to correctly calculate the next sqrt price here

czarcas7ic and others added 13 commits September 7, 2023 11:24

first commit

13cdba8

inital push

7abac9d

initial push

f5438d2

naive approach that works

aef5d7f

minor clean up

e82544f

clean up

c35ddd0

Update README.md

25608f9

push initial test

333181b

add rust test

4c7646d

finish feat and tests

4e3ed0e

update README

09a9015

Merge branch 'czarcas7ic:main' into main

60cb44a

Merge branch 'main' into project

a2ddfcc

czarcas7ic commented Sep 14, 2023

View reviewed changes

czarcas7ic mentioned this pull request Sep 14, 2023

feat: Single-asset CL join contract for Skip/Quasar osmosis-labs/osmosis#6255

Open

p0mvn reviewed Sep 14, 2023

View reviewed changes

p0mvn reviewed Sep 15, 2023

View reviewed changes

remove target from git ignore

38cd4d1

p0mvn reviewed Sep 15, 2023

View reviewed changes

czarcas7ic added 2 commits September 19, 2023 12:44

tidy

41202a6

update test

7660a2d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

full feature review #1

full feature review #1

czarcas7ic commented Sep 14, 2023

czarcas7ic Sep 14, 2023

p0mvn Sep 15, 2023

czarcas7ic Sep 14, 2023

czarcas7ic Sep 14, 2023

czarcas7ic Sep 14, 2023

czarcas7ic Sep 14, 2023

czarcas7ic Sep 14, 2023

czarcas7ic Sep 14, 2023

p0mvn Sep 14, 2023

p0mvn Sep 14, 2023

p0mvn Sep 15, 2023

p0mvn Sep 15, 2023

p0mvn Sep 15, 2023

p0mvn Sep 15, 2023

p0mvn Sep 15, 2023

full feature review #1

Are you sure you want to change the base?

full feature review #1

Conversation

czarcas7ic commented Sep 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment