Skip to content

Commit

Permalink
Various bug fixes in rust sdk (#5)
Browse files Browse the repository at this point in the history
PR originally from delvtech/hyperdrive#1005

# Resolved Issues
This solves various rust crashes resulting from python fuzz testing.
https://github.com/delvtech/hyperdrive/issues/1004

# Description
Fixes the following issues:
- Max long guesses is below the minimum transaction amount, so
`calc_open_long` throws a minimum transaction amount error. We clamp the
guess to be minimum transaction amount. and do a final check at the end
to ensure the max amount is greater than the minimum transaction amount.
- We catch a case in `absolute_max_long` where the
`target_share_reserves < effective_share_reserves`. In this case, we
throw a better descriptive panic.
- We short circuit `calc_max_long` and return 0 if the spot price after
a minimum long exceeds the max spot price.
- Fixing bug with wheel build script.
- Building and uploading to pypi on tag instead of push to main.

# Review Checklists

Please check each item **before approving** the pull request. While
going
through the checklist, it is recommended to leave comments on items that
are
referenced in the checklist to make sure that they are reviewed. If
there are
multiple reviewers, copy the checklists into sections titled `##
[Reviewer Name]`.
If the PR doesn't touch Solidity and/or Rust, the corresponding
checklist can
be removed.

## [[Reviewer Name]]

### Rust

- [ ] **Testing**
    - [ ] Are there new or updated unit or integration tests?
    - [ ] Do the tests cover the happy paths?
    - [ ] Do the tests cover the unhappy paths?
- [ ] Are there an adequate number of fuzz tests to ensure that we are
          covering the full input space?
- [ ] If matching Solidity behavior, are there differential fuzz tests
that
          ensure that Rust matches Solidity?
  • Loading branch information
slundqui authored Apr 30, 2024
1 parent 676e62e commit 1919734
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 24 deletions.
21 changes: 2 additions & 19 deletions .github/workflows/build_wheels.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,15 @@ name: build wheels

on:
push:
branches:
- main
tags:
- "v*"

concurrency:
group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}
cancel-in-progress: true

jobs:
detect-version-changes:
uses: ./.github/workflows/check_version.yml
with:
file_path: bindings/hyperdrivepy/pyproject.toml

build-wheels-linux:
needs: detect-version-changes
# Run on main if version has changed
if: needs.detect-version-changes.outputs.version_changed == 'true'
name: build on linux
runs-on: ubuntu-latest
steps:
Expand Down Expand Up @@ -75,9 +67,6 @@ jobs:
path: ./wheelhouse/*.whl

build-wheels-cibw:
needs: detect-version-changes
# Run on main if version has changed
if: needs.detect-version-changes.outputs.version_changed == 'true'
name: Build on ${{ matrix.os }}
runs-on: ${{ matrix.os }}
strategy:
Expand Down Expand Up @@ -143,11 +132,8 @@ jobs:
path: ./wheelhouse/*.whl

build-sdist:
needs: detect-version-changes
name: Build source distribution
runs-on: ubuntu-latest
# Run on main if version has changed
if: needs.detect-version-changes.outputs.version_changed == 'true'
steps:
- uses: actions/checkout@v3

Expand All @@ -165,14 +151,11 @@ jobs:
build-wheels-linux,
build-wheels-cibw,
build-sdist,
detect-version-changes,
]
runs-on: ubuntu-latest
environment: pypi
permissions:
id-token: write
# Run on main if version has changed
if: needs.detect-version-changes.outputs.version_changed == 'true'
steps:
- uses: actions/download-artifact@v4
with:
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/check_version.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
# TODO this workflow isn't used anymore, but we keep this around in case it's useful in the future
# We likely want to abstract this out used across delv org
name: Check Version

on:
Expand Down
43 changes: 40 additions & 3 deletions crates/hyperdrive-math/src/long/max.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,15 @@ impl State {
let budget = budget.into();
let checkpoint_exposure = checkpoint_exposure.into();

// Check the spot price after opening a minimum long is less than the
// max spot price
let spot_price_after_min_long = self
.calculate_spot_price_after_long(self.minimum_transaction_amount(), None)
.unwrap();
if spot_price_after_min_long > self.calculate_max_spot_price() {
return fixed!(0);
}

// Calculate the maximum long that brings the spot price to 1. If the pool is
// solvent after opening this long, then we're done.
let (absolute_max_base_amount, absolute_max_bond_amount) = self.absolute_max_long();
Expand Down Expand Up @@ -81,6 +90,12 @@ impl State {
// we converge to the solution.
let mut max_base_amount =
self.max_long_guess(absolute_max_base_amount, checkpoint_exposure);

// possible_max_base_amount might be less than minimum transaction amount.
// we clamp here if so
if max_base_amount < self.minimum_transaction_amount() {
max_base_amount = self.minimum_transaction_amount();
}
let mut maybe_solvency = self.solvency_after_long(
max_base_amount,
self.calculate_open_long(max_base_amount).unwrap(),
Expand Down Expand Up @@ -118,7 +133,15 @@ impl State {
if maybe_derivative.is_none() {
break;
}
let possible_max_base_amount = max_base_amount + solvency / maybe_derivative.unwrap();
let mut possible_max_base_amount =
max_base_amount + solvency / maybe_derivative.unwrap();

// possible_max_base_amount might be less than minimum transaction amount.
// we clamp here if so
if possible_max_base_amount < self.minimum_transaction_amount() {
possible_max_base_amount = self.minimum_transaction_amount();
}

maybe_solvency = self.solvency_after_long(
possible_max_base_amount,
self.calculate_open_long(possible_max_base_amount).unwrap(),
Expand All @@ -132,6 +155,11 @@ impl State {
}
}

// If the max base amount is less than the minimum transaction amount, we return 0 as the max long.
if max_base_amount <= self.minimum_transaction_amount() {
return fixed!(0);
}

// Ensure that the final result is less than the absolute max and clamp
// to the budget.
if max_base_amount >= absolute_max_base_amount {
Expand Down Expand Up @@ -210,9 +238,18 @@ impl State {

// The absolute max base amount is given by:
//
// absoluteMaxBaseAmount = c * (z_t - z)

// Here, the target share reserves may be smaller than the effective share reserves.
// Instead of throwing a panic error in fixed point, we catch this here and throw a
// descriptive panic.
// TODO this likely should throw an err.
let effective_share_reserves = self.effective_share_reserves();
if target_share_reserves < effective_share_reserves {
panic!("target share reserves less than effective share reserves");
}

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

// The absolute max bond amount is given by:
//
Expand Down
2 changes: 1 addition & 1 deletion crates/hyperdrive-math/src/short/max.rs
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ impl State {
let mut maybe_solvency =
self.solvency_after_short(max_bond_amount, spot_price, checkpoint_exposure);
if maybe_solvency.is_none() {
panic!("Initial guess in `max_short` is insolvent.");
panic!("Initial guess in `absolute_max_short` is insolvent.");
}
let mut solvency = maybe_solvency.unwrap();
for _ in 0..maybe_max_iterations.unwrap_or(7) {
Expand Down
2 changes: 1 addition & 1 deletion scripts/build_python.sh
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ python -m pip install --upgrade -r requirements-dev.txt
python -m pip install auditwheel

echo "nav into the crate so relative paths work"
cd crates/hyperdrivepy
cd bindings/hyperdrivepy

echo "build the wheel for the current platform"
python setup.py bdist_wheel
Expand Down

0 comments on commit 1919734

Please sign in to comment.