delvtech / hyperdrive

An automated market maker for fixed and variable yield with on-demand terms.
Apache License 2.0
33 stars 4 forks source link

`test-utils` overhaul #858

Closed jalextowle closed 7 months ago

jalextowle commented 8 months ago

A major area of technical debt in the Rust codebase is the test-utils crate. The abstractions that are currently being used are brittle and hard to work with. They don't lend themselves well to working with a remote chain, and they are tightly coupled to a deployment that uses anvil. The Agent struct and methods leaves things to be desired and is an area that could be improved. The major areas to improve are:

  1. Re-work the Chain abstraction so that it works equally well for local and remote deployments. We may not even need something like the Chain trait, and we may be better off working with a set of orthogonal tools that we can use to build ether-rs clients with signer, nonce management, and error handling middleware that makes it trivial to get a provider, signer, and client set up for an arbitrary chain with arbitrary signer metadata.
  2. Re-work the DevChain abstraction. Instead of tightly coupling address lookups to a DevChain struct, it would be better if this was a utility that could be called to read from the artifacts server. That way we could mix and match test utils regardless of whether we are interacting with a local anvil chain or an infra deployment.
  3. Re-work the TestChain abstraction. Instead of tightly coupling pool deployment to a TestChain struct, it would be better to have separate deployment functions that make it easy to deploy the factory, deployer coordinators, and pools. It would be nice to still have an integrated deployment flow (we'll still need this to build the docker images), but this should be more flexible. The benefit of doing this is that it would make it easier to test hyperdrive-math with many different pool configurations.
  4. Revamp the Agent struct. This struct is straddling the line between a tool that could actually be used to write trading bots and something that is only useful for testing. Some important logic is held in this struct (like the logic surrounding getting a conservative price for get_max_short) that should be upstreamed to hyperdrive-math. Generally speaking, we should just clean this struct up and make sure that it is well-tested and fully-featured.
dpaiton commented 8 months ago

I'd like to point out that (I think) all of this has been implemented in the Python IHyperdrive class. That might adjust your perceived priority on updating the test-utils if we can run equivalent tests in Python already. I'd be happy to meet to elaborate further (this should also include @slundqui).

This also brings up a meta discussion that we should have on differentiating the python & rust feature sets. While it makes sense for there to be parity in many areas (e.g. everything that we wrap via hyperdrivepy), there are other areas where we should take care to avoid duplicate work. That being said, I realize that testing in particular might require duplicates since we want most tests for rust fns to live in rust.

jalextowle commented 8 months ago

Yep, makes sense. Just to clarify the intent here, I’m not trying to duplicate functionality from agent0. Instead, I’d like us to be able to write better tests in Rust for the existing and new features. These changes are needed for us to do things like pass in fuzzed configs and similar things.

Going forward, I also think it makes sense for the protocol team to do more of the smart contract testing in Rust rather than Foundry, and this is setting us up for that.

dpaiton commented 8 months ago

Yep, makes sense. Just to clarify the intent here, I’m not trying to duplicate functionality from agent0. Instead, I’d like us to be able to write better tests in Rust for the existing and new features. These changes are needed for us to do things like pass in fuzzed configs and similar things.

Going forward, I also think it makes sense for the protocol team to do more of the smart contract testing in Rust rather than Foundry, and this is setting us up for that.

Heard, that makes a lot of sense to me.