Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Sumtree]: E2E and Fuzz Testing #191

Merged
merged 109 commits into from
Jul 5, 2024
Merged

[Sumtree]: E2E and Fuzz Testing #191

merged 109 commits into from
Jul 5, 2024

Conversation

crnbarr93
Copy link
Collaborator

@crnbarr93 crnbarr93 commented Jun 17, 2024

What is the purpose of the change

These changes introduce a testing suite for end-to-end testing of the sumtree orderbook. Testing uses osmosis-test-tube so that the use of the CosmWasm Pool module can simultaneously be tested. There are currently two types of e2e tests:

  1. Case based testing (test_orders)
  2. Fuzz testing (test_fuzz)

The case based testing uses predefined values and expected outputs in order to ensure basic usage of the orderbook functions as intended. The fuzz testing suite attempts to randomly place a limit, claim a limit, cancel a limit or place a market order depending on the type of fuzz test being run. There are two types of fuzz tests:

  1. Linear - places a set amount of limit orders, fills all possible liquidity and then attempts to claim/cancel the remaining orders.
  2. Mixed - randomly chooses an operation and attempts to run it, a run may (expectedly) fail in which case we choose another operation.

There are many different tests for both of these types that require various inputs (such as amount of operations, tick ranges, cancel probability, etc). These are separate so that they may run asynchronously.

To help with both case based and fuzz testing a utils file is defined. This provides a setup macro and two modules; assert and order, both providing relevant utility methods. The more complicated assertion logic is in balance_changes which takes in a desired action, records balances for the provided addresses before and after the action and compares the difference.

One short coming of this testing suite is that testing the maker fee/admin functions is not possible due to a restriction with test-tube. Testing can be done by removing authorisation checks in the admin functions and having any address update the values but this is a risky adjustment and must be reverted before pushing any code.

Included is also an additional query for orderbook state and an adjustment for inclusion when querying orders.

Note: the contract must be compiled via cargo wasm after any adjustments before running these tests

Testing and Verifying

All tests can be run using:

cargo wasm
cargo unit-test test_fuzz
cargo unit-test test_orders

These tests can be skipped by running:

cargo unit-test --workspace --features skip-integration-tests

Testing Outcomes

End-to-end testing caught the following issues:

  1. We were not syncing ticks before cancellations
  2. Our sumtree logic did not include batch realization
  3. The price logic was inverted
  4. Empty ticks were being included in market order logic (although not adding/removing anything)

crnbarr93 added 30 commits June 14, 2024 15:31
@crnbarr93 crnbarr93 requested a review from AlpinYukseloglu July 3, 2024 11:48
@crnbarr93 crnbarr93 marked this pull request as ready for review July 3, 2024 12:10
Copy link
Collaborator

@AlpinYukseloglu AlpinYukseloglu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this -- caught several important issues. The tests themselves LGTM, as do the fixes (which should already be merged in separately right?)

@crnbarr93
Copy link
Collaborator Author

Great work on this -- caught several important issues. The tests themselves LGTM, as do the fixes (which should already be merged in separately right?)

Yep!

Changes from last weekend are here:

https://github.com/osmosis-labs/orderbook/pull/191/files#diff-30a52a468ba41031642eaae9f080b4f28bf053a73ea4504df7c4e42b6894f008R149

@crnbarr93 crnbarr93 merged commit 72ce4b5 into main Jul 5, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants