StableRatedPool drink tests #17

JanKuczma · 2024-07-03T09:57:53Z

deuszx

Gone through the first iteration, leaving some comments.

I'm wondering – swap_exact_in and swap_received should be behaving in the exact same way, right? i.e. for the same amount_in they should be outputing the same amount_out. If that's true, it means they should share the test cases as well. It'd provide additional layer of verification.

amm/drink-tests/src/stable_swap_tests/tests_swap_received.rs

amm/drink-tests/src/stable_swap_tests/tests_add_remove_lp.rs

deuszx

I think there's another invariant that should hold for all swap types and adding/removing tokens:

amount of tokens should be constant, tokens should not disappear

For example:

For swaps, trader balance + pool reserves should be equivalent pre- and post- swap
For add/remove liquidity, trader balance + pool reserves should be equivalent pre- and post- add/remove liquidity.

This reverts commit 4559499.

deuszx · 2024-07-04T11:46:29Z

amm/drink-tests/src/stable_swap_tests/tests_getters.rs

+        300000 * ONE_LPT + 498999996725367 + 498999993395420, // -- DIFF -- 300000 * ONE_LPT + 499999996666583 + 499999993277742
+        "Incorrect LP token supply"
+    );
+}


MIssing newline at the end of the file.

deuszx · 2024-07-04T11:46:57Z

amm/drink-tests/src/stable_swap_tests/tests_getters.rs

+    );
+    assert_eq!(
+        stable_swap::fees(&mut session, stable_swap),
+        (25, 2000),


You could extract fees and amp_coef to vars and use here and in the constructor.

deuszx

Do you plan on merging tests_swap_exact_in and tests_swap_received into a single test file and asserting the two flows result in the same trade outputs?

amm/drink-tests/src/stable_swap_tests/tests_add_remove_lp.rs

deuszx · 2024-07-04T11:52:43Z

amm/drink-tests/src/stable_swap_tests/tests_add_remove_lp.rs

+        BOB,
+    );
+
+    _ = stable_swap::add_liquidity(


In setup_stable_swap_with_tokens it's hidden that it's BOB that gets all the "moni" and it increases the allownce on stable_swap. I'd extract that b/c - it's not helping with reviewing the code.

deuszx · 2024-07-04T14:50:42Z

amm/drink-tests/src/stable_swap_tests/tests_add_remove_lp.rs

+
+    assert_eq!(
+        share_price_and_total_shares(&mut session, stable_swap, None),
+        (last_share_price, last_total_shares + 1500 * ONE_LPT)


Do we need the last_share_price? What we're really checking is that reserves increased by 500 each and that there's 3*1500 more LP tokens. What I want to say is that we're really testing that reserves/LP tokens increase as expected. Maybe that's worth extracting to a standalone test? Maybe I'm overreacting but I like small and simple tests that test one thing only (if possible).

deuszx · 2024-07-04T14:57:25Z

amm/drink-tests/src/stable_swap_tests/tests_add_remove_lp.rs

+        BOB,
+    );
+
+    // add more liquidity with imbalanced tokens (dave)


This could be a separate test – for managing liquidity w/ imbalanced token amounts.

deuszx · 2024-07-04T15:00:11Z

amm/drink-tests/src/stable_swap_tests/tests_add_remove_lp.rs

+    let (current_share_price, current_total_shares) =
+        share_price_and_total_shares(&mut session, stable_swap, None);
+    assert!(
+        current_share_price > last_share_price,


Does the share price increase when you add tokens in imbalanced amounts?

Yes, b/c when depositing in imbalanced amounts then fees apply, hence the value of one share in terms of tokens in the pool increases.

amm/drink-tests/src/stable_swap_tests/tests_add_remove_lp.rs

Test try withdrawing too much

woocash2 · 2024-07-15T12:21:17Z

amm/drink-tests/src/stable_swap_tests/tests_getters.rs

+        stable_swap,
+        BOB,
+        tokens[0], // DAI
+        tokens[1], // USDC


It should be USDT, right?

amm/drink-tests/src/stable_swap_tests/tests_getters.rs

woocash2 · 2024-07-15T16:02:34Z

amm/drink-tests/src/stable_swap_tests/tests_rated.rs

+const ONE_WAZERO: u128 = 10u128.pow(WAZERO_DEC as u32);
+const ONE_SAZERO: u128 = 10u128.pow(SAZERO_DEC as u32);
+
+const EXPIRE_TS: u64 = 24 * 3600 * 1000; // 24h


What TS is supposed to mean? I see it's in millis so maybe EXPIRE_TIME_MILLIS?

woocash2 · 2024-07-15T16:05:19Z

amm/drink-tests/src/stable_swap_tests/tests_rated.rs

+        vec![50000 * ONE_SAZERO, 100000 * ONE_WAZERO],
+        charlie(),
+    )
+    .expect("Should successfully swap");


Suggested change

.expect("Should successfully swap");

.expect("Should successfully add liquidity");

woocash2 · 2024-07-15T16:07:48Z

amm/drink-tests/src/stable_swap_tests/tests_rated.rs

+        vec![1 * ONE_SAZERO, 1 * ONE_WAZERO],
+        charlie(),
+    )
+    .expect("Should successfully swap");


