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.
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
SIP-165 Debt Orcale #1694
SIP-165 Debt Orcale #1694
Changes from 53 commits
6522f12
510823f
8ed2fff
834ec9d
bff2fb6
9043de1
ae5c111
d532380
3b53405
5076ab1
e351a65
f1280ab
fea5c91
10ea7bd
7cf745b
113ada3
f267e6e
667fcc8
2265bf6
fb1530a
bdf3e69
87cd0da
8171cb3
1deac59
e961d51
09c501e
6d380b8
00cb270
f2cc452
dc6aefa
3f7ffe7
122e37e
0107563
a2c42d3
1ea3e68
eb4fad0
5afa691
68269c4
e287b37
cb5a604
6b3f157
a5f47ba
ed6f858
4d5e5fe
b5b81ef
79c33f7
c3bed7d
45d94e4
5229444
1676f1f
4180c14
9bb44cf
05005cf
86b41d8
db35a57
657443e
e9450a5
f74803d
20bb800
a0eba7e
e45b6d9
edcce22
c375a93
ad3d288
e64d771
27293b5
f81aae7
770b3c1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
[np] these internal functions should have an underscore prefix for consistency
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.
do you know why internal functions have underscores btw? I still have yet to figure this out...
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's just a naming convention for functions and variables that are supposed to be internal or private to the contract.
Sometimes it's also used to avoid shadowing variables.
Technically it isn't needed (unless you got a linter like solint), but it does improve readability when scanning through contracts imo.
Like without reading all the keywords I can quickly say: "OK, this function is internal so it's not called by anyone else."
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.
would it be useful to have this as public or is this info available through some other view?
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.
same question about oracle output validation
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.
figured out how to do it, will be impl before merge today
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.
should there be some validation on the values returned from these oracles? what if the oracles experience some issues / are fed obviously bad data?
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.
will be impl
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.
nit: refactor address to intermediate variable for readability?
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.
nit(s):
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.
the
onlyRelayer
is only used once I think, and it looks like an an important and comlplex part of access control: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.
should this be 0 or
FEE_PERIOD_LENGTH - 2
like below? I know they evaluate to the same thing with current parameters, but what the correct number should be if theFEE_PERIOD_LENGTH
is different, since the one that actually is being closed is theFEE_PERIOD_LENGTH - 2
one?If they should evaluate to the same number, maybe better to just reuse the
storage periodClosing
from below to update the values.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.
nit:
onlySelfOrBridge
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.
this check is not being hit as in this contract the
_closeSecondary
is directly called incloseCurrentFeePeriod
so I guess this check can be removedThere 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.
nit: might be a good place to add some comments about the invocation path
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.
should there be some
isInvalid
value (or something thatOR
s both) that does some validation on the oracle values (some expected minimal size, or some expected ratio deviation from previous known values)?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.
should this check that
issuedSynthsUpdatedAt
andratioUpdatedAt
are not in future relative toblock.timestamp
? in case something bad happens to optimism injected timestamps?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.
nit: probably safer to append values to enums, although I don't think it matters here