feat: medusa tests #51
Conversation
| // Calculate request ID using same logic as Oracle | ||
| bytes32 requestId = keccak256(abi.encode(requestData)); |
There was a problem hiding this comment.
A getId() util would come in handy.
There was a problem hiding this comment.
prophet-core/solidity/libraries/ValidatorLib.sol is at hand.
| if (_ghost_activeResponses[requestId].length == 0) return (bytes32(0)); | ||
| bytes32 responseId = _ghost_activeResponses[requestId][0]; |
There was a problem hiding this comment.
Shouldn't _getRandomActiveResponse() be used in these situations (seen in handleDisputeResponseOracle() and handleFinalize()) so to fuzz _ghost_activeResponses?
# 🤖 Linear Closes GRT-XXX --------- Co-authored-by: drgorillamd <[email protected]> Co-authored-by: Simon Something /DrGoNoGo <[email protected]>
| // address proposer = _pickActor(_actorSeed); | ||
|
|
||
| // Build response data | ||
| IOracle.Response memory responseData = IOracle.Response(msg.sender, requestId, _response); // abi.encode(_blockNumber)? |
There was a problem hiding this comment.
Considering that the caller must coincide with the proposer, should an actor be picked as the proposer, or is using msg.sender enough? _stakeGRT() and _provisionGRT() helpers do not accept actor picking currently, so I set the proposer to msg.sender.
More generally, when should an actor be picked and/or pranked instead of relying on the fuzzed unpranked msg.sender?
| // active because finalized without answer | ||
| if ((oracle.finalizedAt(currRequestId) != 0 && oracle.finalizedResponseId(currRequestId) == 0)) { | ||
| ++activeRequests; |
There was a problem hiding this comment.
I understand that a request is active after it has been created but before it has been finalized. A finalized request, be it with or without response, shouldn't be counted as an active request.
In fact, if so happens, EBORequestCreator wouldn't permit to create same chainId/epoch requests after a previous one has finalized without response – which I think is the opposite of what is wanted. Consider the assertion of this catch block: assertTrue(isFinalizedWithResponse || activeRequests > 0).
test/invariants/FuzzTest.t.sol
Outdated
| // check that it is not pointing to the same request. If it does, check it wasn't | ||
| // finalized without answer | ||
|
|
||
| if ( | ||
| _ghost_activeResponses[currentRequestId][currentRequestResponseIdx] | ||
| != _ghost_activeResponses[otherRequestId][otherRequestResponseIdx], | ||
| 'prop 13: different response id for same request' | ||
| ); | ||
| == _ghost_activeResponses[otherRequestId][otherRequestResponseIdx] | ||
| && ( | ||
| (oracle.finalizedAt(currentRequestId) == 0 && oracle.finalizedResponseId(currentRequestId) != 0) | ||
| || (oracle.finalizedAt(otherRequestId) == 0 && oracle.finalizedResponseId(otherRequestId) != 0) |
There was a problem hiding this comment.
Why does the finalization status have to do with this property? I'd expect that requests IDs, responses IDs and disputes IDs shouldn't collide regardless of the request finalization, be it with or without response.
Besides, the condition checks for a request unfinalized (oracle.finalizedAt(requestId) == 0) and finalized with response (oracle.finalizedResponseId(requestId) != 0), which doesn't seem possible at the same time.
| if ( | ||
| _ghost_activeResponses[currentRequestId][currentRequestDisputeIdx] | ||
| == _ghost_activeResponses[otherRequestId][otherRequestDisputeIdx] |
There was a problem hiding this comment.
Shouldn't _ghost_activeResponses be replaced with _ghost_disputes here?
| try bondEscalationModule.pledgeAgainstDispute(_requestData, _disputeData) { | ||
| assertTrue(_ghost_escalatedDisputes[_disputeId], 'property 8a: pledging for a dispute not escalated'); | ||
| try bondEscalationModule.pledgeForDispute(_requestData, _disputeData) { | ||
| assertTrue( | ||
| _totalPledgesFor(_disputeId) >= _totalPledgesAgainst(_disputeId), 'property 8b: pledging for the wrong side' |
There was a problem hiding this comment.
Isn't the purpose of property 8b to call pledgeAgainstDispute() instead of pledgeForDispute()?
| IBondEscalationModule.BondEscalation memory _escalation = bondEscalationModule.getEscalation(_disputeId); | ||
|
|
||
| assertTrue( | ||
| !_ghost_escalatedDisputes[_disputeId] | ||
| || block.timestamp > oracle.disputeCreatedAt(_disputeId) + DISPUTE_DEADLINE | ||
| || _escalation.status != IBondEscalationModule.BondEscalationStatus.Active |
There was a problem hiding this comment.
Isn't the escalation status comparison redundant with _ghost_escalatedDisputes?
Add fuzzing tests (using Medusa) to the current codebase (see properties.MD and summary.MD for details)