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: update fill logic #55

Merged
merged 1 commit into from
Jul 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 15 additions & 10 deletions .gas-snapshot
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
OrdersTest:test_initiate_ERC20() (gas: 81255)
OrdersTest:test_initiate_ETH() (gas: 44801)
OrdersTest:test_initiate_both() (gas: 118573)
OrdersTest:test_initiate_multiERC20() (gas: 688314)
OrdersTest:test_initiate_multiETH() (gas: 75288)
OrdersTest:test_initiate_underflowETH() (gas: 63455)
OrdersTest:test_onlyBuilder() (gas: 12793)
OrdersTest:test_orderExpired() (gas: 27993)
OrdersTest:test_sweep_ERC20() (gas: 60250)
OrdersTest:test_sweep_ETH() (gas: 81788)
OrdersTest:test_fill_ERC20() (gas: 70364)
OrdersTest:test_fill_ETH() (gas: 68414)
OrdersTest:test_fill_both() (gas: 166580)
OrdersTest:test_fill_multiETH() (gas: 131926)
OrdersTest:test_fill_underflowETH() (gas: 115281)
OrdersTest:test_initiate_ERC20() (gas: 81435)
OrdersTest:test_initiate_ETH() (gas: 44949)
OrdersTest:test_initiate_both() (gas: 118677)
OrdersTest:test_initiate_multiERC20() (gas: 688417)
OrdersTest:test_initiate_multiETH() (gas: 75304)
OrdersTest:test_onlyBuilder() (gas: 12815)
OrdersTest:test_orderExpired() (gas: 27956)
OrdersTest:test_sweepERC20() (gas: 60402)
OrdersTest:test_sweepETH() (gas: 81940)
OrdersTest:test_underflowETH() (gas: 63528)
PassageTest:test_configureEnter() (gas: 82311)
PassageTest:test_disallowedEnter() (gas: 17916)
PassageTest:test_enter() (gas: 25563)
Expand Down
50 changes: 26 additions & 24 deletions src/Orders.sol
Original file line number Diff line number Diff line change
Expand Up @@ -22,36 +22,37 @@ struct Output {
uint256 amount;
/// @dev The address to receive the output tokens
address recipient;
/// @dev The destination chain for this output
/// @dev When emitted on the origin chain, the destination chain for the Output.
/// When emitted on the destination chain, the origin chain for the Order containing the Output.
uint32 chainId;
}

