feat: Add ut to all helper functions #66

apollo-pablo · 2022-11-16T17:04:00Z

Closes #54

Problem

We dont have any test on math functions.

Tasks

Add UT to implementations Helpers functions.
Add on every file instead of new files.

Notes

Format and lints failures are from develop branch, we should make a PR and fix them first.

Signed-off-by: apollo-pablo <[email protected]>

pacmanifold

nice work! 👍 a few comments @apollo-pablo

src/implementations/astroport/helpers.rs

pacmanifold · 2022-11-17T14:42:10Z

src/implementations/astroport/helpers.rs

+            // do we need clone here since the reference d_product is mut? -> d.clone()
            let mut d_product = d;


i don't think this should affect the value of d even though d_product is mut. U256 implements the Copy trait

removed the todo

pacmanifold · 2022-11-17T14:44:14Z

src/implementations/astroport/helpers.rs

+/// 
+/// [Paper](https://curve.fi/files/stableswap-paper.pdf)
+/// [playground](https://github.com/asquare08/AMM-Models/blob/main/Curve%20AMM%20plots.ipynb)


this is just a copy from astroports code. Do they reference this? or do they implement it exactlyAs described there? otherwise this might be confusing

Why would be confusing if it gives more information? its describe the formula and the second link is a playground to test things. IMO i would left it as is more helpfull than the eq described in plain text.

leave the description as original

pacmanifold · 2022-11-17T14:46:02Z

src/implementations/astroport/helpers.rs

