Improved Testing of the Rust SDK

[jalextowle]

There were some issues reported a while ago by @slundqui. We'll need to review these calculations to see if we can find the source of the issue. We should also take the time to improve the test coverage. The current testing methodology is pretty good, but we don't test with different configurations, which really limits the robustness of the testing.

We should differentially test every rust function against Solidity.

[ryangoree]

I may have fallen down a rabbit hole trying to understand some errors 🕳️🔍

TL;DR

• absolute_max_long was panicking about 14% of the time.
• Operations on FixedPoint can overflow the U256 type.
• Refactoring functions to return a Result and adding specific checks before each operation is tedious.
• I wrote a result!() macro that wraps an expression in a catch_unwind block and returns an Err with the panic message if the expression panics.
• The FixedPoint library could also be refactored to prevent overflowing internally by dynamically adjusting the precision.
• Before going further, I'd like to get a sanity check on the direction I'm heading in and look deeper into the cause of the original absolute_max_long panic.

The Rabbit Hole

I pulled the latest on main and started by running the tests for max long. I noticed a lot of stack traces being logged during tests, so I investigated and learned that they were coming from the absolute_max_long function being called in the fuzz_absolute_max_long test:

hyperdrive-rs/crates/hyperdrive-math/src/long/max.rs

Line 483 in 3360daa

let actual = panic::catch_unwind(|| state.absolute_max_long());

Specifically, this line was panicking about 14% of the time from an arithmetic operation overflow trying to subtract a larger number (self.effective_share_reserves()) from a smaller one (target_share_reserves):

hyperdrive-rs/crates/hyperdrive-math/src/long/max.rs

Lines 214 to 215 in 3360daa

	let absolute_max_base_amount =
	(target_share_reserves - self.effective_share_reserves()) * self.vault_share_price();

So, I refactored the function to return a Result and added a check to return early with an Err in these cases:

fn absolute_max_long(&self) -> Result<(FixedPoint, FixedPoint)> {
	// ...
	let effective_share_reserves = self.effective_share_reserves();
	if effective_share_reserves > target_share_reserves {
		return Err(eyre!(
			"Effective share reserves exceed target share reserves."
		));
	}
	// ...
 Ok((absolute_max_base_amount, absolute_max_bond_amount))
}

And in the test:

-let actual = panic::catch_unwind(|| state.absolute_max_long());
+let actual = state.absolute_max_long();

This fixed the fuzz_absolute_max_long test, but then the fuzz_calculate_max_long test starting failing. This time it was panicking in the solvency_after_long function with an arithmetic operation overflow trying to perform fixed point division on the a large number:

hyperdrive-rs/crates/hyperdrive-math/src/long/max.rs

Line 354 in 3360daa

if share_reserves + checkpoint_exposure / self.vault_share_price()

When dividing a FixedPoint with the / operator, the library uses div_down which calls mul_div_down internally to scale the number before dividing:

hyperdrive-rs/crates/fixed-point/src/lib.rs

Lines 174 to 176 in 3360daa

	pub fn div_down(self, other: FixedPoint) -> FixedPoint {
	self.mul_div_down(fixed!(1e18), other)
	}

If the number is big enough, scaling the number can overflow the U256. For example:

let number = fixed!(52413673737912410055885968403491055370236425929284030968784.659601181251437610);
let scaled = number.0 * fixed!(1e18).0 // panics because 52413673737912410055885968403491055370236425929284030968784659601181251437610000000000000000000n > U256::MAX

This means that a FixedPoint can hold up to U256::MAX, but can only perform division on U256::MAX / fixed!(1e18). I looked to see if the Solidity (FixedPointMath.sol) has the same limitation and learned that it does:

hyperdrive-rs/contracts/src/libraries/FixedPointMath.sol

