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

More custom errors instead of revert strings #179

Merged
merged 1 commit into from
Jan 11, 2024
Merged

More custom errors instead of revert strings #179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
116 changes: 77 additions & 39 deletions contracts/L2ForkArbitrator.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,33 @@ we are in the 1 week period before a fork) the new one will be queued.
// NB This doesn't implement IArbitrator because that requires slightly more functions than we need
// TODO: Would be good to make a stripped-down IArbitrator that only has the essential functions
contract L2ForkArbitrator is IBridgeMessageReceiver {
// @dev Error thrown when the arbitration fee is 0
error ArbitrationFeeMustBePositive();
// @dev Error thrown when the arbitration request has already been made
error ArbitrationAlreadyRequested();
// @dev Error thrown when the fork is already in progress over something else
error ForkInProgress();
// @dev Error thrown when the L2 bridge is not set
error L2BridgeNotSet();
// @dev Error thrown when the fork is not in progress
error QuestionNotForked();
// @dev Error thrown when status is not FORK_REQUESTED
error StatusNotForkRequested();
// @dev Error thrown when status is not FORK_REQUEST_FAILED
error StatusNotForkRequestFailed();
// @dev Error thrown when the fork is not in progress
error WrongStatus();
// @dev Error thrown when called with wrong network
error WrongNetwork();
// @dev Error thrown when called with wrong sender
error WrongSender();
// @dev Error thrown when called from the wrong bridge
error WrongBridge();
// @dev Error thrown when the fork is not in progress
error ForkNotInProgress();
// @dev Error thrown when contract is not awaiting activation
error NotAwaitingActivation();

bool public isForkInProgress;
IRealityETH public realitio;

Expand Down Expand Up @@ -83,12 +110,12 @@ contract L2ForkArbitrator is IBridgeMessageReceiver {
uint256 maxPrevious
) external payable returns (bool) {
uint256 arbitration_fee = getDisputeFee(questionId);
require(arbitration_fee > 0, "fee must be positive");

require(
arbitrationRequests[questionId].status == RequestStatus.NONE,
"Already requested"
);
if (arbitration_fee == 0) {
revert ArbitrationFeeMustBePositive();
}
if (arbitrationRequests[questionId].status != RequestStatus.NONE) {
revert ArbitrationAlreadyRequested();
}

arbitrationRequests[questionId] = ArbitrationRequest(
RequestStatus.QUEUED,
Expand All @@ -114,22 +141,29 @@ contract L2ForkArbitrator is IBridgeMessageReceiver {
/// @dev Talks to the L1 ForkingManager asynchronously, and may fail.
/// @param question_id The question in question
function requestActivateFork(bytes32 question_id) public {
require(!isForkInProgress, "Already forking"); // Forking over something else
if (isForkInProgress) {
revert ForkInProgress(); // Forking over something else
}

RequestStatus status = arbitrationRequests[question_id].status;
require(
status == RequestStatus.QUEUED ||
edmundedgar marked this conversation as resolved.
Show resolved Hide resolved
status == RequestStatus.FORK_REQUEST_FAILED,
"not awaiting activation"
);
if (
status != RequestStatus.QUEUED &&
status != RequestStatus.FORK_REQUEST_FAILED
) {
revert NotAwaitingActivation();
}
arbitrationRequests[question_id].status = RequestStatus.FORK_REQUESTED;

uint256 forkFee = chainInfo.getForkFee();
uint256 paid = arbitrationRequests[question_id].paid;
require(paid >= forkFee, "fee paid too low");
if (paid < forkFee) {
revert ArbitrationFeeMustBePositive();
}

address l2Bridge = chainInfo.l2Bridge();
require(l2Bridge != address(0), "l2Bridge not set");
if (l2Bridge == address(0)) {
revert L2BridgeNotSet();
}

IPolygonZkEVMBridge bridge = IPolygonZkEVMBridge(l2Bridge);

Expand Down Expand Up @@ -167,20 +201,24 @@ contract L2ForkArbitrator is IBridgeMessageReceiver {
bytes memory _data
) external payable {
address l2Bridge = chainInfo.l2Bridge();
require(msg.sender == l2Bridge, "Not our bridge");
require(_originNetwork == uint32(0), "Wrong network, WTF");
require(
_originAddress == address(l1GlobalForkRequester),
"Unexpected sender"
);
if (msg.sender != l2Bridge) {
revert WrongBridge();
}
if (_originNetwork != uint32(0)) {
revert WrongNetwork();
}
if (_originAddress != address(l1GlobalForkRequester)) {
revert WrongSender();
}

bytes32 question_id = bytes32(_data);
RequestStatus status = arbitrationRequests[question_id].status;
require(
status == RequestStatus.FORK_REQUESTED,
"not in fork-requested state"
);
require(isForkInProgress, "No fork in progress to clear");
if (status != RequestStatus.FORK_REQUESTED) {
revert WrongStatus();
}
if (!isForkInProgress) {
revert ForkNotInProgress();
}

isForkInProgress = false;
arbitrationRequests[question_id].status = RequestStatus
Expand All @@ -202,15 +240,14 @@ contract L2ForkArbitrator is IBridgeMessageReceiver {
) external {
// Read from directory what the result was
RequestStatus status = arbitrationRequests[question_id].status;
require(
status == RequestStatus.FORK_REQUESTED,
"not in fork-requested state"
);

require(
chainInfo.questionToChainID(false, address(this), question_id) > 0,
"Dispute not found in ChainInfo"
);
if (status != RequestStatus.FORK_REQUESTED) {
revert StatusNotForkRequested();
}
if (
chainInfo.questionToChainID(false, address(this), question_id) == 0
) {
revert QuestionNotForked();
}

// We get the fork result from the L2ChainInfo contract
bytes32 answer = chainInfo.forkQuestionResults(
Expand Down Expand Up @@ -239,13 +276,14 @@ contract L2ForkArbitrator is IBridgeMessageReceiver {
/// @param question_id The question in question
function cancelArbitration(bytes32 question_id) external {
// For simplicity we won't let you cancel until forking is sorted, as you might retry and keep failing for the same reason
require(!isForkInProgress, "Fork in progress");
if (isForkInProgress) {
revert ForkInProgress();
}

RequestStatus status = arbitrationRequests[question_id].status;
require(
status == RequestStatus.FORK_REQUEST_FAILED,
"Not in fork-failed state"
);
if (status != RequestStatus.FORK_REQUEST_FAILED) {
revert StatusNotForkRequestFailed();
}

address payable payer = arbitrationRequests[question_id].payer;
realitio.cancelArbitration(question_id);
Expand Down
12 changes: 10 additions & 2 deletions contracts/mixin/InitializeChain.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,24 @@ pragma solidity ^0.8.20;
This contract uses the fact that after a fork the chainId changes and thereby detects forks*/

contract InitializeChain {
/// @dev Error thrown when contract is expected to be called on a new fork
error NotOnNewFork();
/// @dev Error thrown when contract is expected not to be called on a new fork
error OnNewFork();
uint256 public chainId;

modifier onlyChainUninitialized() {
require(chainId != block.chainid, "Not on new fork");
if (chainId == block.chainid) {
revert NotOnNewFork();
}
_;
chainId = block.chainid;
}

modifier onlyChainInitialized() {
require(chainId == block.chainid, "On new fork");
if (chainId != block.chainid) {
revert OnNewFork();
}
_;
}

Expand Down
17 changes: 5 additions & 12 deletions test/InitializeChain.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ pragma solidity ^0.8.17;

import {Test} from "forge-std/Test.sol";
import {InitializeChainWrapper} from "./testcontract/InitializeChainWrapper.sol";
import {InitializeChain} from "../contracts/mixin/InitializeChain.sol";

contract InitializeChainWrapperTest is Test {
InitializeChainWrapper public wrapper;
Expand All @@ -21,13 +22,8 @@ contract InitializeChainWrapperTest is Test {
wrapper.onlyChainUninitializedWrapper(); // This should pass since it's the first TX after fork

// Subsequent calls should fail
try wrapper.onlyChainUninitializedWrapper() {
fail(
"Should have reverted because it's not the first TX after fork."
);
} catch Error(string memory reason) {
assertEq(reason, "Not on new fork");
}
vm.expectRevert(InitializeChain.NotOnNewFork.selector);
wrapper.onlyChainUninitializedWrapper();
}

function testonlyChainInitialized() public {
Expand All @@ -38,11 +34,8 @@ contract InitializeChainWrapperTest is Test {
vm.chainId(2);

// This call should fail since it's the first TX after fork
try wrapper.onlyChainInitializedWrapper() {
fail("Should have reverted because it's the first TX after fork.");
} catch Error(string memory reason) {
assertEq(reason, "On new fork");
}
vm.expectRevert(InitializeChain.OnNewFork.selector);
wrapper.onlyChainInitializedWrapper();

// Call a function with onlyChainUninitialized to update chainId
wrapper.onlyChainUninitializedWrapper();
Expand Down