Skip to content

Commit

Permalink
chore: review TODOs (#32)
Browse files Browse the repository at this point in the history
  • Loading branch information
gas1cent authored Jun 28, 2023
1 parent e77bb4b commit c484288
Show file tree
Hide file tree
Showing 11 changed files with 13 additions and 17 deletions.
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
3 changes: 1 addition & 2 deletions solidity/contracts/Oracle.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);

Expand Down
2 changes: 1 addition & 1 deletion solidity/contracts/extensions/AccountingExtension.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion solidity/contracts/extensions/BondEscalationAccounting.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
4 changes: 2 additions & 2 deletions solidity/contracts/modules/BondEscalationResolutionModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand Down
2 changes: 0 additions & 2 deletions solidity/contracts/modules/BondedResponseModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
8 changes: 4 additions & 4 deletions solidity/contracts/modules/ERC20ResolutonModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 1 addition & 1 deletion solidity/test/integration/Oracle.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -176,5 +176,5 @@ contract IntegrationOracle is IntegrationBase {
);
}

// TODO: Test disputes and slashing
// TODO: [OPO-55] Test disputes and slashing
}
1 change: 0 additions & 1 deletion solidity/test/unit/BondEscalationModule.t.sol
Original file line number Diff line number Diff line change
@@ -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';

Expand Down
2 changes: 1 addition & 1 deletion solidity/test/unit/BondEscalationResolutionModule.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ contract BondEscalationResolutionModule_UnitTest is Test {

assertEq(_startTime, uint128(block.timestamp));

// TODO: expect event emission
// TODO: [OPO-89] expect event emission
}

////////////////////////////////////////////////////////////////////
Expand Down

0 comments on commit c484288

Please sign in to comment.