From c484288105973bbad3eef8b9719ceba4abcd70fd Mon Sep 17 00:00:00 2001 From: Gas <86567384+gas1cent@users.noreply.github.com> Date: Wed, 28 Jun 2023 15:47:59 +0400 Subject: [PATCH] chore: review TODOs (#32) --- README.md | 2 +- solidity/contracts/Oracle.sol | 3 +-- solidity/contracts/extensions/AccountingExtension.sol | 2 +- .../contracts/extensions/BondEscalationAccounting.sol | 2 +- .../contracts/modules/BondEscalationResolutionModule.sol | 4 ++-- solidity/contracts/modules/BondedResponseModule.sol | 2 -- solidity/contracts/modules/ERC20ResolutonModule.sol | 8 ++++---- .../modules/IBondEscalationResolutionModule.sol | 2 +- solidity/test/integration/Oracle.t.sol | 2 +- solidity/test/unit/BondEscalationModule.t.sol | 1 - solidity/test/unit/BondEscalationResolutionModule.t.sol | 2 +- 11 files changed, 13 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 75e9bf2d..de0bbfcc 100644 --- a/README.md +++ b/README.md @@ -35,7 +35,7 @@ Make sure to set `OPTIMISM_RPC` environment variable before running end-to-end t ## Licensing -The primary license for OpOO contracts is the GNU Affero General Public License 3.0 (`AGPL-3.0`), see [`LICENSE`](./LICENSE). +The primary license for OpOO contracts is MIT, see [`LICENSE`](./LICENSE). ## Contributors diff --git a/solidity/contracts/Oracle.sol b/solidity/contracts/Oracle.sol index 6ed2c8ad..cfe4c5ee 100644 --- a/solidity/contracts/Oracle.sol +++ b/solidity/contracts/Oracle.sol @@ -43,7 +43,7 @@ contract Oracle is IOracle { } } - // TODO: Same as `createRequest` but with multiple requests passed in as an array + // TODO: [OPO-86] Same as `createRequest` but with multiple requests passed in as an array function createRequests(bytes[] calldata _requestsData) external returns (bytes32[] memory _requestsIds) {} function listRequests(uint256 _startFrom, uint256 _batchSize) external view returns (Request[] memory _list) { @@ -189,7 +189,6 @@ contract Oracle is IOracle { _ids = _responseIds[_requestId]; } - // TODO: discuss - should the Oracle have any reverts other than checking for empty values, or does this become too opinionated? function finalize(bytes32 _requestId, bytes32 _finalizedResponseId) external { if (_finalizedResponses[_requestId].createdAt != 0) revert Oracle_AlreadyFinalized(_requestId); diff --git a/solidity/contracts/extensions/AccountingExtension.sol b/solidity/contracts/extensions/AccountingExtension.sol index 7205f33d..22332b58 100644 --- a/solidity/contracts/extensions/AccountingExtension.sol +++ b/solidity/contracts/extensions/AccountingExtension.sol @@ -68,7 +68,7 @@ contract AccountingExtension is IAccountingExtension { // If weth, unwrap if (_token == IERC20(address(WETH))) { - // TODO: should we just send back the WETH using a safeTransferFrom instead of unwrapping? + // TODO: [OPO-145] Replace plain value transfers with ERC20 transfers WETH.withdraw(_amount); payable(msg.sender).transfer(_amount); } else { diff --git a/solidity/contracts/extensions/BondEscalationAccounting.sol b/solidity/contracts/extensions/BondEscalationAccounting.sol index 4d7f8af5..8625f86b 100644 --- a/solidity/contracts/extensions/BondEscalationAccounting.sol +++ b/solidity/contracts/extensions/BondEscalationAccounting.sol @@ -39,7 +39,7 @@ contract BondEscalationAccounting is AccountingExtension, IBondEscalationAccount pledges[_requestId][_disputeId][_token] += _amount; } - // TODO: add _disputeId parameter + // TODO: [OPO-89] add _disputeId parameter emit Pledge(_pledger, _requestId, _token, _amount); } diff --git a/solidity/contracts/modules/BondEscalationResolutionModule.sol b/solidity/contracts/modules/BondEscalationResolutionModule.sol index bc615103..5d027ab6 100644 --- a/solidity/contracts/modules/BondEscalationResolutionModule.sol +++ b/solidity/contracts/modules/BondEscalationResolutionModule.sol @@ -376,7 +376,7 @@ contract BondEscalationResolutionModule is Module, IBondEscalationResolutionModu } } _accountingExtension.payWinningPledgers(_requestId, _disputeId, _winningPledgers, _token, _amountPerPledger); - // TODO: emit event + // TODO: [OPO-89] emit event return; } @@ -392,7 +392,7 @@ contract BondEscalationResolutionModule is Module, IBondEscalationResolutionModu } } _accountingExtension.payWinningPledgers(_requestId, _disputeId, _winningPledgers, _token, _amountPerPledger); - // TODO: emit event + // TODO: [OPO-89] emit event return; } diff --git a/solidity/contracts/modules/BondedResponseModule.sol b/solidity/contracts/modules/BondedResponseModule.sol index f0248097..ca743f29 100644 --- a/solidity/contracts/modules/BondedResponseModule.sol +++ b/solidity/contracts/modules/BondedResponseModule.sol @@ -53,8 +53,6 @@ contract BondedResponseModule is Module, IBondedResponseModule { if (_dispute.status == IOracle.DisputeStatus.Lost) revert BondedResponseModule_AlreadyResponded(); } - // TODO: Allow only one response per proposer? - // NG: Due to the bonding aspect of the mechanism, I think it's okay to allow multiple responses per proposer _response = IOracle.Response({ requestId: _requestId, disputeId: bytes32(0), diff --git a/solidity/contracts/modules/ERC20ResolutonModule.sol b/solidity/contracts/modules/ERC20ResolutonModule.sol index 353c383f..97de69e9 100644 --- a/solidity/contracts/modules/ERC20ResolutonModule.sol +++ b/solidity/contracts/modules/ERC20ResolutonModule.sol @@ -65,7 +65,7 @@ contract ERC20ResolutionModule is Module, IERC20ResolutionModule { if (_disputerBondSize != 0) { // seize disputer bond until resolution - this allows for voters not having to call deposit in the accounting extension - // todo: should another event be emitted with disputerBond? + // TODO: should another event be emitted with disputerBond? _accounting.pay(_requestId, _dispute.disputer, address(this), _token, _disputerBondSize); _accounting.withdraw(_token, _disputerBondSize); escalationData[_disputeId].disputerBond = _disputerBondSize; @@ -77,7 +77,7 @@ contract ERC20ResolutionModule is Module, IERC20ResolutionModule { // casts vote in favor of dispute function castVote(bytes32 _requestId, bytes32 _disputeId, uint256 _numberOfVotes) public { /* - 1. Check that the disputeId is Escalated - todo + 1. Check that the disputeId is Escalated - TODO 2. Check that the voting deadline is not over 3. Transfer tokens from msg.sender to this address and increase the mapping 4. Emit VoteCast event @@ -124,7 +124,7 @@ contract ERC20ResolutionModule is Module, IERC20ResolutionModule { uint256 _deadline = _escalationData.startTime + _timeUntilDeadline; if (block.timestamp < _deadline) revert ERC20ResolutionModule_OnGoingVotingPhase(); - // 3. Check quorum - todo: check if this is precise- think if using totalSupply makes sense, perhaps minQuorum can be + // 3. Check quorum - TODO: check if this is precise- think if using totalSupply makes sense, perhaps minQuorum can be // min amount of tokens required instead of a percentage // not sure if safe but the actual formula is _token.totalSupply() * _minQuorum * BASE(100) / 100 so base disappears // i guess with a shit token someone could front run this call and increase totalSupply enough for this to fail @@ -141,7 +141,7 @@ contract ERC20ResolutionModule is Module, IERC20ResolutionModule { // 5. Pay and Release if (_quorumReached == 1) { for (uint256 _i; _i < _voterData.length;) { - // todo: check math -- remember _numVotesForQuorum is escalated + // TODO: check math -- remember _numVotesForQuorum is escalated _amountToPay = _disputerBond == 0 ? _voterData[_i].numOfVotes : _voterData[_i].numOfVotes + (_voterData[_i].numOfVotes * _numVotesForQuorum / _disputerBond * BASE); diff --git a/solidity/interfaces/modules/IBondEscalationResolutionModule.sol b/solidity/interfaces/modules/IBondEscalationResolutionModule.sol index 83c6adc2..7fd59e96 100644 --- a/solidity/interfaces/modules/IBondEscalationResolutionModule.sol +++ b/solidity/interfaces/modules/IBondEscalationResolutionModule.sol @@ -38,7 +38,7 @@ interface IBondEscalationResolutionModule is IResolutionModule { uint256 pledgesAgainst; } - // TODO: should I add requestId as a param? + // TODO: [OPO-89] should I add requestId as a param? event DisputeResolved(bytes32 indexed _disputeId, IOracle.DisputeStatus _status); event DisputeEscalated(bytes32 indexed _disputeId, bytes32 indexed requestId); event PledgedForDispute( diff --git a/solidity/test/integration/Oracle.t.sol b/solidity/test/integration/Oracle.t.sol index 680eb5c0..ccbb525e 100644 --- a/solidity/test/integration/Oracle.t.sol +++ b/solidity/test/integration/Oracle.t.sol @@ -176,5 +176,5 @@ contract IntegrationOracle is IntegrationBase { ); } - // TODO: Test disputes and slashing + // TODO: [OPO-55] Test disputes and slashing } diff --git a/solidity/test/unit/BondEscalationModule.t.sol b/solidity/test/unit/BondEscalationModule.t.sol index 9140e65b..7d3023d1 100644 --- a/solidity/test/unit/BondEscalationModule.t.sol +++ b/solidity/test/unit/BondEscalationModule.t.sol @@ -1,7 +1,6 @@ // SPDX-License-Identifier: AGPL-3.0-only pragma solidity ^0.8.19; -//TODO: improve natspec -- cursed // solhint-disable-next-line import 'forge-std/Test.sol'; diff --git a/solidity/test/unit/BondEscalationResolutionModule.t.sol b/solidity/test/unit/BondEscalationResolutionModule.t.sol index 634f7203..c1354977 100644 --- a/solidity/test/unit/BondEscalationResolutionModule.t.sol +++ b/solidity/test/unit/BondEscalationResolutionModule.t.sol @@ -180,7 +180,7 @@ contract BondEscalationResolutionModule_UnitTest is Test { assertEq(_startTime, uint128(block.timestamp)); - // TODO: expect event emission + // TODO: [OPO-89] expect event emission } ////////////////////////////////////////////////////////////////////