-
Notifications
You must be signed in to change notification settings - Fork 190
feat: add managed OOv2 #4841
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
base: master
Are you sure you want to change the base?
feat: add managed OOv2 #4841
Conversation
Signed-off-by: Matt Rice <[email protected]>
import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; | ||
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; | ||
import "@openzeppelin/contracts/utils/math/SafeMath.sol"; | ||
import "@openzeppelin/contracts/utils/Address.sol"; | ||
import "@openzeppelin/contracts/access/Ownable.sol"; | ||
|
||
import "../../data-verification-mechanism/interfaces/StoreInterface.sol"; | ||
import "../../data-verification-mechanism/interfaces/OracleAncillaryInterface.sol"; | ||
import "../../data-verification-mechanism/interfaces/FinderInterface.sol"; | ||
import "../../data-verification-mechanism/interfaces/IdentifierWhitelistInterface.sol"; | ||
import "../../data-verification-mechanism/implementation/Constants.sol"; | ||
|
||
import "../interfaces/OptimisticOracleV2Interface.sol"; | ||
|
||
import "../../common/implementation/Testable.sol"; | ||
import "../../common/implementation/Lockable.sol"; | ||
import "../../common/implementation/FixedPoint.sol"; | ||
import "../../common/implementation/AncillaryData.sol"; | ||
import "../../common/implementation/AddressWhitelist.sol"; |
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.
These imports are not strictly required as we have them in the inherited OptimisticOracleV2
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.
removed and replaced with named imports
using SafeERC20 for IERC20; | ||
using Address for address; |
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.
These are not directly used in the derived contract
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.
removed
bytes memory ancillaryData, | ||
address whitelist | ||
) external nonReentrant() onlyOwner() { | ||
customProposerWhitelists[_getId(requester, identifier, timestamp, ancillaryData)] = AddressWhitelistInterface( |
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 might make sense to also check if the given request state is State.Requested
. This could help the caller be better assured that the applied whitelist would in fact be effective. Otherwise they could be under impression that the whitelist was set, but in fact the proposal has been already made against the default whitelist.
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 no longer applies as we want the manager being able to set these before the request has be made
* @notice Sets the requester whitelist. | ||
* @param whitelist address of the whitelist to set. | ||
*/ | ||
function ownerSetRequesterWhitelist(address whitelist) external nonReentrant() onlyOwner() { |
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.
for the sake of completeness, shall we also have similar function for setting the default proposer whitelist?
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.
Yep, this was my mistake. Good call!
packages/core/contracts/optimistic-oracle-v2/implementation/WhitelistOptimisticOracleV2.sol
Outdated
Show resolved
Hide resolved
) public override returns (uint256 totalBond) { | ||
AddressWhitelistInterface whitelist = | ||
customProposerWhitelists[_getId(requester, identifier, timestamp, ancillaryData)]; | ||
if (address(whitelist) == address(0)) { |
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.
we should allow for a request to have no whitelist so it can be proposed by anyone.
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.
added AllowAllList
contract that always returns true when checking the address. so admin or manager can set whitelist to this AllowAllList
contract if disabling the whitelist restrictions is desired.
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
require(_getState(requester, identifier, timestamp, ancillaryData) == State.Requested, "setBond: Requested"); | ||
_validateBond(bond); | ||
Request storage request = _getRequest(requester, identifier, timestamp, ancillaryData); | ||
request.requestSettings.bond = bond; |
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.
Just confirming - it looks like the request sender can still call setBond
directly on the OOv2
and bypass the onlyRequestManager
restriction. That might be acceptable but is this behavior intentded? Same for liveness.
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.
yes, I believe its expected since requester would be an integration contract with known behavior. This contract only extends these requester's priviledges to request managers.
* @param whitelist address of the whitelist to set. | ||
*/ | ||
function _setDefaultProposerWhitelist(address whitelist) internal { | ||
defaultProposerWhitelist = AddressWhitelistInterface(whitelist); |
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 we require whitelist != zeroAddress?
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.
good call, we should definitely check this
* @param ancillaryData ancillary data of the price being requested. | ||
* @return AddressWhitelistInterface the custom proposer whitelist for the request or zero address if not set. | ||
*/ | ||
function getCustomProposerWhitelist( |
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.
we should also extend this to have a function recursing the effective whitelist array and boolean on whether the whitelist is enabled.
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.
added getProposerWhitelistWithEnforcementStatus
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
whitelist = defaultProposerWhitelist; | ||
} | ||
|
||
require(whitelist.isOnWhitelist(proposer) || msg.sender == owner(), "Proposer not whitelisted"); |
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.
now that owner and manager roles are separated I don't think it makes any sense to allow admin owner to propose. so going to remove the owner check here
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.
removed it
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
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.
Pull Request Overview
This PR introduces a whitelisting layer on top of the OptimisticOracleV2, enabling default and per-request address restrictions for both requesters and proposers, as well as owner-managed bond and liveness limits.
- Added
WhitelistOptimisticOracleV2
contract with default/custom proposer and requester whitelists. - Enforced whitelist checks in
requestPrice
andproposePriceFor
and provided admin functions to configure bounds and whitelists. - Updated
OptimisticOracleV2
and whitelist contracts to expose/internalize functions for subclassing.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
packages/core/contracts/optimistic-oracle-v2/implementation/WhitelistOptimisticOracleV2.sol | New contract layering whitelist enforcement over OptimisticOracleV2 |
packages/core/contracts/optimistic-oracle-v2/implementation/OptimisticOracleV2.sol | Updated visibilities and added virtual /internal for subclass support |
packages/core/contracts/common/interfaces/DisableableAddressWhitelistInterface.sol | New interface for togglable enforcement |
packages/core/contracts/common/implementation/DisableableAddressWhitelist.sol | Implementation allowing enforcement toggling |
packages/core/contracts/common/implementation/AddressWhitelist.sol | Changed isOnWhitelist to public virtual for override compatibility |
Comments suppressed due to low confidence (2)
packages/core/contracts/optimistic-oracle-v2/implementation/WhitelistOptimisticOracleV2.sol:163
- Add unit tests to cover the
requestPrice
path when the requester is not on the whitelist, as well as success cases.
function requestPrice(
packages/core/contracts/optimistic-oracle-v2/implementation/WhitelistOptimisticOracleV2.sol:363
- [nitpick] The error message is ambiguous; consider specifying
Requester whitelist cannot be the zero address
to clarify which whitelist is invalid.
require(whitelist != address(0), "Whitelist cannot be zero address");
* @return totalBond new bond + final fee that the proposer and disputer will be required to pay. This can be | ||
* changed again with a subsequent call to setBond(). | ||
*/ | ||
function requestManagerSetBond( |
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.
[nitpick] Consider emitting an event (e.g., RequestBondUpdated
) when a request manager updates the bond, to aid off-chain indexing and auditing.
Copilot uses AI. Check for mistakes.
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.
not yet sure on this as the parent contract is not emitting events on this
* @param ancillaryData ancillary data of the price being requested. | ||
* @param customLiveness new custom liveness. | ||
*/ | ||
function requestManagerSetCustomLiveness( |
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.
[nitpick] It may be helpful to emit an event (e.g., RequestLivenessUpdated
) when custom liveness is set, improving transparency of changes.
Copilot uses AI. Check for mistakes.
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.
not yet sure on this as the parent contract is not emitting events on this
bytes memory ancillaryData | ||
) external view returns (address[] memory allowedProposers, bool isEnforced) { | ||
DisableableAddressWhitelistInterface whitelist = | ||
_getEffectiveProposerWhitelist(requester, identifier, ancillaryData); | ||
isEnforced = whitelist.isEnforced(); | ||
allowedProposers = isEnforced ? whitelist.getWhitelist() : new address[](0); | ||
return (allowedProposers, isEnforced); |
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.
Fetching the entire whitelist array can be expensive if it grows large; consider pagination or limiting the maximum size for on-chain calls.
bytes memory ancillaryData | |
) external view returns (address[] memory allowedProposers, bool isEnforced) { | |
DisableableAddressWhitelistInterface whitelist = | |
_getEffectiveProposerWhitelist(requester, identifier, ancillaryData); | |
isEnforced = whitelist.isEnforced(); | |
allowedProposers = isEnforced ? whitelist.getWhitelist() : new address[](0); | |
return (allowedProposers, isEnforced); | |
bytes memory ancillaryData, | |
uint256 startIndex, | |
uint256 pageSize | |
) external view returns (address[] memory paginatedProposers, bool isEnforced) { | |
DisableableAddressWhitelistInterface whitelist = | |
_getEffectiveProposerWhitelist(requester, identifier, ancillaryData); | |
isEnforced = whitelist.isEnforced(); | |
if (isEnforced) { | |
address[] memory fullWhitelist = whitelist.getWhitelist(); | |
uint256 endIndex = startIndex + pageSize > fullWhitelist.length | |
? fullWhitelist.length | |
: startIndex + pageSize; | |
paginatedProposers = new address[](endIndex - startIndex); | |
for (uint256 i = startIndex; i < endIndex; i++) { | |
paginatedProposers[i - startIndex] = fullWhitelist[i]; | |
} | |
} else { | |
paginatedProposers = new address[](0); | |
} | |
return (paginatedProposers, isEnforced); |
Copilot uses AI. Check for mistakes.
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 is meant for offchain usage only
function isOnWhitelist(address elementToCheck) | ||
public | ||
view | ||
override(AddressWhitelist, AddressWhitelistInterface) |
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 we add the nonReentrantView()
that the parent function has?
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.
yes, added it here, as well as to setWhitelistEnforcement
uint256 timestamp, | ||
bytes memory ancillaryData, | ||
IERC20 currency, | ||
uint256 reward |
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 we add the nonReentrant() modifier that the parent has?
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'm afraid the _preEntranceCheck
in parent would fail then?
bytes32 identifier, | ||
uint256 timestamp, | ||
bytes memory ancillaryData, | ||
int256 proposedPrice |
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 we add nonReentrant()?
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'm afraid the _preEntranceCheck
in parent would fail then?
Signed-off-by: Reinis Martinsons <[email protected]>
mapping(bytes32 => AddressWhitelistInterface) public customProposerWhitelists; | ||
|
||
// Owner controlled bounds limiting the changes that can be made by request managers. | ||
uint256 public maximumBond; |
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 assumes only a single bond collateral will be used. consider making this a mapping of collateral token address to maximumBond.
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.
good call, will implement this
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.
added in 0888783
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
_getEffectiveProposerWhitelist(requester, identifier, ancillaryData); | ||
|
||
require(whitelist.isOnWhitelist(proposer), "Proposer not whitelisted"); | ||
require(whitelist.isOnWhitelist(msg.sender), "Sender not whitelisted"); |
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.
Question: Do we expect msg.sender to ever differ from the proposer here?
I would have assumed msg.sender == proposer
, effectively making proposePriceFor
behave like proposePrice
conceptually. So we should only need to check that msg.sender
is whitelisted.
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.
yes, it might not be strictly necessary to check for proposer
, but since the msg.sender
is not emitted in events it would be harder to curate the whitelist in case the admin needs to remove proposers with high dispute rate. Otherwise a msg.sender
that is part of whitelist could submit bad proposals using different proposer address making it harder for the manager to analyze. Its still possible to track the msg.sender
, but that would require inspecting transaction call traces that puts extra burden on maintainer.
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 a follow-up question in proposePriceFor whitelist requires: https://github.com/UMAprotocol/protocol/pull/4841/files#r2216241197
Signed-off-by: Reinis Martinsons <[email protected]>
No description provided.