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

Fix test by checking that new orders were created #4

Merged
merged 4 commits into from
Oct 9, 2024
Merged

Conversation

fedgiac
Copy link

@fedgiac fedgiac commented Oct 8, 2024

The current on-chain test fails as it is on main.
It also has the following issues:

  • It runs by executing actual transactions on Görli, but that network has been shut down.
  • The Milkman contract uses hardcoded mainnet addresses, so it's designed to only work on mainnet. Deploying on a testnet is just a workaround to make this very test work.
  • It just checks that at some point some swaps have been created on the test instance of Milkman, without any link to any real contracts.
  • Some parts of the tests are unrelated to the code in this repo (why test for the UNI balance on Görli?).

I made the following changes to the test.

  • It's run on a mainnet fork.
  • It uses the actual deployment address(es) of Milkman.
  • It tries to create a swap in the fork and only considers the swap event generated in the fork.
  • It verifies that the event matches the creation parameter of the swap.

For this I had to introduce Anvil. For reproducibility, I froze the mainnet block used in the test (though the dependencies on the blockchain are minimal, i.e., deployed contracts, tokens, and a whale).

There are many more places where the test could be improved, but for now this is passable enough.

Note on the suppressed clippy warning: it's unrelated to this code and the amount of code to remove is large enough that it would distract from this PR.

Note on the flaky test: this is very hard to reproduce locally (~2 bad runs out of 100) but it triggered immediately on CI. I left this issue open and documented in the code.

How to test

CI should work.

Copy link

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

Just nits which are not necessary to address.

src/ethereum_client.rs Outdated Show resolved Hide resolved

let requested_swaps = eth_client
.get_requested_swaps(0, latest_block_num)
// Without mining an empty block, the test is flaky.

Choose a reason for hiding this comment

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

Is this a matter of mining more blocks or just waiting for a short time?

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately I don't know, it's very rare to see this behavior locally (maybe 2 times out of 100?), but it immediately triggered once in CI (it did work when rerunning the job however).
My goal here is document the issue as much as it is trying to fix it.

Copy link
Author

@fedgiac fedgiac Oct 9, 2024

Choose a reason for hiding this comment

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

Have you ever seen this when using Anvil in the services?

Choose a reason for hiding this comment

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

I don't recall having a block mined and not seeing the state change by inspecting the chain immediately afterwards. But the services also have quite a few code paths that can introduce delay so things like this regularly require some artificial waiting.

  1. change chain state with tx
  2. assume services will react to it (e.g. settle an order after injecting liquidity into a pool)
  3. check on chain that user balances changed after 2

Since 2 happens in a background tasks the test code would have to inject some waiting mechanism between 1 and 3.
Not sure if this applies to milkman as well but it seems reasonable to me. Because as long as the reaction of milkman for something happening on-chain doesn't happen in the same block the original chain state transition happens there is a race condition.

Copy link
Author

Choose a reason for hiding this comment

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

Here there's no waiting time: get current block, send a transaction, get the current block, get logs between the two blocks: the test just calls a function in the Milkman bot.
For now, I'll merge as it is so that I can work on implementing the contract changes, which are the main goal of this test revision, and leave this issue open for now.

@@ -16,5 +16,9 @@ jobs:
steps:
- uses: actions/checkout@v2
- uses: Swatinem/rust-cache@v1
- run: cargo test
- uses: foundry-rs/foundry-toolchain@v1
- run: anvil --fork-url 'https://eth.merkle.io' --fork-block-number 20920411 &

Choose a reason for hiding this comment

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

This would require all tests to work on the same for block.
But I guess fixing this is not worth it overall if we don't regularly add more tests.

Copy link
Author

Choose a reason for hiding this comment

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

True. It would also require all tests to reset the fork state after each test.
But yeah, the reason it's like this is that there's a single test that requires an external node (in general this project is very poorly tested) and I didn't want to take the time to set up an extendable testing environment. (Note that the existing test just uses the current state of the Görli blockchain, so for example you can't send out transactions).

Co-authored-by: Martin Magnus <martin.beckmann@protonmail.com>
@fedgiac fedgiac merged commit 7885e41 into main Oct 9, 2024
1 check 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