From 55afd31001928e7075c506bf985911b7b79c8bcb Mon Sep 17 00:00:00 2001 From: Ashitaka Date: Mon, 8 Jul 2024 19:26:53 -0300 Subject: [PATCH 1/4] fix: adding escalated status --- .../resolution/PrivateERC20ResolutionModule.sol | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol b/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol index 4019f307..eb5a5be7 100644 --- a/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol +++ b/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol @@ -54,7 +54,10 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule { function commitVote(IOracle.Request calldata _request, IOracle.Dispute calldata _dispute, bytes32 _commitment) public { bytes32 _disputeId = _getId(_dispute); if (ORACLE.createdAt(_disputeId) == 0) revert PrivateERC20ResolutionModule_NonExistentDispute(); - if (ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.None) { + if ( + ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.None + && ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.Escalated + ) { revert PrivateERC20ResolutionModule_AlreadyResolved(); } @@ -114,7 +117,10 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule { IOracle.Dispute calldata _dispute ) external onlyOracle { if (ORACLE.createdAt(_disputeId) == 0) revert PrivateERC20ResolutionModule_NonExistentDispute(); - if (ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.None) { + if ( + ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.None + && ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.Escalated + ) { revert PrivateERC20ResolutionModule_AlreadyResolved(); } From bd1378e75bda69ee700066077a10285a30488e43 Mon Sep 17 00:00:00 2001 From: Ashitaka Date: Fri, 12 Jul 2024 21:43:04 -0300 Subject: [PATCH 2/4] fix: comments --- .../PrivateERC20ResolutionModule.t.sol | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol b/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol index 11ebc992..ea37a020 100644 --- a/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol +++ b/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol @@ -239,6 +239,68 @@ contract PrivateERC20ResolutionModule_Unit_CommitVote is BaseTest { module.commitVote(mockRequest, mockDispute, _commitment); } + /** + * @notice Test that `commitVote` reverts if called with `_disputeId` of an already active dispute. + */ + function test_revertIfActive(bytes32 _requestId, bytes32 _commitment) public { + // Computer proper IDs + mockDispute.requestId = _requestId; + bytes32 _disputeId = _getId(mockDispute); + + // Mock and expect IOracle.createdAt to be called + _mockAndExpect(address(oracle), abi.encodeCall(IOracle.createdAt, (_disputeId)), abi.encode(1)); + // Mock and expect IOracle.disputeStatus to be called + _mockAndExpect( + address(oracle), abi.encodeCall(IOracle.disputeStatus, (_disputeId)), abi.encode(IOracle.DisputeStatus.Active) + ); + + // Check: does it revert if the dispute is already resolved? + vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_AlreadyResolved.selector); + module.commitVote(mockRequest, mockDispute, _commitment); + } + + /** + * @notice Test that `commitVote` reverts if called with `_disputeId` of a dispute with no resolution. + */ + function test_revertIfNoResolution(bytes32 _requestId, bytes32 _commitment) public { + // Computer proper IDs + mockDispute.requestId = _requestId; + bytes32 _disputeId = _getId(mockDispute); + + // Mock and expect IOracle.createdAt to be called + _mockAndExpect(address(oracle), abi.encodeCall(IOracle.createdAt, (_disputeId)), abi.encode(1)); + // Mock and expect IOracle.disputeStatus to be called + _mockAndExpect( + address(oracle), + abi.encodeCall(IOracle.disputeStatus, (_disputeId)), + abi.encode(IOracle.DisputeStatus.NoResolution) + ); + + // Check: does it revert if the dispute is already resolved? + vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_AlreadyResolved.selector); + module.commitVote(mockRequest, mockDispute, _commitment); + } + + /** + * @notice Test that `commitVote` reverts if called with `_disputeId` of a dispute that has already been won. + */ + function test_revertIfWon(bytes32 _requestId, bytes32 _commitment) public { + // Computer proper IDs + mockDispute.requestId = _requestId; + bytes32 _disputeId = _getId(mockDispute); + + // Mock and expect IOracle.createdAt to be called + _mockAndExpect(address(oracle), abi.encodeCall(IOracle.createdAt, (_disputeId)), abi.encode(1)); + // Mock and expect IOracle.disputeStatus to be called + _mockAndExpect( + address(oracle), abi.encodeCall(IOracle.disputeStatus, (_disputeId)), abi.encode(IOracle.DisputeStatus.Won) + ); + + // Check: does it revert if the dispute is already resolved? + vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_AlreadyResolved.selector); + module.commitVote(mockRequest, mockDispute, _commitment); + } + /** * @notice Test that `commitVote` reverts if called with `_disputeId` of an already resolved dispute. */ From cdecadbf1ecc4d77f01dd532a8069a826dc57877 Mon Sep 17 00:00:00 2001 From: Ashitaka Date: Wed, 17 Jul 2024 15:36:28 -0300 Subject: [PATCH 3/4] feat: tests --- .../PrivateERC20ResolutionModule.t.sol | 130 ++++++++++++++++++ 1 file changed, 130 insertions(+) diff --git a/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol b/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol index ea37a020..c2d48728 100644 --- a/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol +++ b/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol @@ -668,4 +668,134 @@ contract PrivateERC20ResolutionModule_Unit_ResolveDispute is BaseTest { module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute); } } + + function test_revertIfActive() public { + // Set request data + mockRequest.resolutionModuleData = abi.encode( + IPrivateERC20ResolutionModule.RequestParameters({ + accountingExtension: IAccountingExtension(makeAddr('AccountingExtension')), + votingToken: token, + minVotesForQuorum: 1, + committingTimeWindow: 500_000, + revealingTimeWindow: 1_000_000 + }) + ); + + // Compute proper ids + bytes32 _requestId = _getId(mockRequest); + mockDispute.requestId = _requestId; + bytes32 _disputeId = _getId(mockDispute); + + module.forTest_setStartTime(_disputeId, 1); + + // Mock and expect IOracle.createdAt to be called + _mockAndExpect(address(oracle), abi.encodeCall(IOracle.createdAt, (_disputeId)), abi.encode(1)); + // Mock and expect IOracle.disputeStatus to be called + _mockAndExpect( + address(oracle), abi.encodeCall(IOracle.disputeStatus, (_disputeId)), abi.encode(IOracle.DisputeStatus.Active) + ); + + // Check: does it revert if the dispute is already resolved? + vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_AlreadyResolved.selector); + vm.prank(address(oracle)); + module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute); + } + + function test_revertIfWon() public { + // Set request data + mockRequest.resolutionModuleData = abi.encode( + IPrivateERC20ResolutionModule.RequestParameters({ + accountingExtension: IAccountingExtension(makeAddr('AccountingExtension')), + votingToken: token, + minVotesForQuorum: 1, + committingTimeWindow: 500_000, + revealingTimeWindow: 1_000_000 + }) + ); + + // Compute proper ids + bytes32 _requestId = _getId(mockRequest); + mockDispute.requestId = _requestId; + bytes32 _disputeId = _getId(mockDispute); + + module.forTest_setStartTime(_disputeId, 1); + + // Mock and expect IOracle.createdAt to be called + _mockAndExpect(address(oracle), abi.encodeCall(IOracle.createdAt, (_disputeId)), abi.encode(1)); + // Mock and expect IOracle.disputeStatus to be called + _mockAndExpect( + address(oracle), abi.encodeCall(IOracle.disputeStatus, (_disputeId)), abi.encode(IOracle.DisputeStatus.Won) + ); + + // Check: does it revert if the dispute is already resolved? + vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_AlreadyResolved.selector); + vm.prank(address(oracle)); + module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute); + } + + function test_revertIfLost() public { + // Set request data + mockRequest.resolutionModuleData = abi.encode( + IPrivateERC20ResolutionModule.RequestParameters({ + accountingExtension: IAccountingExtension(makeAddr('AccountingExtension')), + votingToken: token, + minVotesForQuorum: 1, + committingTimeWindow: 500_000, + revealingTimeWindow: 1_000_000 + }) + ); + + // Compute proper ids + bytes32 _requestId = _getId(mockRequest); + mockDispute.requestId = _requestId; + bytes32 _disputeId = _getId(mockDispute); + + module.forTest_setStartTime(_disputeId, 1); + + // Mock and expect IOracle.createdAt to be called + _mockAndExpect(address(oracle), abi.encodeCall(IOracle.createdAt, (_disputeId)), abi.encode(1)); + // Mock and expect IOracle.disputeStatus to be called + _mockAndExpect( + address(oracle), abi.encodeCall(IOracle.disputeStatus, (_disputeId)), abi.encode(IOracle.DisputeStatus.Lost) + ); + + // Check: does it revert if the dispute is already resolved? + vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_AlreadyResolved.selector); + vm.prank(address(oracle)); + module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute); + } + + function test_revertIfNonResolve() public { + // Set request data + mockRequest.resolutionModuleData = abi.encode( + IPrivateERC20ResolutionModule.RequestParameters({ + accountingExtension: IAccountingExtension(makeAddr('AccountingExtension')), + votingToken: token, + minVotesForQuorum: 1, + committingTimeWindow: 500_000, + revealingTimeWindow: 1_000_000 + }) + ); + + // Compute proper ids + bytes32 _requestId = _getId(mockRequest); + mockDispute.requestId = _requestId; + bytes32 _disputeId = _getId(mockDispute); + + module.forTest_setStartTime(_disputeId, 1); + + // Mock and expect IOracle.createdAt to be called + _mockAndExpect(address(oracle), abi.encodeCall(IOracle.createdAt, (_disputeId)), abi.encode(1)); + // Mock and expect IOracle.disputeStatus to be called + _mockAndExpect( + address(oracle), + abi.encodeCall(IOracle.disputeStatus, (_disputeId)), + abi.encode(IOracle.DisputeStatus.NoResolution) + ); + + // Check: does it revert if the dispute is already resolved? + vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_AlreadyResolved.selector); + vm.prank(address(oracle)); + module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute); + } } From 52ceaabd179240be5e2144c17d3685a22ccc638f Mon Sep 17 00:00:00 2001 From: Ashitaka Date: Wed, 17 Jul 2024 16:51:37 -0300 Subject: [PATCH 4/4] fix: comments --- .../resolution/PrivateERC20ResolutionModule.sol | 10 ++-------- .../resolution/PrivateERC20ResolutionModule.t.sol | 14 ++++++++------ 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol b/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol index 134974f6..f0c0cfd1 100644 --- a/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol +++ b/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol @@ -54,10 +54,7 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule { function commitVote(IOracle.Request calldata _request, IOracle.Dispute calldata _dispute, bytes32 _commitment) public { bytes32 _disputeId = _getId(_dispute); if (ORACLE.createdAt(_disputeId) == 0) revert PrivateERC20ResolutionModule_NonExistentDispute(); - if ( - ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.None - && ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.Escalated - ) { + if (ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.Escalated) { revert PrivateERC20ResolutionModule_AlreadyResolved(); } @@ -117,10 +114,7 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule { IOracle.Dispute calldata _dispute ) external onlyOracle { if (ORACLE.createdAt(_disputeId) == 0) revert PrivateERC20ResolutionModule_NonExistentDispute(); - if ( - ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.None - && ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.Escalated - ) { + if (ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.Escalated) { revert PrivateERC20ResolutionModule_AlreadyResolved(); } diff --git a/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol b/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol index c2d48728..a6ec44a2 100644 --- a/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol +++ b/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol @@ -87,7 +87,9 @@ contract BaseTest is Test, Helpers { _mockAndExpect(address(oracle), abi.encodeCall(IOracle.createdAt, (_disputeId)), abi.encode(1)); _mockAndExpect( - address(oracle), abi.encodeCall(IOracle.disputeStatus, (_disputeId)), abi.encode(IOracle.DisputeStatus.None) + address(oracle), + abi.encodeCall(IOracle.disputeStatus, (_disputeId)), + abi.encode(IOracle.DisputeStatus.Escalated) ); module.commitVote(_request, _dispute, _commitment); @@ -196,7 +198,7 @@ contract PrivateERC20ResolutionModule_Unit_CommitVote is BaseTest { _mockAndExpect(address(oracle), abi.encodeCall(IOracle.createdAt, (_disputeId)), abi.encode(1)); // Mock and expect IOracle.disputeStatus to be called _mockAndExpect( - address(oracle), abi.encodeCall(IOracle.disputeStatus, (_disputeId)), abi.encode(IOracle.DisputeStatus.None) + address(oracle), abi.encodeCall(IOracle.disputeStatus, (_disputeId)), abi.encode(IOracle.DisputeStatus.Escalated) ); // Check: does it revert if no commitment is given? @@ -333,7 +335,7 @@ contract PrivateERC20ResolutionModule_Unit_CommitVote is BaseTest { _mockAndExpect(address(oracle), abi.encodeCall(IOracle.createdAt, (_disputeId)), abi.encode(1)); // Mock and expect IOracle.disputeStatus to be called _mockAndExpect( - address(oracle), abi.encodeCall(IOracle.disputeStatus, (_disputeId)), abi.encode(IOracle.DisputeStatus.None) + address(oracle), abi.encodeCall(IOracle.disputeStatus, (_disputeId)), abi.encode(IOracle.DisputeStatus.Escalated) ); // Check: reverts if dispute is not escalated? == no escalation data @@ -373,7 +375,7 @@ contract PrivateERC20ResolutionModule_Unit_CommitVote is BaseTest { _mockAndExpect(address(oracle), abi.encodeCall(IOracle.createdAt, (_disputeId)), abi.encode(1)); // Mock and expect IOracle.disputeStatus to be called _mockAndExpect( - address(oracle), abi.encodeCall(IOracle.disputeStatus, (_disputeId)), abi.encode(IOracle.DisputeStatus.None) + address(oracle), abi.encodeCall(IOracle.disputeStatus, (_disputeId)), abi.encode(IOracle.DisputeStatus.Escalated) ); // Check: does it revert if the committing phase is over? @@ -607,7 +609,7 @@ contract PrivateERC20ResolutionModule_Unit_ResolveDispute is BaseTest { // Mock and expect IOracle.disputeStatus to be called _mockAndExpect( - address(oracle), abi.encodeCall(IOracle.disputeStatus, (_disputeId)), abi.encode(IOracle.DisputeStatus.None) + address(oracle), abi.encodeCall(IOracle.disputeStatus, (_disputeId)), abi.encode(IOracle.DisputeStatus.Escalated) ); // Check: is the event emitted? @@ -650,7 +652,7 @@ contract PrivateERC20ResolutionModule_Unit_ResolveDispute is BaseTest { _mockAndExpect(address(oracle), abi.encodeCall(IOracle.createdAt, (_disputeId)), abi.encode(1)); // Mock and expect IOracle.disputeStatus to be called _mockAndExpect( - address(oracle), abi.encodeCall(IOracle.disputeStatus, (_disputeId)), abi.encode(IOracle.DisputeStatus.None) + address(oracle), abi.encodeCall(IOracle.disputeStatus, (_disputeId)), abi.encode(IOracle.DisputeStatus.Escalated) ); // Jump to timestamp