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

cleanup tests & test support code #211

Merged
merged 13 commits into from
Jan 22, 2025
Merged

cleanup tests & test support code #211

merged 13 commits into from
Jan 22, 2025

Conversation

dpaiton
Copy link
Contributor

@dpaiton dpaiton commented Jan 10, 2025

Description

  • I moved a validity check from calculate_open_short to solvency_after_short. And we now call solvency_after_short in calculate_open_short. This diverges a bit from Solidity because we do not split the apply_open_short function from the calculate_open_short function in rust. This means it was possible to get a valid answer from calculate_open_short that would revert in the contract. That should be fixed, now.
  • I updated a bunch of tests to check if a short is possible by directly calling solvency_after_short(min_txn) instead of the incorrect check it was doing.
  • I removed the get_max_short function from the preamble. We don't need all of the extra checks.
  • I modified the preamble random state generator to be faster by taking fewer steps and using the approximate max short. This was a really slow step in the solidity tests.
  • the Agent class now uses the new functions when you call agent.calculate_max_short
  • I increased the defaults for calculate_short_bonds_given_deposit to maybe_base_tolerance=1e10 (from 1e9) and maybe_max_iterations=1_000 (from 500).

@dpaiton dpaiton changed the base branch from main to dpaiton/fix-max-short-tests January 10, 2025 23:20
@dpaiton dpaiton changed the title Cleanup tests & test support code cleanup tests & test support code Jan 10, 2025
@dpaiton dpaiton mentioned this pull request Jan 10, 2025
7 tasks
@dpaiton dpaiton force-pushed the dpaiton/code-cleanup branch 2 times, most recently from 593e47f to c065238 Compare January 11, 2025 01:06
@dpaiton
Copy link
Contributor Author

dpaiton commented Jan 18, 2025

I was able to fix the failing test locally by increasing the budget tolerance from 1e9 to 1e10 in crates/hyperdrive-math/src/short/max.rs:fuzz_calculate_max_short_budget_consumed (line 1205).

I've lost write privileges to these branches. @sentilesdal would you mind pushing up a commit with that fix to see if CI passes after?

@sentilesdal sentilesdal force-pushed the dpaiton/fix-max-short-tests branch from 33c1a9b to 201b048 Compare January 22, 2025 21:27
Base automatically changed from dpaiton/fix-max-short-tests to main January 22, 2025 22:04
@sentilesdal sentilesdal self-requested a review January 22, 2025 23:06
Copy link
Contributor

@sentilesdal sentilesdal left a comment

Choose a reason for hiding this comment

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

Went through with Dylan IRL. This is basically just test cleanups right before he left. Approved.

@dpaiton
Copy link
Contributor Author

dpaiton commented Jan 22, 2025

Lingering issues are summarized here #136 (comment)

@sentilesdal sentilesdal merged commit 83009e4 into main Jan 22, 2025
8 checks passed
@sentilesdal sentilesdal deleted the dpaiton/code-cleanup branch January 22, 2025 23:59
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