-            let amp_range = next_amp - init_amp;
-            let res = init_amp + (amp_range * elapsed_time).checked_div(time_range)?;
-            Ok(res.u128() as u64)
+            let amp_range = next_amp.checked_sub(init_amp)?;
+            let res = init_amp
+                .checked_add((amp_range.checked_mul(elapsed_time))?.checked_div(time_range)?)?;
+            // Ok(res.u128() as u64) -> downgrading from u128 to u64 implies loose conversion
+            Ok(u64::try_from(res.u128()).unwrap())
        } else {
-            let amp_range = init_amp - next_amp;
-            let res = init_amp - (amp_range * elapsed_time).checked_div(time_range)?;
-            Ok(res.u128() as u64)
+            let amp_range = init_amp.checked_sub(next_amp)?;
+            let res = init_amp
+                .checked_sub((amp_range.checked_mul(elapsed_time))?.checked_div(time_range)?)?;
+            // Ok(res.u128() as u64) -> downgrading from u128 to u64 implies loose conversion
+            Ok(u64::try_from(res.u128()).unwrap())


since this is a copy of astroports code i think we should keep it as is. If it fails or has rounding errors that will happen on astro also andAll we want is to exactly emulate their behaviour

OK, but keep in mind that they change the whole code in their master, so they did it for a reason. They also create a simulator to test the new code , so my guess is that the old code doesnt work (that why test fails) and refactor to a new code that can be tested. astroport stabe-swap simulator

Do we have someone from astroport to talk to about this?

leave the code as original, mark test as failure in code comments. I think if we want to use the original code should be reviewed and tested properly using their simulator. On our ut it fails.

src/implementations/astroport/helpers.rs

src/implementations/astroport/helper_test.rs

Signed-off-by: apollo-pablo <[email protected]>

…lpers files Signed-off-by: apollo-pablo <[email protected]>

apollo-sturdy · 2022-11-21T10:32:34Z

Cargo.toml

+# TODO: is it safe to use a beta, unaudited code?
 wasmswap = { git = "https://github.com/Wasmswap/wasmswap-contracts.git", tag = "v1.1.0-beta", features = ["library"] }


We're not sure which versions of wasmswap are live on mainnet actually. We have asked their developers but not yet received a response. There is an open issue. Please remove comment.

Suggested change

# TODO: is it safe to use a beta, unaudited code?

wasmswap = { git = "https://github.com/Wasmswap/wasmswap-contracts.git", tag = "v1.1.0-beta", features = ["library"] }

wasmswap = { git = "https://github.com/Wasmswap/wasmswap-contracts.git", tag = "v1.1.0-beta", features = ["library"] }

apollo-sturdy · 2022-11-21T10:48:33Z

Cargo.toml

+# core cosmwasm
+cosmwasm-schema = "1.1"
+# standard libs
+anyhow = "1"


What is this used for?

apollo-sturdy · 2022-11-21T10:51:22Z

src/implementations/junoswap/helpers.rs

@@ -193,6 +193,7 @@ pub(crate) fn juno_get_token2_amount_required(
    token2_reserve: Uint128,
    token1_reserve: Uint128,
 ) -> Result<Uint128, CwDexError> {
+    // TODO: on master they handle errors https://github.com/Wasmswap/wasmswap-contracts/blob/main/src/contract.rs#L222-L240


The question mark operator does the same as their map_err. Remove TODO comment.

apollo-sturdy · 2022-11-21T10:59:06Z

src/implementations/junoswap/helpers.rs

@@ -212,6 +213,7 @@ pub(crate) fn juno_get_token1_amount_required(
    token2_reserve: Uint128,
    token1_reserve: Uint128,
 ) -> Result<Uint128, CwDexError> {
+    // TODO: why liquidity_supply is not used here?


I'm not sure even why they use it in juno_get_token2_amount_required, I guess because they are dividing by token1_reserve there, but then they should be checking if that value is zero and not liquidity_supply (even though they would likely be zero at the same time). Regardless, if you look at the code which calls juno_get_token1_amount_required you will see that token2_reserve which we divide with cannot be zero, but perhaps defensive coding here would be good. Feel free to add it if you want and pls remove TODO comment.

removed the TODO.

apollo-sturdy · 2022-11-21T10:59:57Z

src/implementations/junoswap/helpers.rs

@@ -252,6 +261,7 @@ pub(crate) fn juno_simulate_provide_liquidity(
    let token1_to_use;
    let token2_to_use;

+    // TODO: What happens if pool_ratio == asset_ratio?


It will go into the else statement. I don't see an issue here. Can you elaborate on what the problem is? Please remove TODO comment.

If pool_ratio == asset_ratio then it goes by the else case.
The description of the else case is

// We have a higher ratio of token 2 than token1, so calculate how much // token2 to use (and approve spend for, since we don't want to approve // spend on any extra).

This case doesnt implies that token2 > token1 -> for example suppose:

pool_ratio = 1.05 and asset_ratio = 1.05, meaning that the pool is unbalance and the relation between token1 and token2 to deposit is unbalance

for asset_ratio token1/token2 = 1.05 implies that token1 is greater than token2

apollo-sturdy · 2022-11-21T11:01:10Z

src/implementations/astroport/helpers.rs

+                    // Equality with the precision of 1
            if d == d_previous {
-                break;
-            }
+                        break;
+                    }


Why add extra spacing? Pls use rustfmt.

apollo-sturdy · 2022-11-21T11:06:00Z

src/implementations/junoswap/helpers.rs

+/// ### Note
+/// This function does not take into account protocol_fee_percent and lp_fee_percent


Neither does Junoswap, so why mention it? Please remove comment.

removed, but why they added it to the smart contract if they are not going to use it?

apollo-sturdy · 2022-11-21T11:06:26Z

src/implementations/junoswap/helpers.rs

+/// ### Requirements
+/// - pool_info.token2_reserve > 0
+/// - token2.amount > 0


Not sure what this means. Can you please either remove this or explain it further so its understandable?

I explained a little better. The idea of requirements is that a developer should just by seen the description knows what range the function parameters should be as normal path and is great for testers since constrains the SUT. We should add requirements in the future to our descriptions.

apollo-sturdy · 2022-11-21T11:12:15Z

src/implementations/junoswap/helpers.rs

+            token1_denom: usdc.info.0,
+            token2_reserve: Uint128::from(reserve_b),
+            token2_denom: dai.info.0,
+            lp_token_supply: usdc.amount + dai.amount, // should this be reserve_a+reserve_b?


Shouldn't matter what this value is. Remove comment.

apollo-sturdy · 2022-11-21T11:16:26Z

src/implementations/junoswap/helpers.rs

+    #[test_case(0,1,0,1 => Ok( (0,0,0) ); "when amount_a and amount_b zero should work")]
+    #[test_case(30,1,1,1 => with |i: Result<(u128,u128,u128),StdError> | assert!(i.unwrap().2 == 1u128); "when asset_ratio gt pool_ratio with amount_a gt amount_b")] // TODO: what happens with the rest of amount_a? 
+    #[test_case(1,2,1,2 => Ok( (1,2,1) ); "when pool_ratio greater than asset_ratio")]
+    fn juno_simulate_provide_liquidity_test(


Some of these tests fail. Please fix before request review.

I fix them, but what happens if someone deposit amount_a = 0 and amount_b = 0 in a balance pool = 1? the result is token2_to_use=1 :
#[test_case(0,1,0,1 => Ok( (0,1,0) ); "when amount_a and amount_b zero should work?")]

Where:

input

amount_a: u128, reserve_a: u128, amount_b: u128, reserve_b: u128

output

result.token1_to_use.amount.u128(), result.token2_to_use.amount.u128(), result.lp_token_expected_amount.u128(),

Signed-off-by: apollo-pablo <[email protected]>

feat: Add ut to astroport helper

74ee54b

Signed-off-by: apollo-pablo <[email protected]>

apollo-pablo changed the title ~~feat: Add ut to astroport helper~~ Draft: feat: Add ut to astroport helper Nov 16, 2022

Merge branch 'develop' into feat-add-ut-to-helpers

e808e16

apollo-pablo self-assigned this Nov 16, 2022

apollo-pablo added the enhancement New feature or request label Nov 16, 2022

apollo-pablo changed the title ~~Draft: feat: Add ut to astroport helper~~ Draft: feat: Add ut to all helper functions Nov 16, 2022

apollo-pablo and others added 2 commits November 17, 2022 14:52

Merge branch 'develop' into feat-add-ut-to-helpers

027f648

feat: Add more ut to astroport impl

7999775

Signed-off-by: apollo-pablo <[email protected]>

pacmanifold marked this pull request as draft November 17, 2022 14:33

pacmanifold reviewed Nov 17, 2022

View reviewed changes

apollo-pablo and others added 3 commits November 17, 2022 19:14

feat: Add junoswap ut

f12aee8

Signed-off-by: apollo-pablo <[email protected]>

Merge branch 'develop' into feat-add-ut-to-helpers

86aa923

fix: leave astroport stableswap as original. move all tests inside he…

94d93b9

…lpers files Signed-off-by: apollo-pablo <[email protected]>

apollo-pablo changed the title ~~Draft: feat: Add ut to all helper functions~~ feat: Add ut to all helper functions Nov 18, 2022

apollo-pablo requested review from dirtyshab and georgemc98 November 18, 2022 11:13

apollo-pablo marked this pull request as ready for review November 18, 2022 11:14

apollo-sturdy requested changes Nov 21, 2022

View reviewed changes

apollo-pablo and others added 3 commits November 23, 2022 11:33

fix: some new reviews

097bafa

Signed-off-by: apollo-pablo <[email protected]>

Merge branch 'develop' into feat-add-ut-to-helpers

f829550

fix: minor space fmt

6776721

Signed-off-by: apollo-pablo <[email protected]>

pacmanifold closed this Feb 13, 2023

pacmanifold deleted the feat-add-ut-to-helpers branch February 13, 2023 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add ut to all helper functions #66

feat: Add ut to all helper functions #66

apollo-pablo commented Nov 16, 2022 •

edited by apollo-sturdy

Loading

pacmanifold left a comment •

edited

Loading

pacmanifold Nov 17, 2022

apollo-pablo Nov 18, 2022

pacmanifold Nov 17, 2022

apollo-pablo Nov 18, 2022

apollo-pablo Nov 18, 2022

pacmanifold Nov 17, 2022

apollo-pablo Nov 18, 2022

apollo-pablo Nov 18, 2022

apollo-pablo Nov 18, 2022

apollo-sturdy Nov 21, 2022

apollo-pablo Nov 23, 2022

apollo-sturdy Nov 21, 2022

apollo-pablo Nov 23, 2022

apollo-pablo Nov 23, 2022

apollo-sturdy Nov 21, 2022

apollo-pablo Nov 23, 2022

apollo-sturdy Nov 21, 2022

apollo-pablo Nov 23, 2022

apollo-sturdy Nov 21, 2022

apollo-pablo Nov 23, 2022 •

edited

Loading

apollo-sturdy Nov 21, 2022

apollo-pablo Nov 23, 2022

apollo-sturdy Nov 21, 2022

apollo-pablo Nov 23, 2022 •

edited

Loading

apollo-sturdy Nov 21, 2022

apollo-pablo Nov 23, 2022

apollo-sturdy Nov 21, 2022

apollo-pablo Nov 23, 2022 •

edited

Loading

apollo-sturdy Nov 21, 2022

apollo-pablo Nov 23, 2022 •

edited

Loading

		// do we need clone here since the reference d_product is mut? -> d.clone()
		let mut d_product = d;

		# TODO: is it safe to use a beta, unaudited code?
		wasmswap = { git = "https://github.com/Wasmswap/wasmswap-contracts.git", tag = "v1.1.0-beta", features = ["library"] }

		/// ### Note
		/// This function does not take into account protocol_fee_percent and lp_fee_percent

feat: Add ut to all helper functions #66

feat: Add ut to all helper functions #66

Conversation

apollo-pablo commented Nov 16, 2022 • edited by apollo-sturdy Loading

pacmanifold left a comment • edited Loading

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

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

apollo-pablo Nov 23, 2022 • edited Loading

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

apollo-pablo Nov 23, 2022 • edited Loading

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

apollo-pablo Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apollo-pablo Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

apollo-pablo commented Nov 16, 2022 •

edited by apollo-sturdy

Loading

pacmanifold left a comment •

edited

Loading

apollo-pablo Nov 23, 2022 •

edited

Loading

apollo-pablo Nov 23, 2022 •

edited

Loading

apollo-pablo Nov 23, 2022 •

edited

Loading

apollo-pablo Nov 23, 2022 •

edited

Loading