Lines 25 to 37 in 3360daa

	function mulDivDown(
	uint256 x,
	uint256 y,
	uint256 denominator
	) internal pure returns (uint256 z) {
	/// @solidity memory-safe-assembly
	assembly {
	// Equivalent to require(denominator != 0 && (y == 0 || x <= type(uint256).max / y))
	if iszero(
	mul(denominator, iszero(mul(y, gt(x, div(MAX_UINT256, y)))))
	) {
	revert(0, 0)
	}

Like absolute_max_long, I could refactor solvency_after_long to return a Result and add a check to ensure we don't perform the division if the value is too big, but I was noticing a pattern where the functions I want to refactor to return a Result are required to know when an operation might cause an overflow. So, instead of writing specific checks before each operation, I wrote a macro that wraps the expression in a catch_unwind block and returns an Err with the panic message if the expression panics:

#[proc_macro]
pub fn result(input: TokenStream) -> TokenStream {
    let expr = parse_macro_input!(input as Expr);
    quote!(
    std::panic::catch_unwind(|| #expr).or_else(|panic_info| {
        let panic_message = if let Some(s) = panic_info.downcast_ref::<String>() {
            s.as_str()
        } else if let Some(&s) = panic_info.downcast_ref::<&str>() {
            s
        } else {
            "Operation failed with unknown panic"
        };
        Err(eyre::eyre!("Operation failed: {}", panic_message))
    }))
    .into()
}

And, in the solvency_after_long function, I wrapped the entire expression in the result! macro:

    pub(super) fn solvency_after_long(
        &self,
        base_amount: FixedPoint,
        bond_amount: FixedPoint,
        checkpoint_exposure: I256,
    ) -> Option<FixedPoint> {
        result!({
            let governance_fee = self.open_long_governance_fee(base_amount);
            let share_reserves = self.share_reserves() + base_amount / self.vault_share_price()
                - governance_fee / self.vault_share_price();
            let exposure = self.long_exposure() + bond_amount;
            let checkpoint_exposure = FixedPoint::from(-checkpoint_exposure.min(int256!(0)));
            if share_reserves + checkpoint_exposure / self.vault_share_price()
                >= exposure / self.vault_share_price() + self.minimum_share_reserves()
            {
                Some(
                    share_reserves + checkpoint_exposure / self.vault_share_price()
                        - exposure / self.vault_share_price()
                        - self.minimum_share_reserves(),
                )
            } else {
                None
            }
        })
        .unwrap_or(None)
    }

This fixed the specific issue but the test still fails at the next unwrapped operation that overflows. To fully address the problem, all the functions that perform arithmetic operations on FixedPoint would need to be refactored to use the result! macro or validate the inputs to every operation before performing it and return early with an Err if the operation would overflow. The latter is tedious and specific to each operation.

Alternatively the FixedPoint library could be refactored to handle these cases internally. For example, the div_down function could dynamically adjust the scale to prevent overflow which would reduce the precision of the result:

    pub fn div_down(self, other: FixedPoint) -> FixedPoint {
        let scale = uint256!(1e18);
        let mut adjustment: u64 = 1;

        // Dynamically adjust the scale to prevent overflow.
        while let (_, true) = self.0.overflowing_mul(scale / adjustment) {
            adjustment *= 10;
        }

        let truncated_value = self.mul_div_down(FixedPoint(scale / adjustment), other);
        FixedPoint(truncated_value.0 * adjustment)
    }

Example:

let number = fixed!(2410055885968403491055370236425929284030968784.659601181251437610);
let divisor = fixed!(1.226983151123229214);

// expected value (target precision): 1964212698244586656204108415688430900380716129.257078308393140637
// actual value (adjusted precision): 1964212698244586656204108415688430900380716129.257078308393100000

This avoids having a FixedPoint that can't be divided by anything, however this would be a discrepancy between the Rust and Solidity implementations that tests would need to account for.

These are both pretty big changes, so before going further, I'd like to get a sanity check on the direction I'm heading in and look deeper into the cause of the original error in absolute_max_long.

Phew! 🐇🕳️🔍

[slundqui]

FYI I ended up touching on some of these changes in this pr:
#5

[dpaiton]

Todos:

• write out testing coverage & depth for each fn

  • bucket rust fns that should have parity in sol vs shouldn't
  • write tests for all fns that are paired (sol & rs) but lacking a paired test
    Ensure all parity functions are tested against solidity #130

• add ability to spin up random // different configs when deploying a pool for sol vs rs testing
  Adjust test chain to allow you to fuzz over config values #128

• add test coverage calculator to repo
  Add coverage report in CI #129

[dpaiton]

PRs that work towards this goal:

• Rust cleanup; add tests; simplify fn signatures -- Rust cleanup; add tests; simplify fn signatures hyperdrive#990
• Add short::open tests -- Add short::open tests hyperdrive#970
• fix calculate_spot_price_after_short -- fix calculate_spot_price_after_short and add open short deltas & preview #15
• add hyperdrivepy short tests -- add hyperdrivepy short tests agent0#1449

[dpaiton]

I bumped up the assignees on this. I'm happy to manage it, but it's a broad enough task that everyone should try to contribute tests as they can & be aware of how their current efforts can influence the progress of this (e.g. make sure you're hella testing new features!)

[dpaiton]

Duplicate issues:
Fuzz test calculate_* trades against solidity -- delvtech/hyperdrive#937

I'm reproducing here and closing it:

There are limited tests for rust calculate_* functions against what's implemented in Solidity:

Open long/short doesn't fuzz against solidity.
Close long/short only fuzzes the underlying calculate_close_long_flat_plus_curve and calculate_close_short_flat_plus_curve against the calculate_close_* from hyperdrive math. However, these functions do not take fees or negative interest checks into account (both on the solidity and rust side).
Ideally, we would fuzz these trades against the public interface of hyperdrive.

[dpaiton]

@ryangoree

Alternatively the FixedPoint library could be refactored to handle these cases internally.

I think this is the move. We should refactor fixedpoint to return results instead of panic #99

This would clean up a lot of testing code.

[dpaiton]

U256 issue outlined here: #127

[dpaiton]

Updating test chain to support fuzzing over multiple states: #128

[dpaiton]

panic / result issue resolved as much as we want by #113

[dpaiton]

closing in favor of new smaller issues