-
Notifications
You must be signed in to change notification settings - Fork 17
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
ChainlinkBoundedCompositeOracle #440
Conversation
Signed-off-by: Elliot <[email protected]>
@lyoungblood what do we want our upper and lower parameters to be for this? |
ChainlinkBoundedCompositeOracle
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
Signed-off-by: Elliot <[email protected]>
Signed-off-by: Elliot <[email protected]>
Signed-off-by: Elliot <[email protected]>
Signed-off-by: Elliot <[email protected]>
Signed-off-by: Elliot <[email protected]>
Signed-off-by: Elliot <[email protected]>
Signed-off-by: Elliot <[email protected]>
Signed-off-by: Elliot <[email protected]>
Signed-off-by: Elliot <[email protected]>
Signed-off-by: Elliot <[email protected]>
Signed-off-by: Elliot <[email protected]>
Signed-off-by: Elliot <[email protected]>
Signed-off-by: Elliot <[email protected]>
Signed-off-by: Elliot <[email protected]>
int256 _upperBound | ||
) external onlyOwner { | ||
require( | ||
_lowerBound < _upperBound, |
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.
weak inequity also here
fallbackLBTCOracle = AggregatorV3Interface(_fallbackOracle); | ||
|
||
require( | ||
_lowerBound < _upperBound, |
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.
it is not an unlikely scenario that one might want to set _lowerBound = _upperBound = 1
, and currently it is not possible.
Currently it is not our plan, and can also easily workaround it with a 1 wei delta, but consider using <=
instead of <
.
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.
really?
then once the oracle reserves change, the oracle will fallback
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.
yes, that the idea
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.
won't reserves increase over time though as LBTC is yield bearing, so the lower and upper bound being the same would mean that once the oracle updates and price increases, we use the fallback price, which isn't really intended behavior?
int256 public upperBound; | ||
|
||
/// @notice Scaling factor for price calculations | ||
uint8 public constant decimals = 18; |
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.
my mistake, it should be 8, to match redstone's PoR.
it would also work with 18, but then we might get confused with lower/upper bound configuration, as they will have to be with 18 decimals and not 8.
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.
sure, I can move to 8 decimals.
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.
then I can remove decimal normalization
Signed-off-by: Elliot <[email protected]>
…nto feat/lbtc-oracle
|
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.
LGTM
No description provided.