Suggested change

.expect("Should successfully swap");

.expect("Should successfully remove liquidity");

woocash2 · 2024-07-15T16:12:20Z

amm/drink-tests/src/stable_swap_tests/tests_rated.rs

+        vec![100000 * ONE_WAZERO, 50000 * ONE_WAZERO, 100000 * ONE_WAZERO],
+        bob(),
+    )
+    .expect("Should successfully add LP");


Suggested change

.expect("Should successfully add LP");

.expect("Should successfully add liquidity");

(many occurrences)

woocash2 · 2024-07-15T16:38:21Z

amm/drink-tests/src/stable_swap_tests/tests_rated.rs

+    );
+    assert_eq!(
+        psp22_utils::balance_of(&mut session, tokens[1], charlie()),
+        997499999501,


woocash2 · 2024-07-15T16:39:08Z

amm/drink-tests/src/stable_swap_tests/tests_rated.rs

+    );
+    assert_eq!(
+        stable_swap::reserves(&mut session, rated_swap),
+        vec![100001 * ONE_WAZERO, 99999 * ONE_WAZERO + 2500000499],


woocash2 · 2024-07-15T17:02:40Z

amm/drink-tests/src/stable_swap_tests/tests_swap_exact_in.rs

+        vec![6, 6],                       // decimals
+        vec![100000000000, 100000000000], // initial reserves
+        1000,                             // A
+        600_000,                          // fee BPS


Our fees have 1e9 precision, so BPS is not a good word here, right?

woocash2 · 2024-07-15T17:06:02Z

amm/drink-tests/src/stable_swap_tests/tests_swap_exact_out.rs

+    T: Add + Sub + Copy + PartialEq + PartialOrd + Eq + Ord + std::fmt::Display,
+    <T as Sub>::Output: PartialOrd<T>,
+{
+    if a > b {


Can't we simplify to |a - b| > delta?
Oh, I suppose not necessarily, because of the generic types

woocash2 · 2024-07-15T17:13:29Z

amm/drink-tests/src/stable_swap_tests/tests_swap_exact_out.rs

+    )
+    .expect("swap_exact_out failed");
+
+    let swap_exact_in_result = stable_swap::swap_exact_in(


very clever to use swap_exact_in for verification 🙂

woocash2

LGTM!

JanKuczma added 4 commits July 3, 2024 01:03

wip

6f9e5b4

add ref to tests

9c43c4b

add comments on differences

8dc6989

rename tests

110128a

JanKuczma requested a review from deuszx July 3, 2024 09:57

deuszx reviewed Jul 3, 2024

View reviewed changes

amm/drink-tests/src/stable_swap_tests/tests_add_remove_lp.rs Outdated Show resolved Hide resolved

deuszx reviewed Jul 3, 2024

View reviewed changes

JanKuczma added 8 commits July 3, 2024 17:52

mock rate contract refactor

4559499

Revert "mock rate contract refactor"

ee0bfb2

This reverts commit 4559499.

Merge branch 'main' into drink-tests

fbcba92

fix share price calculation

82a9180

additional swap tests

b76ef43

split tests

b214984

ensure reserves updated properly

b63808a

test rated wip

83aecb8

JanKuczma mentioned this pull request Jul 4, 2024

Minimum reserves check #19

Closed

Merge branch 'main' into drink-tests

8092ec7

deuszx reviewed Jul 4, 2024

View reviewed changes

amm/drink-tests/src/stable_swap_tests/tests_add_remove_lp.rs Outdated Show resolved Hide resolved

deuszx reviewed Jul 4, 2024

View reviewed changes

amm/drink-tests/src/stable_swap_tests/tests_add_remove_lp.rs Outdated Show resolved Hide resolved

JanKuczma added 3 commits July 5, 2024 13:21

fmt

f59a452

fix helper function

07f687f

test rated part 1

d327786

JanKuczma and others added 10 commits July 9, 2024 17:07

adjust expected results in test due to change in fee precision

3aaba7c

allow salt in test contracts deployment

3531956

cross tests for swap_exact_out

4b6d765

Merge branch 'main' into drink-tests

6b6cdfc

wip - test lp withdraw getter

23a9466

tests expoit add-remove liqudity

0506eee

try exploit test

2dafd4a

add some comments

62b4a24

Merge branch 'drink-tests' into drink-test-try-exploit

1b82a05

Merge pull request #29 from Cardinal-Cryptography/drink-test-try-exploit

4be58d5

Test try withdrawing too much

JanKuczma marked this pull request as ready for review July 15, 2024 07:36

Merge branch 'main' into drink-tests

a4bbcd4

JanKuczma requested a review from woocash2 July 15, 2024 09:42

woocash2 reviewed Jul 15, 2024

View reviewed changes

amm/drink-tests/src/stable_swap_tests/tests_getters.rs Show resolved Hide resolved

fix typo in comment

28e6ac1

woocash2 reviewed Jul 15, 2024

View reviewed changes

woocash2 approved these changes Jul 15, 2024

View reviewed changes

JanKuczma added 2 commits July 15, 2024 21:11

update old comments

c4c7b00

combine swap_exact_in and swap_received tests

6b4bed8

JanKuczma merged commit 60f7e5f into main Jul 15, 2024
1 check passed

JanKuczma deleted the drink-tests branch July 15, 2024 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StableRatedPool drink tests #17

StableRatedPool drink tests #17

JanKuczma commented Jul 3, 2024 •

edited

Loading

deuszx left a comment

deuszx left a comment

deuszx Jul 4, 2024

deuszx Jul 4, 2024

deuszx left a comment

deuszx Jul 4, 2024 •

edited

Loading

deuszx Jul 4, 2024

deuszx Jul 4, 2024

deuszx Jul 4, 2024

JanKuczma Jul 8, 2024

woocash2 Jul 15, 2024

woocash2 Jul 15, 2024

woocash2 Jul 15, 2024

woocash2 Jul 15, 2024

woocash2 Jul 15, 2024 •

edited

Loading

woocash2 Jul 15, 2024

woocash2 Jul 15, 2024

woocash2 Jul 15, 2024

woocash2 Jul 15, 2024 •

edited

Loading

woocash2 Jul 15, 2024

woocash2 left a comment

	.expect("Should successfully swap");
	.expect("Should successfully add liquidity");

	.expect("Should successfully swap");
	.expect("Should successfully remove liquidity");

	.expect("Should successfully add LP");
	.expect("Should successfully add liquidity");

StableRatedPool drink tests #17

StableRatedPool drink tests #17

Conversation

JanKuczma commented Jul 3, 2024 • edited Loading

deuszx left a comment

Choose a reason for hiding this comment

deuszx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deuszx left a comment

Choose a reason for hiding this comment

deuszx Jul 4, 2024 • 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

woocash2 Jul 15, 2024 • 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

woocash2 Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woocash2 left a comment

Choose a reason for hiding this comment

JanKuczma commented Jul 3, 2024 •

edited

Loading

deuszx Jul 4, 2024 •

edited

Loading

woocash2 Jul 15, 2024 •

edited

Loading

woocash2 Jul 15, 2024 •

edited

Loading