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.

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: checks createAt #37

Merged
merged 11 commits into from
Aug 6, 2024
Merged

feat: checks createAt #37

merged 11 commits into from
Aug 6, 2024

Conversation

ashitakah
Copy link
Contributor

🤖 Linear

Closes GRT-18

Copy link

linear bot commented Jul 26, 2024

GRT-18 TOB-WOND-13

0xShaito
0xShaito previously approved these changes Jul 26, 2024
Copy link
Member

@0xJabberwock 0xJabberwock left a comment

Choose a reason for hiding this comment

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

The existential validation of requests seems good to me. However, you should check as well whether the response exists in all related functions but proposeResponse(); similarly, the existence of the dispute should be validated in all but disputeResponse().

solidity/contracts/Oracle.sol Outdated Show resolved Hide resolved
solidity/contracts/Oracle.sol Outdated Show resolved Hide resolved
solidity/test/unit/Oracle.t.sol Outdated Show resolved Hide resolved
solidity/test/unit/Oracle.t.sol Outdated Show resolved Hide resolved
solidity/test/unit/Oracle.t.sol Outdated Show resolved Hide resolved
solidity/test/unit/Oracle.t.sol Outdated Show resolved Hide resolved
solidity/test/unit/Oracle.t.sol Outdated Show resolved Hide resolved
solidity/scripts/Deploy.sol Outdated Show resolved Hide resolved
solidity/test/unit/Oracle.t.sol Outdated Show resolved Hide resolved
Comment on lines 929 to 931
function test_finalize_revertsIfInvalidRequest() public {
oracle.mock_setRequestCreatedAt(_getId(mockRequest), 0);
vm.expectRevert(IOracle.Oracle_InvalidRequestBody.selector);
vm.expectRevert(IOracle.Oracle_InvalidResponseBody.selector);
Copy link
Member

Choose a reason for hiding this comment

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

The test case name and the expected reversion don't seem to match.

solidity/test/unit/Oracle.t.sol Outdated Show resolved Hide resolved
solidity/contracts/Oracle.sol Outdated Show resolved Hide resolved
solidity/contracts/Oracle.sol Outdated Show resolved Hide resolved
Comment on lines 189 to 194
contract ValidatorValidateResponseAndDispute is BaseTest {
function test_validateResponseAndDispute() public {
(bytes32 responseId, bytes32 disputeId) =
validator.validateResponseAndDispute(mockRequest, mockResponse, mockDispute);
assertEq(responseId, keccak256(abi.encode(mockResponse)));
assertEq(disputeId, keccak256(abi.encode(mockDispute)));
Copy link
Member

Choose a reason for hiding this comment

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

All the test cases of the above kind should be kept within Validator.t.sol, as we're still returning IDs in the validation functions.

Actually, it would make sense to duplicate all the test cases moved to ValidatorLib.t.sol in order to make sure that the library is properly being called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it for not duplicate test logic in the contracts, even so, i can do it again all those tests.

solidity/test/unit/Validator.t.sol Outdated Show resolved Hide resolved
solidity/test/mocks/contracts/MockRequestModule.sol Outdated Show resolved Hide resolved
solidity/contracts/Validator.sol Outdated Show resolved Hide resolved
solidity/contracts/Oracle.sol Outdated Show resolved Hide resolved
Copy link
Member

@0xJabberwock 0xJabberwock left a comment

Choose a reason for hiding this comment

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

🔥

@0xShaito 0xShaito merged commit 438de1c into dev Aug 6, 2024
3 checks passed
@0xShaito 0xShaito deleted the feat/created-at-checks branch August 6, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants