Skip to content

Conversation

@cashd
Copy link
Contributor

@cashd cashd commented Mar 27, 2024

Resolved Issues

Resolves #885 #890

Description

Build on #883 and adds generalized testing coverage for open_long test cases.

  • adds test_open_long_with_base to InstanceTest contract.
  • adds test_open_long_with_shares to InstanceTest contract.
  • adds test_open_short_with_base to InstanceTest contract.
  • adds test_open_short_with_shares to InstanceTest contract.

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]]

Solidity

  • Tokens
    • Do all approve calls use forceApprove?
    • Do all transfer calls use safeTransfer?
    • Do all transferFrom calls use msg.sender as the from address?
      • If not, is the function access restricted to prevent unauthorized
        token spend?
  • Low-level calls (call, delegatecall, staticcall, transfer, send)
    • Is the returned success boolean checked to handle failed calls?
    • If using delegatecall, are there strict access controls on the
      addresses that can be called? It shouldn't be possible to delegatecall
      arbitrary addresses, so the list of possible targets should either be
      immutable or tightly controlled by an admin.
  • Reentrancy
    • Are functions that make external calls or transfer ether marked as nonReentrant?
      • If not, is there documentation that explains why reentrancy is
        not a concern or how it's mitigated?
  • Gas Optimizations
    • Is the logic as simple as possible?
    • Are the storage values that are used repeatedly cached in stack or
      memory variables?
    • If loops are used, are there guards in place to avoid out-of-gas
      issues?
  • Visibility
    • Are all payable functions restricted to avoid stuck ether?
  • Math
    • Is all of the arithmetic checked or guarded by if-statements that will
      catch underflows?
    • If Safe functions are altered, are potential underflows and
      overflows caught so that a failure flag can be thrown?
    • Are all of the rounding directions clearly documented?
  • 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?

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?

@github-actions
Copy link

github-actions bot commented Mar 27, 2024

Hyperdrive Gas Benchmark

Benchmark suite Current: 777a085 Previous: 46c834d Deviation Status
addLiquidity: min 33915 gas 1546 gas 2093.7257% 🚨
addLiquidity: avg 145835 gas 67936 gas 114.6653% 🚨
addLiquidity: max 405253 gas 293159 gas 38.2366% 🚨
checkpoint: min 29232 gas 1182 gas 2373.0964% 🚨
checkpoint: avg 104095 gas 48477 gas 114.7307% 🚨
checkpoint: max 211255 gas 191559 gas 10.2819% 🚨
closeLong: min 31428 gas 1492 gas 2006.4343% 🚨
closeLong: avg 132784 gas 29374 gas 352.0460% 🚨
closeLong: max 299537 gas 152303 gas 96.6718% 🚨
closeShort: min 31349 gas 1494 gas 1998.3266% 🚨
closeShort: avg 127871 gas 33444 gas 282.3436% 🚨
closeShort: max 201614 gas 149156 gas 35.1699% 🚨
initialize: min 31283 gas 1451 gas 2055.9614% 🚨
initialize: avg 252983 gas 213962 gas 18.2374% 🚨
initialize: max 322173 gas 253953 gas 26.8632% 🚨
openLong: min 33503 gas 1499 gas 2135.0233% 🚨
openLong: avg 167063 gas 51658 gas 223.4020% 🚨
openLong: max 252525 gas 185562 gas 36.0866% 🚨
openShort: min 33848 gas 1520 gas 2126.8421% 🚨
openShort: avg 169651 gas 51482 gas 229.5346% 🚨
openShort: max 363609 gas 181325 gas 100.5289% 🚨
redeemWithdrawalShares: min 31227 gas 1488 gas 1998.5887% 🚨
redeemWithdrawalShares: avg 63666 gas 22375 gas 184.5408% 🚨
redeemWithdrawalShares: max 153524 gas 109301 gas 40.4598% 🚨
removeLiquidity: min 31323 gas 1530 gas 1947.2549% 🚨
removeLiquidity: avg 218836 gas 151225 gas 44.7089% 🚨
removeLiquidity: max 376082 gas 325781 gas 15.4401% 🚨

This comment was automatically generated by workflow using github-action-benchmark.

@cashd cashd marked this pull request as ready for review March 27, 2024 19:22
@cashd cashd requested review from jalextowle and jrhea as code owners March 27, 2024 19:22
@cashd cashd changed the title [WIP] Generalize open_long test cases into InstanceTest suite Generalize open_long test cases into InstanceTest suite Mar 27, 2024
@cashd cashd requested a review from mcclurejt as a code owner March 28, 2024 18:30
@cashd cashd force-pushed the cashd/generalize-open-long-instance-test-cases branch from c7d93de to c1d6b62 Compare March 28, 2024 18:30
@cashd cashd changed the title Generalize open_long test cases into InstanceTest suite Generalize open_long & open_short test cases into InstanceTest suite Mar 28, 2024
@cashd
Copy link
Contributor Author

cashd commented Mar 28, 2024

I added open_short test cases to this PR

Copy link
Contributor

@jalextowle jalextowle left a comment

Choose a reason for hiding this comment

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

LGTM once the last few comments are addressed

@cashd cashd force-pushed the cashd/generalize-open-long-instance-test-cases branch from c92a494 to 777a085 Compare March 29, 2024 02:48
@cashd cashd enabled auto-merge (squash) March 29, 2024 02:48
@cashd cashd merged commit 18dfd4a into main Mar 29, 2024
@cashd cashd deleted the cashd/generalize-open-long-instance-test-cases branch March 29, 2024 03:03
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.

Generalize open_long instance test cases

3 participants