Skip to content

Commit

Permalink
feat(contracts/solve): most recent request per status (#2675)
Browse files Browse the repository at this point in the history
Added `getLatestRequestByStatus(status)`, which retrieves the most
recent request to be set to a particular status. I also replaced the
`deployedAt` Arbitrum branch handling with something much more robust.

I attempted to add `getRequestUpdateHistory(id)`, which would return the
timestamp history for all status updates a request has undergone.
Despite this not altering the request and fulfillment flows, it broke
the e2e tests, so this has been excluded for now.

issue: #2679
  • Loading branch information
Zodomo authored Dec 11, 2024
1 parent 681fef7 commit 30c9c93
Show file tree
Hide file tree
Showing 13 changed files with 277 additions and 39 deletions.
35 changes: 33 additions & 2 deletions contracts/bindings/solveinbox.go

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion contracts/bindings/solveoutbox.go

Large diffs are not rendered by default.

60 changes: 30 additions & 30 deletions contracts/solve/.gas-snapshot
Original file line number Diff line number Diff line change
@@ -1,33 +1,33 @@
SolveInbox_accept_Test:test_accept_one_request() (gas: 395276)
SolveInbox_accept_Test:test_accept_reverts() (gas: 1047031)
SolveInbox_accept_Test:test_accept_skip_first() (gas: 703186)
SolveInbox_accept_Test:test_accept_two_requests() (gas: 727312)
SolveInbox_cancel_Test:test_cancel_multiToken() (gas: 548512)
SolveInbox_cancel_Test:test_cancel_nativeMultiToken() (gas: 637552)
SolveInbox_cancel_Test:test_cancel_oldest_request() (gas: 693948)
SolveInbox_cancel_Test:test_cancel_one_request() (gas: 396173)
SolveInbox_cancel_Test:test_cancel_rejected_nativeMultiToken_request() (gas: 647058)
SolveInbox_cancel_Test:test_cancel_rejected_nativeToken_request() (gas: 405192)
SolveInbox_cancel_Test:test_cancel_reverts() (gas: 1068135)
SolveInbox_cancel_Test:test_cancel_singleToken() (gas: 427717)
SolveInbox_cancel_Test:test_cancel_two_requests() (gas: 705726)
SolveInbox_claim_Test:test_claim_multiDeposit() (gas: 719118)
SolveInbox_claim_Test:test_claim_reverts() (gas: 448858)
SolveInbox_claim_Test:test_claim_singleNative() (gas: 471334)
SolveInbox_claim_Test:test_claim_singleToken() (gas: 502256)
SolveInbox_markFulfilled_Test:test_markFulfilled_reverts() (gas: 506937)
SolveInbox_markFulfilled_Test:test_markFulfilled_success() (gas: 417277)
SolveInbox_reject_Test:test_reject_nativeMultiToken() (gas: 601341)
SolveInbox_reject_Test:test_reject_oldest_request() (gas: 666353)
SolveInbox_reject_Test:test_reject_one_request() (gas: 368071)
SolveInbox_reject_Test:test_reject_reverts() (gas: 749056)
SolveInbox_reject_Test:test_reject_two_requests() (gas: 670691)
SolveInbox_request_Test:test_request_multiToken() (gas: 551270)
SolveInbox_request_Test:test_request_nativeMultiToken() (gas: 609471)
SolveInbox_request_Test:test_request_reverts() (gas: 929951)
SolveInbox_request_Test:test_request_singleNative() (gas: 369998)
SolveInbox_request_Test:test_request_singleToken() (gas: 431512)
SolveInbox_request_Test:test_request_two() (gas: 678931)
SolveInbox_accept_Test:test_accept_one_request() (gas: 449411)
SolveInbox_accept_Test:test_accept_reverts() (gas: 1135399)
SolveInbox_accept_Test:test_accept_skip_first() (gas: 757442)
SolveInbox_accept_Test:test_accept_two_requests() (gas: 781616)
SolveInbox_cancel_Test:test_cancel_multiToken() (gas: 604009)
SolveInbox_cancel_Test:test_cancel_nativeMultiToken() (gas: 694160)
SolveInbox_cancel_Test:test_cancel_oldest_request() (gas: 758515)
SolveInbox_cancel_Test:test_cancel_one_request() (gas: 450572)
SolveInbox_cancel_Test:test_cancel_rejected_nativeMultiToken_request() (gas: 738004)
SolveInbox_cancel_Test:test_cancel_rejected_nativeToken_request() (gas: 491782)
SolveInbox_cancel_Test:test_cancel_reverts() (gas: 1157387)
SolveInbox_cancel_Test:test_cancel_singleToken() (gas: 482102)
SolveInbox_cancel_Test:test_cancel_two_requests() (gas: 760517)
SolveInbox_claim_Test:test_claim_multiDeposit() (gas: 819751)
SolveInbox_claim_Test:test_claim_reverts() (gas: 537428)
SolveInbox_claim_Test:test_claim_singleNative() (gas: 569792)
SolveInbox_claim_Test:test_claim_singleToken() (gas: 600715)
SolveInbox_markFulfilled_Test:test_markFulfilled_reverts() (gas: 551134)
SolveInbox_markFulfilled_Test:test_markFulfilled_success() (gas: 493564)
SolveInbox_reject_Test:test_reject_nativeMultiToken() (gas: 657806)
SolveInbox_reject_Test:test_reject_oldest_request() (gas: 730802)
SolveInbox_reject_Test:test_reject_one_request() (gas: 422363)
SolveInbox_reject_Test:test_reject_reverts() (gas: 815736)
SolveInbox_reject_Test:test_reject_two_requests() (gas: 725273)
SolveInbox_request_Test:test_request_multiToken() (gas: 584500)
SolveInbox_request_Test:test_request_nativeMultiToken() (gas: 643788)
SolveInbox_request_Test:test_request_reverts() (gas: 1018635)
SolveInbox_request_Test:test_request_singleNative() (gas: 402136)
SolveInbox_request_Test:test_request_singleToken() (gas: 463652)
SolveInbox_request_Test:test_request_two() (gas: 711209)
SolveOutbox_fulfill_test:test_fulfillFee() (gas: 27996)
SolveOutbox_fulfill_test:test_fulfill_reverts() (gas: 673377)
SolveOutbox_fulfill_test:test_fulfill_succeeds() (gas: 274856)
1 change: 1 addition & 0 deletions contracts/solve/src/Solve.sol
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ library Solve {
* @param status Request status (open, accepted, cancelled, rejected, fulfilled, paid).
* @param call Details of the call to be executed on another chain.
* @param deposits Array of deposits backing the request.
* @param updateHistory Array of status updates including timestamps.
*/
struct Request {
bytes32 id;
Expand Down
48 changes: 45 additions & 3 deletions contracts/solve/src/SolveInbox.sol
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,23 @@ contract SolveInbox is OwnableRoles, ReentrancyGuard, Initializable, XAppBase, I
*/
mapping(bytes32 id => Solve.Request) internal _requests;

/**
* @notice Map status to latest request ID.
*/
mapping(Solve.Status => bytes32 id) internal _latestReqByStatus;

constructor() {
// Must get Arbitrum block number from ArbSys precompile, block.number returns L1 block number on Arbitrum.
// This is a temporary fix, we need a robust way of properly setting this value when on any Arbitrum chain.
if (block.chainid != 42_161 && block.chainid != 421_614) deployedAt = block.number;
else deployedAt = IArbSys(ARB_SYS).arbBlockNumber();
if (_isContract(ARB_SYS)) {
try IArbSys(ARB_SYS).arbBlockNumber() returns (uint256 arbBlockNumber) {
deployedAt = arbBlockNumber;
} catch {
deployedAt = block.number;
}
} else {
deployedAt = block.number;
}

_disableInitializers();
}

Expand All @@ -97,6 +109,13 @@ contract SolveInbox is OwnableRoles, ReentrancyGuard, Initializable, XAppBase, I
return _requests[id];
}

/**
* @notice Returns the latest request with the given status.
*/
function getLatestRequestByStatus(Solve.Status status) external view returns (Solve.Request memory) {
return _requests[_latestReqByStatus[status]];
}

/**
* @notice Open a request to execute a call on another chain, backed by deposits.
* Token deposits are transferred from msg.sender to this inbox.
Expand Down Expand Up @@ -134,6 +153,8 @@ contract SolveInbox is OwnableRoles, ReentrancyGuard, Initializable, XAppBase, I
req.status = Solve.Status.Accepted;
req.acceptedBy = msg.sender;

_latestReqByStatus[Solve.Status.Accepted] = id;

emit Accepted(id, msg.sender);
}

Expand All @@ -149,6 +170,8 @@ contract SolveInbox is OwnableRoles, ReentrancyGuard, Initializable, XAppBase, I
req.updatedAt = uint40(block.timestamp);
req.status = Solve.Status.Rejected;

_latestReqByStatus[Solve.Status.Rejected] = id;

emit Rejected(id, msg.sender, reason);
}

Expand All @@ -165,6 +188,8 @@ contract SolveInbox is OwnableRoles, ReentrancyGuard, Initializable, XAppBase, I
req.updatedAt = uint40(block.timestamp);
req.status = Solve.Status.Reverted;

_latestReqByStatus[Solve.Status.Reverted] = id;

_transferDeposits(req.from, req.deposits);

emit Reverted(id);
Expand All @@ -186,6 +211,8 @@ contract SolveInbox is OwnableRoles, ReentrancyGuard, Initializable, XAppBase, I
req.updatedAt = uint40(block.timestamp);
req.status = Solve.Status.Fulfilled;

_latestReqByStatus[Solve.Status.Fulfilled] = id;

emit Fulfilled(id, callHash, req.acceptedBy);
}

Expand All @@ -202,6 +229,8 @@ contract SolveInbox is OwnableRoles, ReentrancyGuard, Initializable, XAppBase, I
req.updatedAt = uint40(block.timestamp);
req.status = Solve.Status.Claimed;

_latestReqByStatus[Solve.Status.Claimed] = id;

_transferDeposits(to, req.deposits);

emit Claimed(id, msg.sender, to, req.deposits);
Expand Down Expand Up @@ -241,6 +270,8 @@ contract SolveInbox is OwnableRoles, ReentrancyGuard, Initializable, XAppBase, I
req.from = from;
req.call = call;

_latestReqByStatus[Solve.Status.Pending] = id;

if (msg.value > 0) {
req.deposits.push(Solve.Deposit({ isNative: true, token: address(0), amount: msg.value }));
}
Expand Down Expand Up @@ -271,4 +302,15 @@ contract SolveInbox is OwnableRoles, ReentrancyGuard, Initializable, XAppBase, I
function _callHash(bytes32 id, uint64 sourceChainId, Solve.Call storage call) internal pure returns (bytes32) {
return keccak256(abi.encode(id, sourceChainId, call));
}

/**
* @dev Returns true if the address is a contract.
*/
function _isContract(address addr) internal view returns (bool) {
uint32 size;
assembly {
size := extcodesize(addr)
}
return (size > 0);
}
}
24 changes: 21 additions & 3 deletions contracts/solve/src/SolveOutbox.sol
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,16 @@ contract SolveOutbox is OwnableRoles, ReentrancyGuard, Initializable, XAppBase {

constructor() {
// Must get Arbitrum block number from ArbSys precompile, block.number returns L1 block number on Arbitrum.
// This is a temporary fix, we need a robust way of properly setting this value when on any Arbitrum chain.
if (block.chainid != 42_161 && block.chainid != 421_614) deployedAt = block.number;
else deployedAt = IArbSys(ARB_SYS).arbBlockNumber();
if (_isContract(ARB_SYS)) {
try IArbSys(ARB_SYS).arbBlockNumber() returns (uint256 arbBlockNumber) {
deployedAt = arbBlockNumber;
} catch {
deployedAt = block.number;
}
} else {
deployedAt = block.number;
}

_disableInitializers();
}

Expand Down Expand Up @@ -185,4 +192,15 @@ contract SolveOutbox is OwnableRoles, ReentrancyGuard, Initializable, XAppBase {
function _callHash(bytes32 srcReqId, uint64 srcChainId, Solve.Call calldata call) internal pure returns (bytes32) {
return keccak256(abi.encode(srcReqId, srcChainId, call));
}

/**
* @dev Returns true if the address is a contract.
*/
function _isContract(address addr) internal view returns (bool) {
uint32 size;
assembly {
size := extcodesize(addr)
}
return (size > 0);
}
}
5 changes: 5 additions & 0 deletions contracts/solve/src/interfaces/ISolveInbox.sol
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ interface ISolveInbox {
*/
function getRequest(bytes32 id) external view returns (Solve.Request memory);

/**
* @notice Returns the latest request with the given status.
*/
function getLatestRequestByStatus(Solve.Status status) external view returns (Solve.Request memory);

/**
* @notice Open a request to execute a call on another chain, backed by deposits.
* Token deposits are transferred from msg.sender to this inbox.
Expand Down
15 changes: 15 additions & 0 deletions contracts/solve/test/Inbox_accept.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,11 @@ contract SolveInbox_accept_Test is InboxBase {

assertEq(inbox.getRequest(id).acceptedBy, solver, "inbox.getRequest(id).acceptedBy");
assertEq(uint8(inbox.getRequest(id).status), uint8(Solve.Status.Accepted), "inbox.getRequest(id).status");
assertEq(
id,
inbox.getLatestRequestByStatus(Solve.Status.Accepted).id,
"inbox.getLatestRequestByStatus(Solve.Status.Accepted)"
);
}

/// @dev Test accepting two requests
Expand All @@ -104,6 +109,11 @@ contract SolveInbox_accept_Test is InboxBase {
assertEq(inbox.getRequest(id2).acceptedBy, solver, "inbox.getRequest(id2).acceptedBy");
assertEq(uint8(inbox.getRequest(id1).status), uint8(Solve.Status.Accepted), "inbox.getRequest(id1).status");
assertEq(uint8(inbox.getRequest(id2).status), uint8(Solve.Status.Accepted), "inbox.getRequest(id2).status");
assertEq(
id2,
inbox.getLatestRequestByStatus(Solve.Status.Accepted).id,
"inbox.getLatestRequestByStatus(Solve.Status.Accepted)"
);
}

/// @dev Test accepting requests out of order
Expand All @@ -126,5 +136,10 @@ contract SolveInbox_accept_Test is InboxBase {
assertEq(inbox.getRequest(id2).acceptedBy, solver, "inbox.getRequest(id2).acceptedBy");
assertEq(uint8(inbox.getRequest(id1).status), uint8(Solve.Status.Pending), "inbox.getRequest(id1).status");
assertEq(uint8(inbox.getRequest(id2).status), uint8(Solve.Status.Accepted), "inbox.getRequest(id2).status");
assertEq(
id2,
inbox.getLatestRequestByStatus(Solve.Status.Accepted).id,
"inbox.getLatestRequestByStatus(Solve.Status.Accepted)"
);
}
}
55 changes: 55 additions & 0 deletions contracts/solve/test/Inbox_cancel.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,11 @@ contract SolveInbox_cancel_Test is InboxBase {
assertEq(address(inbox).balance, 0, "address(inbox).balance");
assertEq(address(user).balance, 1 ether, "address(user).balance");
assertEq(uint8(inbox.getRequest(id).status), uint8(Solve.Status.Reverted), "inbox.getRequest(id).status");
assertEq(
id,
inbox.getLatestRequestByStatus(Solve.Status.Reverted).id,
"inbox.getLatestRequestByStatus(Solve.Status.Reverted)"
);
}

function test_cancel_two_requests() public {
Expand All @@ -97,6 +102,11 @@ contract SolveInbox_cancel_Test is InboxBase {
assertEq(address(user).balance, 2 ether, "address(user).balance");
assertEq(uint8(inbox.getRequest(id1).status), uint8(Solve.Status.Reverted), "inbox.getRequest(id1).status");
assertEq(uint8(inbox.getRequest(id2).status), uint8(Solve.Status.Reverted), "inbox.getRequest(id2).status");
assertEq(
id2,
inbox.getLatestRequestByStatus(Solve.Status.Reverted).id,
"inbox.getLatestRequestByStatus(Solve.Status.Reverted)"
);
}

function test_cancel_oldest_request() public {
Expand All @@ -117,6 +127,16 @@ contract SolveInbox_cancel_Test is InboxBase {
assertEq(address(user).balance, 1 ether, "address(user).balance");
assertEq(uint8(inbox.getRequest(id1).status), uint8(Solve.Status.Reverted), "inbox.getRequest(id1).status");
assertEq(uint8(inbox.getRequest(id2).status), uint8(Solve.Status.Pending), "inbox.getRequest(id2).status");
assertEq(
id1,
inbox.getLatestRequestByStatus(Solve.Status.Reverted).id,
"inbox.getLatestRequestByStatus(Solve.Status.Reverted)"
);
assertEq(
id2,
inbox.getLatestRequestByStatus(Solve.Status.Pending).id,
"inbox.getLatestRequestByStatus(Solve.Status.Pending)"
);
}

function test_cancel_singleToken() public {
Expand All @@ -136,6 +156,11 @@ contract SolveInbox_cancel_Test is InboxBase {
assertEq(token1.balanceOf(address(inbox)), 0, "token1.balanceOf(inbox)");
assertEq(token1.balanceOf(user), 1 ether, "token1.balanceOf(user)");
assertEq(uint8(inbox.getRequest(id).status), uint8(Solve.Status.Reverted), "inbox.getRequest(id).status");
assertEq(
id,
inbox.getLatestRequestByStatus(Solve.Status.Reverted).id,
"inbox.getLatestRequestByStatus(Solve.Status.Reverted)"
);
}

function test_cancel_multiToken() public {
Expand All @@ -158,6 +183,11 @@ contract SolveInbox_cancel_Test is InboxBase {
assertEq(token1.balanceOf(user), 1 ether, "token1.balanceOf(user)");
assertEq(token2.balanceOf(user), 1 ether, "token2.balanceOf(user)");
assertEq(uint8(inbox.getRequest(id).status), uint8(Solve.Status.Reverted), "inbox.getRequest(id).status");
assertEq(
id,
inbox.getLatestRequestByStatus(Solve.Status.Reverted).id,
"inbox.getLatestRequestByStatus(Solve.Status.Reverted)"
);
}

function test_cancel_nativeMultiToken() public {
Expand All @@ -183,6 +213,11 @@ contract SolveInbox_cancel_Test is InboxBase {
assertEq(token1.balanceOf(user), 1 ether, "token1.balanceOf(user)");
assertEq(token2.balanceOf(user), 1 ether, "token2.balanceOf(user)");
assertEq(uint8(inbox.getRequest(id).status), uint8(Solve.Status.Reverted), "inbox.getRequest(id).status");
assertEq(
id,
inbox.getLatestRequestByStatus(Solve.Status.Reverted).id,
"inbox.getLatestRequestByStatus(Solve.Status.Reverted)"
);
}

function test_cancel_rejected_nativeToken_request() public {
Expand All @@ -204,6 +239,16 @@ contract SolveInbox_cancel_Test is InboxBase {
assertEq(address(inbox).balance, 0, "address(inbox).balance");
assertEq(address(user).balance, 1 ether, "address(user).balance");
assertEq(uint8(inbox.getRequest(id).status), uint8(Solve.Status.Reverted), "inbox.getRequest(id).status");
assertEq(
id,
inbox.getLatestRequestByStatus(Solve.Status.Rejected).id,
"inbox.getLatestRequestByStatus(Solve.Status.Rejected)"
);
assertEq(
id,
inbox.getLatestRequestByStatus(Solve.Status.Reverted).id,
"inbox.getLatestRequestByStatus(Solve.Status.Reverted)"
);
}

function test_cancel_rejected_nativeMultiToken_request() public {
Expand Down Expand Up @@ -233,5 +278,15 @@ contract SolveInbox_cancel_Test is InboxBase {
assertEq(token1.balanceOf(user), 1 ether, "token1.balanceOf(user)");
assertEq(token2.balanceOf(user), 1 ether, "token2.balanceOf(user)");
assertEq(uint8(inbox.getRequest(id).status), uint8(Solve.Status.Reverted), "inbox.getRequest(id).status");
assertEq(
id,
inbox.getLatestRequestByStatus(Solve.Status.Rejected).id,
"inbox.getLatestRequestByStatus(Solve.Status.Rejected)"
);
assertEq(
id,
inbox.getLatestRequestByStatus(Solve.Status.Reverted).id,
"inbox.getLatestRequestByStatus(Solve.Status.Reverted)"
);
}
}
Loading

0 comments on commit 30c9c93

Please sign in to comment.