fix: N-09, Unexplained and unused constants #3909
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chrismaree
reviewed
May 17, 2022
| @@ -127,7 +127,8 @@ contract OptimisticOracle is OptimisticOracleInterface, Testable, Lockable { | |||
|
|
|||
| // This is effectively the extra ancillary data to add ",ooRequester:0000000000000000000000000000000000000000". | |||
| uint256 private constant MAX_ADDED_ANCILLARY_DATA = 53; | |||
| uint256 public constant OO_ANCILLARY_DATA_LIMIT = ancillaryBytesLimit - 53; | |||
| uint256 public constant OO_ANCILLARY_DATA_LIMIT = ancillaryBytesLimit - MAX_ADDED_ANCILLARY_DATA; | |||
| int256 public constant TOO_EARLY_RESPONSE = type(int256).min; | |||
There was a problem hiding this comment.
would be cool to refactor this in a way that we can re-use this magic number throughout some of the other contracts in this package such as here
There was a problem hiding this comment.
Agree! But let's do this in a follow-up just to keep these fixes focused and simple.
mrice32
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Throughout the OptimisticGovernor contract, to check if a proposal has been approved
by the Optimistic Oracle the literal value int256 1e18 is used, where 1e18 signifies that a
proposal was not rejected by the Optimistic Oracle.
Similarly, in the update to the OptimisticOracle contract the function proposedPrice
uses a magic value type(int256).min to indicate that an event-based proposal cannot be
resolved, because the event has not yet taken place.
Lastly, in the OptimisticOracle contract the MAX_ADDED_ANCILLARY_DATA constant is
declared on line 129. On the next line the constant should be used, but instead the value of the
constant is used directly to derive another constant.
To improve the overall readability of the codebase and to facilitate refactoring, consider
defining a constant for every literal or magic value used, giving it a clear and self-explanatory
name, and then using it in place of literal values. Also consider adding an inline comment
explaining how literal values were calculated or why they were chosen.
Summary
Fixed these issues.
Testing
Check a box to describe how you tested these changes and list the steps for reviewers to test.
Issue(s)
N/A