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

test: unit testing suite proposal #493

Open
wei3erHase opened this issue Apr 11, 2024 · 0 comments
Open

test: unit testing suite proposal #493

wei3erHase opened this issue Apr 11, 2024 · 0 comments

Comments

@wei3erHase
Copy link
Collaborator

wei3erHase commented Apr 11, 2024

The current Vault tests for core functions (swap / add / rm liquidity) rely in multiple parts:

  • VaultMock: adds a lot of logic to the Vault
  • PoolMock: a linear pool, with custom code for hooks
  • PoolMockFactory: registers the pool with a given config
  • VaultExtension: handles the pool registration
  • Router: handles the lock + swap + ERC20 balances
  • ERC20s: we check the balances of the actors

This contracts are joined together different tests:

  • VaultSwap
  • VaultSwapRate
  • VaultLiquidity
  • VaultLiquidityFees
  • LiquidityApproximation
  • ...

The setup falls in between what we consider a unit test and an integration one, as it depends on many mock contracts and sets some assumptions on the amounts in or out that the pool may handle. For strict unit tests (that provide a good base for fuzzing and setting expectations), we shouldn't rely on more than just the Vault, or the VaultMock, considering that we shouldn't need to have a VaultExtension for any of the core vault functions (only for registering the pool).

Unit test proposal

  • Remove the VaultExtension from the vault deployment setup (we don't need it)
  • Add VaultTestUtils as the vault implementation (so that gets fallback delegate call) with:
    • setPoolConfig method
    • setPoolBalances method
    • mintBPT (if needed for liquidity tests)
  • Manually set the config (as initialized) and manually set (fuzzed) balances
  • Create a test suite that deploys this proxy setup: Vault -> VaultTestUtils
  • Find a way of circumventing the onlyLocker modifier
  • Perform swap in every possible way
    • fuzz the onSwap return (is a uint256 amount) using vm.mockCall
    • test reverts for when shouldCallBeforeSwap and onBeforeSwap -> false
    • verify that the Vault created debt/credit
    • verify that the Vault changed the raw pool balances
    • if fees, verify that the balances are correctly updated
    • if rate, verify that the Vault changed the live pool balances

In this way, we can properly test this methods, with versatility for calling each part individually, and we could replicate this test setup for VaultExtension mehtods: we create a StorageTestUtils is VaultStorage that fallbacks into VaultExtension (simil being the vault), and we test that the extension methods are generating the expected storage changes on our main implementation (w/o using Vault contract).

In this way, we're removing from the test:

  • VaultExtension (shouldn't be needed for swap add rm liq)
  • Pool (is now just a mockCall)
  • PoolFactory (we manually "register" a pool, we just set the bytes32 on the mapping)
  • Router (we don't use it, just call directly the Vault)
  • ERC20s (we don't do any token transfer)
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

No branches or pull requests

1 participant