Skip to content
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.

Sign up for GitHub

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: standardized timestamp comparison #72

Merged
merged 4 commits into from
Nov 4, 2024
Merged

feat: standardized timestamp comparison #72

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
679c9c3
feat: standardized timestamp comparison
0xShaito Oct 23, 2024
7cf7b75
fix: wrong phase test
0xShaito Oct 23, 2024
571f0a2
fix: better variable creation
0xShaito Oct 23, 2024
3e7af34
Merge branch 'dev' into feat/standardized-timestamp-comparison
gas1cent Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions solidity/contracts/modules/dispute/BondEscalationModule.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ contract BondEscalationModule is Module, IBondEscalationModule {
RequestParameters memory _params = decodeRequestData(_request.disputeModuleData);
BondEscalation storage _escalation = _escalations[_dispute.requestId];

if (block.timestamp > ORACLE.responseCreatedAt(_dispute.responseId) + _params.disputeWindow) {
if (block.timestamp >= ORACLE.responseCreatedAt(_dispute.responseId) + _params.disputeWindow) {
revert BondEscalationModule_DisputeWindowOver();
}

Expand Down Expand Up @@ -82,7 +82,7 @@ contract BondEscalationModule is Module, IBondEscalationModule {
if (_disputeStatus == IOracle.DisputeStatus.Escalated) {
// The dispute has been escalated to the Resolution module
// Make sure the bond escalation deadline has passed and update the status
if (block.timestamp <= ORACLE.disputeCreatedAt(_disputeId) + _params.bondEscalationDeadline) {
if (block.timestamp < ORACLE.disputeCreatedAt(_disputeId) + _params.bondEscalationDeadline) {
revert BondEscalationModule_BondEscalationNotOver();
}

Expand Down Expand Up @@ -254,7 +254,7 @@ contract BondEscalationModule is Module, IBondEscalationModule {
BondEscalation storage _escalation = _escalations[_dispute.requestId];

uint256 _disputeCreatedAt = ORACLE.disputeCreatedAt(_disputeId);
if (block.timestamp <= _disputeCreatedAt + _params.bondEscalationDeadline + _params.tyingBuffer) {
if (block.timestamp < _disputeCreatedAt + _params.bondEscalationDeadline + _params.tyingBuffer) {
revert BondEscalationModule_BondEscalationNotOver();
}

Expand Down Expand Up @@ -302,7 +302,7 @@ contract BondEscalationModule is Module, IBondEscalationModule {
_params = decodeRequestData(_request.disputeModuleData);

uint256 _disputeCreatedAt = ORACLE.disputeCreatedAt(_disputeId);
if (block.timestamp > _disputeCreatedAt + _params.bondEscalationDeadline + _params.tyingBuffer) {
if (block.timestamp >= _disputeCreatedAt + _params.bondEscalationDeadline + _params.tyingBuffer) {
revert BondEscalationModule_BondEscalationOver();
}

Expand All @@ -322,7 +322,7 @@ contract BondEscalationModule is Module, IBondEscalationModule {
}

if (
block.timestamp > _disputeCreatedAt + _params.bondEscalationDeadline
block.timestamp >= _disputeCreatedAt + _params.bondEscalationDeadline
&& _numPledgersForDispute == _numPledgersAgainstDispute
) {
revert BondEscalationModule_CannotBreakTieDuringTyingBuffer();
Expand Down
6 changes: 3 additions & 3 deletions solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,8 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule {
_escalation.startTime + _params.committingTimeWindow,
_escalation.startTime + _params.committingTimeWindow + _params.revealingTimeWindow
);
if (block.timestamp <= _revealStartTime) revert PrivateERC20ResolutionModule_OnGoingCommittingPhase();
if (block.timestamp > _revealEndTime) revert PrivateERC20ResolutionModule_RevealingPhaseOver();
if (block.timestamp < _revealStartTime) revert PrivateERC20ResolutionModule_OnGoingCommittingPhase();
if (block.timestamp >= _revealEndTime) revert PrivateERC20ResolutionModule_RevealingPhaseOver();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about forcing the use of < and inverting this comparison?

if (_revealEndTime < block.timestamp) revert PrivateERC20ResolutionModule_RevealingPhaseOver();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to leave it like this to always have the timestamp in the left side of the comparison. I think it's easier to read like that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understandable.

I agree it's easier to read, but it also forces me to think properly.


VoterData storage _voterData = _votersData[_disputeId][msg.sender];

Expand Down Expand Up @@ -123,7 +123,7 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule {
if (block.timestamp < _escalation.startTime + _params.committingTimeWindow) {
revert PrivateERC20ResolutionModule_OnGoingCommittingPhase();
}
if (block.timestamp <= _escalation.startTime + _params.committingTimeWindow + _params.revealingTimeWindow) {
if (block.timestamp < _escalation.startTime + _params.committingTimeWindow + _params.revealingTimeWindow) {
revert PrivateERC20ResolutionModule_OnGoingRevealingPhase();
}

Expand Down
2 changes: 1 addition & 1 deletion solidity/test/integration/EscalateDispute.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ contract Integration_EscalateDispute is IntegrationBase {
maxNumberOfEscalations: 1,
bondEscalationDeadline: _expectedDeadline,
tyingBuffer: 0,
disputeWindow: 0
disputeWindow: _baseDisputeWindow
})
);

Expand Down
72 changes: 69 additions & 3 deletions solidity/test/unit/modules/dispute/BondEscalationModule.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,8 @@ contract BondEscalationModule_Unit_EscalateDispute is BaseTest {
// dispute once before the bond escalation deadline is over, and that dispute goes through the escalation module.
bondEscalationModule.forTest_setEscalatedDispute(_requestId, _disputeId);

vm.warp(disputeCreatedAt + _params.bondEscalationDeadline - 1);

// Check: does it revert if the bond escalation is not over yet?
vm.expectRevert(IBondEscalationModule.BondEscalationModule_BondEscalationNotOver.selector);
vm.prank(address(oracle));
Expand Down Expand Up @@ -453,7 +455,7 @@ contract BondEscalationModule_Unit_DisputeResponse is BaseTest {
mockDispute.responseId = _responseId;

// Warp to a time after the disputeWindow is over.
vm.warp(block.timestamp + _disputeWindow + 1);
vm.warp(block.timestamp + _disputeWindow);

// Mock and expect IOracle.responseCreatedAt to be called
_mockAndExpect(address(oracle), abi.encodeCall(IOracle.responseCreatedAt, (_responseId)), abi.encode(1));
Expand Down Expand Up @@ -1045,13 +1047,44 @@ contract BondEscalationModule_Unit_PledgeForDispute is BaseTest {
_setBondEscalation(_requestId, _numForPledgers, _numAgainstPledgers);

// warp into the tyingBuffer
vm.warp(disputeCreatedAt + _params.bondEscalationDeadline + 1);
vm.warp(disputeCreatedAt + _params.bondEscalationDeadline);

// Check: does it revert if trying to tie outside of the tying buffer?
vm.expectRevert(IBondEscalationModule.BondEscalationModule_CannotBreakTieDuringTyingBuffer.selector);
bondEscalationModule.pledgeForDispute(mockRequest, _dispute);
}

/**
* @notice Tests that pledgeAgainstDispute reverts if someone tries to pledge for when the escalation is over
*/
function test_revertIfBondEscalationOver(IBondEscalationModule.RequestParameters memory _params)
public
assumeFuzzable(address(_params.accountingExtension))
{
_params.bondSize = 1000;
_params.maxNumberOfEscalations = 3;
_params.bondEscalationDeadline = bondEscalationDeadline;
_params.tyingBuffer = 1000;
mockRequest.disputeModuleData = abi.encode(_params);

(, IOracle.Dispute memory _dispute) = _getResponseAndDispute(oracle);
bytes32 _requestId = _getId(mockRequest);
bytes32 _disputeId = _getId(_dispute);

// Mock and expect
bondEscalationModule.forTest_setEscalatedDispute(_requestId, _disputeId);

uint256 _numForPledgers = 2;
uint256 _numAgainstPledgers = _numForPledgers + 1;

_setBondEscalation(_requestId, _numForPledgers, _numAgainstPledgers);

vm.warp(disputeCreatedAt + _params.bondEscalationDeadline + _params.tyingBuffer + 1);

vm.expectRevert(IBondEscalationModule.BondEscalationModule_BondEscalationOver.selector);
bondEscalationModule.pledgeForDispute(mockRequest, _dispute);
}

/**
* @notice Tests that pledgeForDispute is called successfully
*/
Expand Down Expand Up @@ -1236,13 +1269,44 @@ contract BondEscalationModule_Unit_PledgeAgainstDispute is BaseTest {

_setBondEscalation(_requestId, _numForPledgers, _numAgainstPledgers);

vm.warp(disputeCreatedAt + _params.bondEscalationDeadline + 1);
vm.warp(disputeCreatedAt + _params.bondEscalationDeadline);

// Check: does it revert if trying to tie outside of the tying buffer?
vm.expectRevert(IBondEscalationModule.BondEscalationModule_CannotBreakTieDuringTyingBuffer.selector);
bondEscalationModule.pledgeAgainstDispute(mockRequest, _dispute);
}

/**
* @notice Tests that pledgeAgainstDispute reverts if someone tries to pledge against when the escalation is over
*/
function test_revertIfBondEscalationOver(IBondEscalationModule.RequestParameters memory _params)
public
assumeFuzzable(address(_params.accountingExtension))
{
_params.bondSize = 1000;
_params.maxNumberOfEscalations = 3;
_params.bondEscalationDeadline = bondEscalationDeadline;
_params.tyingBuffer = 1000;
mockRequest.disputeModuleData = abi.encode(_params);

(, IOracle.Dispute memory _dispute) = _getResponseAndDispute(oracle);
bytes32 _requestId = _getId(mockRequest);
bytes32 _disputeId = _getId(_dispute);

// Mock and expect
bondEscalationModule.forTest_setEscalatedDispute(_requestId, _disputeId);

uint256 _numForPledgers = 2;
uint256 _numAgainstPledgers = _numForPledgers + 1;

_setBondEscalation(_requestId, _numForPledgers, _numAgainstPledgers);

vm.warp(disputeCreatedAt + _params.bondEscalationDeadline + _params.tyingBuffer + 1);

vm.expectRevert(IBondEscalationModule.BondEscalationModule_BondEscalationOver.selector);
bondEscalationModule.pledgeAgainstDispute(mockRequest, _dispute);
}

/**
* @notice Tests that pledgeAgainstDispute is called successfully
*/
Expand Down Expand Up @@ -1331,6 +1395,8 @@ contract BondEscalationModule_Unit_SettleBondEscalation is BaseTest {
address(oracle), abi.encodeCall(IOracle.disputeCreatedAt, (_getId(_dispute))), abi.encode(disputeCreatedAt)
);

vm.warp(disputeCreatedAt + bondEscalationDeadline - 1);

// Check: does it revert if the bond escalation is not over?
vm.expectRevert(IBondEscalationModule.BondEscalationModule_BondEscalationNotOver.selector);
bondEscalationModule.settleBondEscalation(mockRequest, _response, _dispute);
Expand Down
36 changes: 20 additions & 16 deletions solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
}
}

contract BaseTest is Test, Helpers {

Check warning on line 46 in solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol

View workflow job for this annotation

GitHub Actions / Run Linters (16.x) 

The order of data in contract BaseTest should be: Constants, Immutable variables, State Variables
// The target contract
ForTest_PrivateERC20ResolutionModule public module;
// A mock oracle
Expand All @@ -51,6 +51,10 @@
// A mock token
IERC20 public token;

uint256 public constant REVEALING_TIME_WINDOW = 40_000;
uint256 public constant START_TIME = 100_000;
uint256 public constant END_TIME = START_TIME + REVEALING_TIME_WINDOW;

// Events
event CommittingPhaseStarted(uint256 _startTime, bytes32 _disputeId);
event VoteCommitted(address _voter, bytes32 _disputeId, bytes32 _commitment);
Expand Down Expand Up @@ -95,7 +99,7 @@
);
module.commitVote(_request, _dispute, _commitment);

vm.warp(140_001);
vm.warp(END_TIME + 1);

vm.mockCall(
address(token),
Expand Down Expand Up @@ -173,7 +177,7 @@
votingToken: token,
minVotesForQuorum: 1,
committingTimeWindow: 40_000,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to use a constant for the committing window as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

revealingTimeWindow: 40_000
revealingTimeWindow: REVEALING_TIME_WINDOW
})
);

Expand All @@ -183,7 +187,7 @@
bytes32 _disputeId = _getId(mockDispute);

// Store mock escalation data with startTime 100_000
module.forTest_setStartTime(_disputeId, 100_000);
module.forTest_setStartTime(_disputeId, START_TIME);

// Set timestamp for valid committingTimeWindow
vm.warp(123_456);
Expand Down Expand Up @@ -365,7 +369,7 @@
* @notice Test that `commitVote` reverts if called outside of the committing time window.
*/
function test_revertIfCommittingPhaseOver(uint256 _timestamp, bytes32 _commitment) public {
_timestamp = bound(_timestamp, 140_000, type(uint96).max);
_timestamp = bound(_timestamp, END_TIME, type(uint96).max);

// Set mock request data
mockRequest.resolutionModuleData = abi.encode(
Expand All @@ -374,7 +378,7 @@
votingToken: token,
minVotesForQuorum: 1,
committingTimeWindow: 40_000,
revealingTimeWindow: 40_000
revealingTimeWindow: REVEALING_TIME_WINDOW
})
);

Expand All @@ -384,7 +388,7 @@
bytes32 _disputeId = _getId(mockDispute);

// Store mock escalation data with startTime 100_000
module.forTest_setStartTime(_disputeId, 100_000);
module.forTest_setStartTime(_disputeId, START_TIME);

// Warp to invalid timestamp for commitment
vm.warp(_timestamp);
Expand Down Expand Up @@ -414,7 +418,7 @@
votingToken: token,
minVotesForQuorum: 1,
committingTimeWindow: 40_000,
revealingTimeWindow: 40_000
revealingTimeWindow: REVEALING_TIME_WINDOW
})
);

Expand All @@ -427,7 +431,7 @@
_mockAndExpect(address(oracle), abi.encodeCall(IOracle.disputeCreatedAt, (_disputeId)), abi.encode(1));

// Store mock escalation data with startTime 100_000
module.forTest_setStartTime(_disputeId, 100_000);
module.forTest_setStartTime(_disputeId, START_TIME);

// Store commitment
vm.prank(_voter);
Expand Down Expand Up @@ -491,7 +495,7 @@
* @notice Test that `revealVote` reverts if called outside the revealing time window.
*/
function test_revertIfInvalidPhase(uint256 _numberOfVotes, bytes32 _salt, uint256 _timestamp) public {
vm.assume(_timestamp >= 100_000 && (_timestamp <= 140_000 || _timestamp > 180_000));
vm.assume(_timestamp >= 100_000 && (_timestamp < END_TIME || _timestamp > 180_000));

// Set mock request data
mockRequest.resolutionModuleData = abi.encode(
Expand All @@ -500,7 +504,7 @@
votingToken: token,
minVotesForQuorum: 1,
committingTimeWindow: 40_000,
revealingTimeWindow: 40_000
revealingTimeWindow: REVEALING_TIME_WINDOW
})
);

Expand All @@ -511,12 +515,12 @@

_mockAndExpect(address(oracle), abi.encodeCall(IOracle.disputeCreatedAt, _disputeId), abi.encode(1));

module.forTest_setStartTime(_disputeId, 100_000);
module.forTest_setStartTime(_disputeId, START_TIME);

// Jump to timestamp
vm.warp(_timestamp);

if (_timestamp <= 140_000) {
if (_timestamp < END_TIME) {
// Check: does it revert if trying to reveal during the committing phase?
vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_OnGoingCommittingPhase.selector);
module.revealVote(mockRequest, _dispute, _numberOfVotes, _salt);
Expand Down Expand Up @@ -550,7 +554,7 @@
votingToken: token,
minVotesForQuorum: 1,
committingTimeWindow: 40_000,
revealingTimeWindow: 40_000
revealingTimeWindow: REVEALING_TIME_WINDOW
})
);

Expand All @@ -562,7 +566,7 @@
// Mock and expect IOracle.disputeCreatedAt to be called
_mockAndExpect(address(oracle), abi.encodeCall(IOracle.disputeCreatedAt, (_disputeId)), abi.encode(1));

module.forTest_setStartTime(_disputeId, 100_000);
module.forTest_setStartTime(_disputeId, START_TIME);

vm.warp(150_000);

Expand Down Expand Up @@ -601,7 +605,7 @@
votingToken: token,
minVotesForQuorum: _minVotesForQuorum,
committingTimeWindow: 40_000,
revealingTimeWindow: 40_000
revealingTimeWindow: REVEALING_TIME_WINDOW
})
);

Expand All @@ -613,7 +617,7 @@
mockDispute.responseId = _responseId;
bytes32 _disputeId = _getId(mockDispute);

module.forTest_setStartTime(_disputeId, 100_000);
module.forTest_setStartTime(_disputeId, START_TIME);

// Store escalation data with startTime 100_000 and votes 0
uint256 _votersAmount = 5;
Expand Down
Loading