/// @notice Contract capable of processing fulfillment of intent-based Orders.
abstract contract OrderDestination {
/// @notice Emitted when an Order's Output is sent to the recipient.
/// @dev There may be multiple Outputs per Order.
/// @param originChainId - The chainId on which the Order was initiated.
/// @param recipient - The recipient of the token.
/// @param token - The address of the token transferred to the recipient. address(0) corresponds to native Ether.
/// @param amount - The amount of the token transferred to the recipient.
event OutputFilled(uint256 indexed originChainId, address indexed recipient, address indexed token, uint256 amount);

/// @notice Send the Output(s) of an Order to fulfill it.
/// The user calls `initiate` on a rollup; the Builder calls `fill` on the target chain for each Output.
/// @custom:emits OutputFilled
/// @param originChainId - The chainId on which the Order was initiated.
/// @param recipient - The recipient of the token.
/// @param token - The address of the token to be transferred to the recipient.
/// address(0) corresponds to native Ether.
/// @param amount - The amount of the token to be transferred to the recipient.
function fill(uint256 originChainId, address recipient, address token, uint256 amount) external payable {
if (token == address(0)) {
require(amount == msg.value);
payable(recipient).transfer(msg.value);
} else {
IERC20(token).transferFrom(msg.sender, recipient, amount);
/// @notice Emitted when Order Outputs are sent to their recipients.
/// @dev NOTE that here, Output.chainId denotes the *origin* chainId.
event Filled(Output[] outputs);

/// @notice Send the Output(s) of any number of Orders.
/// The user calls `initiate` on a rollup; the Builder calls `fill` on the target chain aggregating Outputs.
/// Builder may aggregate multiple Outputs with the same (`chainId`, `recipient`, `token`) into a single Output with the summed `amount`.
/// @dev NOTE that here, Output.chainId denotes the *origin* chainId.
/// @param outputs - The Outputs to be transferred.
/// @custom:emits Filled
Copy link
Contributor

Choose a reason for hiding this comment

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

should note the caller is allowed to collapse outputs if the chain Id, recipient, and token are the same (by adding the value together)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

function fill(Output[] memory outputs) external payable {
// transfer outputs
uint256 value = msg.value;
for (uint256 i; i < outputs.length; i++) {
if (outputs[i].token == address(0)) {
// this line should underflow if there's an attempt to spend more ETH than is attached to the transaction
value -= outputs[i].amount;
Copy link
Contributor

Choose a reason for hiding this comment

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

opt: future versions could crawl the list and collapse transfers to the same account, instead of having 1 transfer per output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, the Solidity for this is a bit gnarly though :) the Builder should aggregate the Outputs offchain to save on gas.

Copy link
Contributor

Choose a reason for hiding this comment

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

builder can't aggregate outputs with different chain ids tho

payable(outputs[i].recipient).transfer(outputs[i].amount);
} else {
IERC20(outputs[i].token).transferFrom(msg.sender, outputs[i].recipient, outputs[i].amount);
}
}
emit OutputFilled(originChainId, recipient, token, amount);
// emit
emit Filled(outputs);
}
}

Expand All @@ -64,6 +65,7 @@ abstract contract OrderOrigin {
error OnlyBuilder();

/// @notice Emitted when an Order is submitted for fulfillment.
/// @dev NOTE that here, Output.chainId denotes the *destination* chainId.
event Order(uint256 deadline, Input[] inputs, Output[] outputs);

/// @notice Emitted when tokens or native Ether is swept from the contract.
Expand Down
69 changes: 64 additions & 5 deletions test/Orders.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ contract OrdersTest is Test {
uint256 amount = 200;
uint256 deadline = block.timestamp;

event OutputFilled(uint256 indexed originChainId, address indexed recipient, address indexed token, uint256 amount);
event Filled(Output[] outputs);

event Order(uint256 deadline, Input[] inputs, Output[] outputs);

Expand Down Expand Up @@ -122,13 +122,13 @@ contract OrdersTest is Test {
assertEq(address(target).balance, amount * 3);
}

function test_initiate_underflowETH() public {
function test_underflowETH() public {
// change first input to ETH
inputs[0].token = address(0);
// add second ETH input
inputs.push(Input(address(0), 1));

// total ETH inputs should be `amount` + 1; function should underflow only sending `amount`
// total ETH inputs should be amount + 1; function should underflow only sending amount
vm.expectRevert();
target.initiate{value: amount}(deadline, inputs, outputs);
}
Expand All @@ -140,7 +140,7 @@ contract OrdersTest is Test {
target.initiate(deadline, inputs, outputs);
}

function test_sweep_ETH() public {
function test_sweepETH() public {
// set self as Builder
vm.coinbase(address(this));

Expand All @@ -158,7 +158,7 @@ contract OrdersTest is Test {
assertEq(recipient.balance, amount);
}

function test_sweep_ERC20() public {
function test_sweepERC20() public {
// set self as Builder
vm.coinbase(address(this));

Expand All @@ -176,4 +176,63 @@ contract OrdersTest is Test {
vm.expectRevert(OrderOrigin.OnlyBuilder.selector);
target.sweep(recipient, token);
}

function test_fill_ETH() public {
outputs[0].token = address(0);

vm.expectEmit();
emit Filled(outputs);
target.fill{value: amount}(outputs);

// ETH is transferred to recipient
assertEq(recipient.balance, amount);
}

function test_fill_ERC20() public {
vm.expectEmit();
emit Filled(outputs);
vm.expectCall(token, abi.encodeWithSelector(ERC20.transferFrom.selector, address(this), recipient, amount));
target.fill(outputs);
}

function test_fill_both() public {
// add ETH output
outputs.push(Output(address(0), amount * 2, recipient, chainId));

// expect Outputs are filled, ERC20 is transferred
vm.expectEmit();
emit Filled(outputs);
vm.expectCall(token, abi.encodeWithSelector(ERC20.transferFrom.selector, address(this), recipient, amount));
target.fill{value: amount * 2}(outputs);

// ETH is transferred to recipient
assertEq(recipient.balance, amount * 2);
}

// fill multiple ETH outputs
function test_fill_multiETH() public {
// change first output to ETH
outputs[0].token = address(0);
// add second ETH oputput
outputs.push(Output(address(0), amount * 2, recipient, chainId));

// expect Order event is initiated
vm.expectEmit();
emit Filled(outputs);
target.fill{value: amount * 3}(outputs);

// ETH is transferred to recipient
assertEq(recipient.balance, amount * 3);
}

function test_fill_underflowETH() public {
// change first output to ETH
outputs[0].token = address(0);
// add second ETH output
outputs.push(Output(address(0), 1, recipient, chainId));

// total ETH outputs should be `amount` + 1; function should underflow only sending `amount`
vm.expectRevert();
target.fill{value: amount}(outputs);
}
}