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

Add assumptions and acknowledgments #44

Merged
merged 3 commits into from
Nov 14, 2023
Merged

Add assumptions and acknowledgments #44

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
96ba187
docs: acknowledge no L2 support
MerlinEgalite Nov 10, 2023
a6482f3
docs: apply suggestion
MerlinEgalite Nov 10, 2023
1ef5a56
docs: apply suggestion
MerlinEgalite Nov 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/ChainlinkOracle.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,12 @@ contract ChainlinkOracle is IOracle {

/* CONSTRUCTOR */

/// @dev Here is the list of assumptions on the inputs that guarantees the oracle behaves as expected:
/// - Feeds are Chainlink-compliant or the address zero.
Rubilmax marked this conversation as resolved.
Show resolved Hide resolved
MerlinEgalite marked this conversation as resolved.
Show resolved Hide resolved
/// - Feeds are set in the correct order.
MerlinEgalite marked this conversation as resolved.
Show resolved Hide resolved
/// - Decimals passed as argument are correct.
Comment on lines +41 to +42
Copy link
Contributor

Choose a reason for hiding this comment

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

to me it's useless, it's like we can also add "the contract should be deployed"

Suggested change
/// - Feeds are set in the correct order.
/// - Decimals passed as argument are correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also feel lik this

Copy link
Contributor Author

@MerlinEgalite MerlinEgalite Nov 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

it's like we can also add "the contract should be deployed"

That's different. it reminds deployers what they should care about those params and it acknowledges to auditors that we don't check it (but we don't forget that this is important for a good setup).

It's closer to your mom saying "you should put your seatbelt". Ofc you know that you should do it but forgetting once could kill you.

PS: if we can reduce by 1% the hundreds of issues raised during the competition I would be happy

Copy link
Contributor

Choose a reason for hiding this comment

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

it reminds deployers what they should care about those params

but it's always like that no? in which contract don't we care about the constructor parameter.

PS: if we can reduce by 1% the hundreds of issues raised during the competition I would be happy

what report does it prevent (honestly idk)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In other contracts we make sure we list the assumptions or do the check though.

what report does it prevent (honestly idk)?

  • lack of sanity checks
  • decimals set as input can be incorrect
  • feeds might not give the expected output
  • ...

/// - The vault conversion sample is low enough to avoid overflows.
/// - The vault, if set, is ERC4626-compliant.
MerlinEgalite marked this conversation as resolved.
Show resolved Hide resolved
/// @param vault Vault. Pass address zero to omit this parameter.
/// @param baseFeed1 First base feed. Pass address zero if the price = 1.
/// @param baseFeed2 Second base feed. Pass address zero if the price = 1.
Expand Down
1 change: 1 addition & 0 deletions src/libraries/ChainlinkDataFeedLib.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ library ChainlinkDataFeedLib {
/// @dev Performs safety checks and returns the latest price of a `feed`.
/// @dev When `feed` is the address zero, returns 1.
/// @dev Notes on safety checks:
/// - L2s are not supported.
/// - Staleness is not checked because it's assumed that the Chainlink feed keeps its promises on this.
/// - The price is not checked to be in the min/max bounds because it's assumed that the Chainlink feed keeps its
/// promises on this.
Expand Down