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.

Sign up for GitHub

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

Jump to bottom

Made the registry more fully-featured #1039

Merged
merged 8 commits into from
Jun 6, 2024
Merged

Made the registry more fully-featured #1039

merged 8 commits into from
Jun 6, 2024

Conversation

jalextowle
Copy link
Contributor

@jalextowle jalextowle commented May 30, 2024 •
edited
Loading

Description

Updates the schema for the registry so that it is more helpful for programmatic users. This PR also updates some inconsistencies in the HyperdriveFactory and interfaces throughout the codebase.

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, the corresponding checklist can
be removed.

[[Reviewer Name]]

  • 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?

mcclurejt
mcclurejt reviewed May 31, 2024
View reviewed changes
Copy link
Contributor

@mcclurejt mcclurejt left a comment

Choose a reason for hiding this comment

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

tried to keep comments high-level since it's a wip

contracts/src/factory/HyperdriveRegistry.sol Outdated Show resolved Hide resolved
contracts/src/factory/HyperdriveRegistry.sol Outdated Show resolved Hide resolved
contracts/src/factory/HyperdriveRegistry.sol Outdated Show resolved Hide resolved
contracts/src/factory/HyperdriveRegistry.sol Show resolved Hide resolved
contracts/src/interfaces/IHyperdriveRegistry.sol Outdated Show resolved Hide resolved
contracts/src/interfaces/IHyperdriveRegistry.sol Show resolved Hide resolved
contracts/src/interfaces/IHyperdriveRegistry.sol Outdated Show resolved Hide resolved
contracts/src/factory/HyperdriveRegistry.sol Outdated Show resolved Hide resolved
contracts/src/factory/HyperdriveRegistry.sol Outdated Show resolved Hide resolved
contracts/src/interfaces/IHyperdriveRegistry.sol Outdated Show resolved Hide resolved
@jrhea jrhea mentioned this pull request May 31, 2024
Closed
@slundqui slundqui mentioned this pull request May 31, 2024
Merged
slundqui added a commit to delvtech/agent0 that referenced this pull request May 31, 2024
@slundqui
Connect existing postgres (#1504)
8f11205 
This PR allows the agent0 pipeline to connect to an existing postgres
resource instead of managing it's own container.

NOTE: actual implementation of this in the infra repo is blocked by
registry updates to query registered pools
(delvtech/hyperdrive#1039). Currently, we use
events to get registered pools, but since infra uses anvil's save/load
state, register events get lost. Need follow up after the hyperdrive
registry update to get list of registered pools from contract call.

- Agent0 `Chain` object now has a flag `use_existing_postgres` to look
for env variables to connect to existing postgres container.
- Using singular `.env` file instead of multiple ones for defining env
variables.
- Fixing checkpoint bots and invariance checks to take an `--infra` flag
to look for environment variables for connection.
- Fixing data pipeline scripts to read hyperdrive addresses from
registry.
- Fixing bug to rolling back db when adding hyperdrive addr to name
fails
ryangoree
ryangoree reviewed Jun 4, 2024
View reviewed changes
contracts/src/factory/HyperdriveRegistry.sol Show resolved Hide resolved
contracts/src/factory/HyperdriveRegistry.sol Outdated Show resolved Hide resolved
contracts/src/factory/HyperdriveRegistry.sol Outdated Show resolved Hide resolved
contracts/src/factory/HyperdriveRegistry.sol Outdated Show resolved Hide resolved
contracts/src/interfaces/IHyperdriveFactory.sol Outdated Show resolved Hide resolved
@jalextowle jalextowle force-pushed the jalextowle/registry/fully-featured-registry branch from 4b82db2 to fdb85e8 Compare June 4, 2024 21:34
@coveralls
Copy link
Collaborator

coveralls commented Jun 4, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9374541861

Details

  • 96 of 109 (88.07%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.3%) to 93.031%
Changes Missing Coverage Covered Lines Changed/Added Lines %
contracts/src/factory/HyperdriveFactory.sol 13 17 76.47%
contracts/src/factory/HyperdriveRegistry.sol 83 92 90.22%
Files with Coverage Reduction New Missed Lines %
contracts/src/factory/HyperdriveRegistry.sol 1 89.36%
Totals Coverage Status
Change from base Build 9334613106: -0.3%
Covered Lines: 1909
Relevant Lines: 2052
💛 - Coveralls

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 4, 2024 •
edited
Loading

Hyperdrive Gas Benchmark

Benchmark suite Current: dc558a6 Previous: d47b4e9 Deviation Status
addLiquidity: min 33827 gas 33827 gas 0% 🟰
addLiquidity: avg 155096 gas 155515 gas -0.2694% ✅
addLiquidity: max 429092 gas 428173 gas 0.2146% 🚨
checkpoint: min 40292 gas 40292 gas 0% 🟰
checkpoint: avg 142270 gas 142272 gas -0.0014% ✅
checkpoint: max 253424 gas 253424 gas 0% 🟰
closeLong: min 31361 gas 31361 gas 0% 🟰
closeLong: avg 136101 gas 134707 gas 1.0348% 🚨
closeLong: max 2625796 gas 2625796 gas 0% 🟰
closeShort: min 31349 gas 31349 gas 0% 🟰
closeShort: avg 132110 gas 131060 gas 0.8012% 🚨
closeShort: max 309547 gas 262384 gas 17.9748% 🚨
initialize: min 31371 gas 31371 gas 0% 🟰
initialize: avg 331019 gas 330116 gas 0.2735% 🚨
initialize: max 397010 gas 396015 gas 0.2513% 🚨
openLong: min 33370 gas 33370 gas 0% 🟰
openLong: avg 174071 gas 172914 gas 0.6691% 🚨
openLong: max 306958 gas 306039 gas 0.3003% 🚨
openShort: min 33936 gas 33936 gas 0% 🟰
openShort: avg 168677 gas 167433 gas 0.7430% 🚨
openShort: max 415814 gas 414787 gas 0.2476% 🚨
redeemWithdrawalShares: min 31251 gas 31251 gas 0% 🟰
redeemWithdrawalShares: avg 75041 gas 74277 gas 1.0286% 🚨
redeemWithdrawalShares: max 210204 gas 209214 gas 0.4732% 🚨
removeLiquidity: min 31301 gas 31301 gas 0% 🟰
removeLiquidity: avg 214444 gas 208376 gas 2.9120% 🚨
removeLiquidity: max 403959 gas 403026 gas 0.2315% 🚨

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

@coveralls
Copy link
Collaborator

coveralls commented Jun 4, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9375326207

Details

  • 96 of 109 (88.07%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.4%) to 92.982%
Changes Missing Coverage Covered Lines Changed/Added Lines %
contracts/src/factory/HyperdriveFactory.sol 13 17 76.47%
contracts/src/factory/HyperdriveRegistry.sol 83 92 90.22%
Files with Coverage Reduction New Missed Lines %
contracts/src/factory/HyperdriveRegistry.sol 1 89.36%
Totals Coverage Status
Change from base Build 9374786147: -0.4%
Covered Lines: 1908
Relevant Lines: 2052
💛 - Coveralls

ryangoree
ryangoree approved these changes Jun 5, 2024
View reviewed changes
Copy link
Member

@ryangoree ryangoree left a comment

Choose a reason for hiding this comment

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

Looks great! The API is really nice with flexible getters and streamlined setters. 🔥

contracts/src/factory/HyperdriveFactory.sol Show resolved Hide resolved
contracts/src/factory/HyperdriveFactory.sol Show resolved Hide resolved
contracts/src/factory/HyperdriveRegistry.sol Show resolved Hide resolved
jrhea
jrhea reviewed Jun 5, 2024
View reviewed changes
Copy link
Contributor

@jrhea jrhea left a comment

Choose a reason for hiding this comment

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

Just a few nits/questions

@slundqui slundqui mentioned this pull request Jun 5, 2024
Closed
@jalextowle jalextowle enabled auto-merge June 5, 2024 22:53
@jalextowle jalextowle force-pushed the jalextowle/registry/fully-featured-registry branch from f8ee174 to 94ec11a Compare June 6, 2024 03:47
@jalextowle jalextowle force-pushed the jalextowle/registry/fully-featured-registry branch from 94ec11a to dc558a6 Compare June 6, 2024 04:05
@coveralls
Copy link
Collaborator

coveralls commented Jun 6, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9394935530

Details

  • 96 of 109 (88.07%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.4%) to 92.982%
Changes Missing Coverage Covered Lines Changed/Added Lines %
contracts/src/factory/HyperdriveFactory.sol 13 17 76.47%
contracts/src/factory/HyperdriveRegistry.sol 83 92 90.22%
Files with Coverage Reduction New Missed Lines %
contracts/src/factory/HyperdriveRegistry.sol 1 89.36%
Totals Coverage Status
Change from base Build 9392859661: -0.4%
Covered Lines: 1908
Relevant Lines: 2052
💛 - Coveralls

@jalextowle jalextowle added this pull request to the merge queue Jun 6, 2024
Merged via the queue into main with commit fd02a5c Jun 6, 2024
30 checks passed
@jalextowle jalextowle deleted the jalextowle/registry/fully-featured-registry branch June 6, 2024 05:09
jalextowle added a commit that referenced this pull request Jun 7, 2024
@jalextowle
Made the registry more fully-featured (#1039)
50fb95f 
* Made the registry more fully-featured

* Added comprehensive tests for `setHyperdriveInfo`

* Added event verification to the registry tests

* Added a mixed test case for `setHyperdriveInfo`

* Added comprehensive testing for `setFactoryInfo`

* Fixed factory tests

* Addressed review feedback

* Addressed review feedback from @jrhea
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DannyDelott DannyDelott DannyDelott left review comments

@slundqui slundqui slundqui left review comments

@mcclurejt mcclurejt mcclurejt approved these changes

@jrhea jrhea jrhea approved these changes

@ryangoree ryangoree ryangoree approved these changes

@cashd cashd Awaiting requested review from cashd cashd is a code owner

@sentilesdal sentilesdal Awaiting requested review from sentilesdal sentilesdal is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jalextowle @coveralls @slundqui @ryangoree @DannyDelott @jrhea @mcclurejt