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.

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

merged 3 commits into from
Nov 14, 2023

Conversation

MerlinEgalite
Copy link
Contributor

@MerlinEgalite MerlinEgalite requested review from a team November 10, 2023 07:19
@MerlinEgalite MerlinEgalite self-assigned this Nov 10, 2023
@MerlinEgalite MerlinEgalite requested review from Rubilmax, Jean-Grimal, QGarchery, peyha and MathisGD and removed request for a team November 10, 2023 07:19
Jean-Grimal
Jean-Grimal previously approved these changes Nov 10, 2023
src/ChainlinkOracle.sol Outdated Show resolved Hide resolved
src/ChainlinkOracle.sol Show resolved Hide resolved
Co-authored-by: Romain Milon <[email protected]>
Signed-off-by: Merlin Egalite <[email protected]>
Rubilmax
Rubilmax previously approved these changes Nov 10, 2023
Comment on lines +41 to +42
/// - 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.

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

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

src/ChainlinkOracle.sol Outdated Show resolved Hide resolved
Co-authored-by: Jean-Grimal <[email protected]>
Signed-off-by: Merlin Egalite <[email protected]>
Copy link
Contributor

@MathisGD MathisGD left a comment

Choose a reason for hiding this comment

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

still in favor of removing this but it's not a big deal

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

@MerlinEgalite MerlinEgalite merged commit e6f8bb0 into main Nov 14, 2023
@MerlinEgalite MerlinEgalite deleted the fix/docs-72 branch November 14, 2023 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants