-
Notifications
You must be signed in to change notification settings - Fork 3
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
Chainlink Integration #1135
Chainlink Integration #1135
Conversation
Pull Request Test Coverage Report for Build 10483818456Details
💛 - Coveralls |
contracts/src/deployers/chainlink/ChainlinkHyperdriveDeployerCoordinator.sol
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I just had a couple of nits, but I will go ahead and approve. I would consider having @DannyDelott take a look at the PR in case he has any issues with the lack of a baseToken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome stuff! coolest integration yet imo
Hyperdrive Gas Benchmark
This comment was automatically generated by workflow using github-action-benchmark. |
Resolved Issues
Fixes: #1104
Description
This PR adds a chainlink integration. This can be used with general price feeds, although the intended use is with price feeds like the
wsteth-eth reference rate
on Gnosis Chain. This integration uses the output of a given Chainlink aggregator as the vault share price. It doesn't support base as a deposit asset since the base token may not even exist. Some documentation is included in the implementation that describes how Chainlink downtime or deprecation will impact the instance.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]]
approve
calls useforceApprove
?transfer
calls usesafeTransfer
?transferFrom
calls usemsg.sender
as thefrom
address?token spend?
call
,delegatecall
,staticcall
,transfer
,send
)success
boolean checked to handle failed calls?delegatecall
, are there strict access controls on theaddresses 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.
nonReentrant
?not a concern or how it's mitigated?
memory variables?
issues?
payable
functions restricted to avoid stuck ether?catch underflows?
Safe
functions are altered, are potential underflows andoverflows caught so that a failure flag can be thrown?
covering the full input space?