-
Notifications
You must be signed in to change notification settings - Fork 146
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
feat: subgraph availability manager contract #882
Conversation
feat: subgraph availability manager contract
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #882 +/- ##
==========================================
+ Coverage 92.70% 92.83% +0.13%
==========================================
Files 46 47 +1
Lines 2370 2415 +45
Branches 426 438 +12
==========================================
+ Hits 2197 2242 +45
Misses 173 173
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Looking good! left a few comments
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.
Some details while I'm still reviewing
Finished reviewing, I'm ready to approve after hearing what you think about the previous comments. |
@pcarranzav thanks for the review, addressed comments! |
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.
One remaining detail but LGTM
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.
Looks good, just one comment.
|
||
executionThreshold = _executionThreshold; | ||
voteTimeLimit = _voteTimeLimit; | ||
oracles = _oracles; |
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.
hmm i think we should set the oracles inside the for loop above and emit OracleSet
for each one.
Otherwise the subgraph will have a tough time getting the initial list of oracles
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.
Nice catch. I guess we should update the subgraph when we're ready to release this, I'll note that.
a4f0362
to
ddbcc86
Compare
@@ -159,3 +159,13 @@ contracts: | |||
controller: "${{Controller.address}}" | |||
calls: | |||
- fn: "syncAllContracts" | |||
SubgraphAvailabilityManager: | |||
init: | |||
governor: "${{Env.deployer}}" |
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.
Is there a reason we don't set the council as the governor from the get go?
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.
I was using the deployer while I'm testing so I don't have to use the multisig.
|
||
/// @notice Mapping of current nonce to subgraph deployment ID to oracle index to timestamp of last deny vote | ||
/// currentNonce => subgraphDeploymentId => oracleIndex => timestamp | ||
mapping(uint256 => mapping(bytes32 => mapping(uint256 => uint256))) public lastDenyVote; |
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.
I think this could be mapping(uint256 => mapping(bytes32 => uint256[NUM_ORACLES])
now? which I think would be a bit cheaper?
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.
Definitely, good idea!
|
||
/// @notice Mapping of current nonce to subgraph deployment ID to oracle index to timestamp of last allow vote | ||
/// currentNonce => subgraphDeploymentId => oracleIndex => timestamp | ||
mapping(uint256 => mapping(bytes32 => mapping(uint256 => uint256))) public lastAllowVote; |
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 as above, the last mapping could be an array now
packages/contracts/contracts/rewards/SubgraphAvailabilityManager.sol
Outdated
Show resolved
Hide resolved
fix: added input validation for executionThreshold (OZ L-01)
docs: added missing docstrings for events (OZ L-03)
fix: change OracleVote event order (OZ N-01)
fix: gas optimization (OZ N-02)
fix: clear vote for earlier rounds (OZ L-02)
fix: remove setDeniedMany since it's unused (OZ N-03)
chore: SAM deployments
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.
🎖️
No description provided.