-
Notifications
You must be signed in to change notification settings - Fork 28
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 ERC4626: allow to pass the vault decimals directly #19
Chainlink ERC4626: allow to pass the vault decimals directly #19
Conversation
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.
Well in fact I'm still not convinced about this.
EDIT: I may be convinced but I feel that the solution is incomplete.
Correct License
Add formatting in CI
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.
Just commented that when you pushed lol: #19 (comment)
I'm not a fan of sample tbh..
Why not ? I feel like it fits pretty well: we take a sample, price it, scale it down to get the generic price |
I have the feeling that a sample is something that can change from the definition (else why taking a sample) while here study the evaluate the value of the same amount of shares each time. So to me it feels more like a reference. I'm approving as I don't think it's a big deal |
src/ChainlinkOracle.sol
Outdated
); | ||
} | ||
|
||
/* PRICE */ | ||
|
||
/// @inheritdoc IOracle | ||
function price() external view returns (uint256) { | ||
return (VAULT.getAssets(10 ** VAULT_PRECISION) * BASE_FEED_1.getPrice() * BASE_FEED_2.getPrice() * SCALE_FACTOR) | ||
uint256 sample = 10 ** CONVERSION_SAMPLE_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.
perhaps it could be an immutable as well?
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.
#26, saves 300 gas apparently
To fix that #7 (comment)