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

address various comments from Spearbit review #32

Merged
merged 19 commits into from
Feb 28, 2024
Merged

address various comments from Spearbit review #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
d17e576
fix comment in AmountDeriver
0age Feb 27, 2024
c1dc0c9
fix comment in ConsiderationDecoder
0age Feb 27, 2024
ea11a5d
improve comments around modified params
0age Feb 27, 2024
17720b1
fix a comment
0age Feb 27, 2024
f217574
fix comment in ConsiderationEncoder
0age Feb 27, 2024
72e8e96
add a clarification about event data copying
0age Feb 27, 2024
2f4598f
clarify constant name
0age Feb 27, 2024
55d72de
clarify another constant name
0age Feb 27, 2024
e3ee057
add natspec and a small optimization to basic order status updates
0age Feb 28, 2024
bb44017
add natspec for CriteriaResolution
0age Feb 28, 2024
e736d1d
natspec and general cleanup in ZoneInteraction
0age Feb 28, 2024
da0684c
clean up OrderFulfiller and add missing natspec
0age Feb 28, 2024
5a4f4cb
progress on max line length 80
0age Feb 28, 2024
27671cd
heh actually sstore it
0age Feb 28, 2024
3cfda8d
small cleanup of OrderValidator
0age Feb 28, 2024
bc82f4a
tiny optimization and lots of formatting tweaks to decoder
0age Feb 28, 2024
d215c4c
more 80-line-limit stuff
0age Feb 28, 2024
0ea718f
80 character line limit was maybe a mistake
0age Feb 28, 2024
831a1a0
Merge branch 'main' into spearbit-tweaks
0age Feb 28, 2024
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
6 changes: 3 additions & 3 deletions src/core/lib/AmountDeriver.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ contract AmountDeriver is AmountDerivationErrors {
amount :=
mul(
iszero(iszero(totalBeforeDivision)),
// Subtract 1 from the numerator and add 1 to the result if
// roundUp is true to get the proper rounding direction.
// Subtract 1 from the numerator and add 1 to the result
// if roundUp is true to get proper rounding direction.
// Division is performed with no zero check as duration
// cannot be zero as long as startTime < endTime.
add(
Expand Down Expand Up @@ -137,7 +137,7 @@ contract AmountDeriver is AmountDerivationErrors {
// Multiply the numerator by the value and ensure no overflow occurs.
uint256 valueTimesNumerator = value * numerator;

// Divide and check for remainder. Note that denominator cannot be zero.
// Divide by the denominator (note that denominator cannot be zero).
assembly {
// Perform division without zero check.
newValue := div(valueTimesNumerator, denominator)
Expand Down
32 changes: 17 additions & 15 deletions src/core/lib/Assertions.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

import { OrderParameters } from "seaport-types/src/lib/ConsiderationStructs.sol";
import {
OrderParameters
} from "seaport-types/src/lib/ConsiderationStructs.sol";

import { GettersAndDerivers } from "./GettersAndDerivers.sol";

Expand All @@ -15,7 +17,7 @@ import {
AddressDirtyUpperBitThreshold,
BasicOrder_additionalRecipients_head_cdPtr,
BasicOrder_additionalRecipients_head_ptr,
BasicOrder_additionalRecipients_length_cdPtr,
BasicOrder_addlRecipients_length_cdPtr,
BasicOrder_basicOrderType_cdPtr,
BasicOrder_basicOrderType_range,
BasicOrder_considerationToken_cdPtr,
Expand Down Expand Up @@ -154,32 +156,32 @@ contract Assertions is
and(
and(
and(
// Order parameters at cd 0x04 must have offset of 0x20.
// Order parameters at cd 0x04 offset = 0x20.
eq(
calldataload(BasicOrder_parameters_cdPtr),
BasicOrder_parameters_ptr
),
// Additional recipients (cd 0x224) arr offset == 0x240.
// Additional recipients at cd 0x224 offset = 0x240.
eq(
calldataload(
BasicOrder_additionalRecipients_head_cdPtr
),
BasicOrder_additionalRecipients_head_ptr
)
),
// Signature offset == 0x260 + (recipients.length * 0x40).
// Signature offset = 0x260 + recipients.length * 0x40.
eq(
// Load signature offset from calldata 0x244.
calldataload(BasicOrder_signature_cdPtr),
// Expected offset is start of recipients + len * 64.
// Expected offset = start of recipients + len * 64.
add(
BasicOrder_signature_ptr,
shl(
// Each additional recipient has length of 0x40.
// Each additional recipient length = 0x40.
AdditionalRecipient_size_shift,
// Additional recipients length at cd 0x264.
calldataload(
BasicOrder_additionalRecipients_length_cdPtr
BasicOrder_addlRecipients_length_cdPtr
)
)
)
Expand All @@ -188,25 +190,25 @@ contract Assertions is
and(
// Ensure BasicOrderType parameter is less than 0x18.
lt(
// BasicOrderType parameter at calldata offset 0x124.
// BasicOrderType parameter = calldata offset 0x124.
calldataload(BasicOrder_basicOrderType_cdPtr),
// Value should be less than 24.
BasicOrder_basicOrderType_range
),
// Ensure no dirty upper bits are present on offerer, zone,
// offer token, or consideration token.
// Ensure no dirty upper bits are present on offerer,
// zone, offer token, or consideration token.
lt(
or(
or(
// Offerer parameter at calldata offset 0x84.
// Offerer parameter = calldata offset 0x84.
calldataload(BasicOrder_offerer_cdPtr),
// Zone parameter at calldata offset 0xa4.
// Zone parameter = calldata offset 0xa4.
calldataload(BasicOrder_zone_cdPtr)
),
or(
// Offer token parameter at cd offset 0xc4.
// Offer token parameter = cd offset 0xc4.
calldataload(BasicOrder_offerToken_cdPtr),
// Consideration token parameter at offset 0x24.
// Consideration parameter = offset 0x24.
calldataload(
BasicOrder_considerationToken_cdPtr
)
Expand Down
90 changes: 67 additions & 23 deletions src/core/lib/BasicOrderFulfiller.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,33 +299,57 @@ contract BasicOrderFulfiller is OrderValidator {
if (route == BasicOrderRouteType.ERC20_TO_ERC721) {
// Transfer ERC721 to caller using offerer's conduit preference.
_transferERC721(
CalldataPointer.wrap(BasicOrder_offerToken_cdPtr).readAddress(),
CalldataPointer.wrap(BasicOrder_offerer_cdPtr).readAddress(),
CalldataPointer.wrap(
BasicOrder_offerToken_cdPtr
).readAddress(),
CalldataPointer.wrap(
BasicOrder_offerer_cdPtr
).readAddress(),
msg.sender,
CalldataPointer.wrap(BasicOrder_offerIdentifier_cdPtr).readUint256(),
CalldataPointer.wrap(BasicOrder_offerAmount_cdPtr).readUint256(),
CalldataPointer.wrap(
BasicOrder_offerIdentifier_cdPtr
).readUint256(),
CalldataPointer.wrap(
BasicOrder_offerAmount_cdPtr
).readUint256(),
conduitKey,
accumulator
);
} else if (route == BasicOrderRouteType.ERC20_TO_ERC1155) {
// Transfer ERC1155 to caller with offerer's conduit preference.
_transferERC1155(
CalldataPointer.wrap(BasicOrder_offerToken_cdPtr).readAddress(),
CalldataPointer.wrap(BasicOrder_offerer_cdPtr).readAddress(),
CalldataPointer.wrap(
BasicOrder_offerToken_cdPtr
).readAddress(),
CalldataPointer.wrap(
BasicOrder_offerer_cdPtr
).readAddress(),
msg.sender,
CalldataPointer.wrap(BasicOrder_offerIdentifier_cdPtr).readUint256(),
CalldataPointer.wrap(BasicOrder_offerAmount_cdPtr).readUint256(),
CalldataPointer.wrap(
BasicOrder_offerIdentifier_cdPtr
).readUint256(),
CalldataPointer.wrap(
BasicOrder_offerAmount_cdPtr
).readUint256(),
conduitKey,
accumulator
);
} else if (route == BasicOrderRouteType.ERC721_TO_ERC20) {
// Transfer ERC721 to offerer using caller's conduit preference.
_transferERC721(
CalldataPointer.wrap(BasicOrder_considerationToken_cdPtr).readAddress(),
CalldataPointer.wrap(
BasicOrder_considerationToken_cdPtr
).readAddress(),
msg.sender,
CalldataPointer.wrap(BasicOrder_offerer_cdPtr).readAddress(),
CalldataPointer.wrap(BasicOrder_considerationIdentifier_cdPtr).readUint256(),
CalldataPointer.wrap(BasicOrder_considerationAmount_cdPtr).readUint256(),
CalldataPointer.wrap(
BasicOrder_offerer_cdPtr
).readAddress(),
CalldataPointer.wrap(
BasicOrder_considerationIdentifier_cdPtr
).readUint256(),
CalldataPointer.wrap(
BasicOrder_considerationAmount_cdPtr
).readUint256(),
conduitKey,
accumulator
);
Expand All @@ -334,11 +358,19 @@ contract BasicOrderFulfiller is OrderValidator {

// Transfer ERC1155 to offerer with caller's conduit preference.
_transferERC1155(
CalldataPointer.wrap(BasicOrder_considerationToken_cdPtr).readAddress(),
CalldataPointer.wrap(
BasicOrder_considerationToken_cdPtr
).readAddress(),
msg.sender,
CalldataPointer.wrap(BasicOrder_offerer_cdPtr).readAddress(),
CalldataPointer.wrap(BasicOrder_considerationIdentifier_cdPtr).readUint256(),
CalldataPointer.wrap(BasicOrder_considerationAmount_cdPtr).readUint256(),
CalldataPointer.wrap(
BasicOrder_offerer_cdPtr
).readAddress(),
CalldataPointer.wrap(
BasicOrder_considerationIdentifier_cdPtr
).readUint256(),
CalldataPointer.wrap(
BasicOrder_considerationAmount_cdPtr
).readUint256(),
conduitKey,
accumulator
);
Expand All @@ -354,7 +386,11 @@ contract BasicOrderFulfiller is OrderValidator {
}

// Determine whether order is restricted and, if so, that it is valid.
_assertRestrictedBasicOrderValidity(orderHash, orderType, callDataPointer);
_assertRestrictedBasicOrderValidity(
orderHash,
orderType,
callDataPointer
);

// Clear the reentrancy guard.
_clearReentrancyGuard();
Expand Down Expand Up @@ -594,8 +630,9 @@ contract BasicOrderFulfiller is OrderValidator {

// Read length of the additionalRecipients array from calldata
// and iterate.
totalAdditionalRecipients :=
calldataload(BasicOrder_totalOriginalAdditionalRecipients_cdPtr)
totalAdditionalRecipients := calldataload(
BasicOrder_totalOriginalAdditionalRecipients_cdPtr
)
let i := 0
for { } lt(i, totalAdditionalRecipients) { i := add(i, 1) } {
/*
Expand Down Expand Up @@ -963,7 +1000,9 @@ contract BasicOrderFulfiller is OrderValidator {
OrderFulfilled_baseOffset,
shl(
OneWordShift,
calldataload(BasicOrder_additionalRecipients_length_cdPtr)
calldataload(
BasicOrder_additionalRecipients_length_cdPtr
)
)
)

Expand Down Expand Up @@ -992,7 +1031,9 @@ contract BasicOrderFulfiller is OrderValidator {
add(
OrderFulfilled_baseSize,
mul(
calldataload(BasicOrder_additionalRecipients_length_cdPtr),
calldataload(
BasicOrder_additionalRecipients_length_cdPtr
),
ReceivedItem_size
)
)
Expand All @@ -1019,7 +1060,7 @@ contract BasicOrderFulfiller is OrderValidator {
mstore(FreeMemoryPointerSlot, add(eventDataPtr, dataSize))
}

// Verify and update the status of the derived order.
// Verify the status of the derived order.
OrderStatus storage orderStatus = _validateBasicOrder(
orderHash,
_toBytesReturnType(_decodeBytes)(
Expand All @@ -1038,7 +1079,10 @@ contract BasicOrderFulfiller is OrderValidator {
);

// Determine whether order is restricted and, if so, that it is valid.
callDataPointer = _assertRestrictedBasicOrderAuthorization(orderHash, orderType);
callDataPointer = _assertRestrictedBasicOrderAuthorization(
orderHash,
orderType
);

// Update the status of the order and mark as fully filled.
_updateBasicOrderStatus(orderStatus);
Expand Down
34 changes: 20 additions & 14 deletions src/core/lib/Consideration.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,13 @@ contract Consideration is ConsiderationInterface, OrderCombiner {
* to the documentation for a more comprehensive summary of how to
* utilize this method and what orders are compatible with it.
*
* @custom:param parameters Additional information on the fulfilled order. Note
* that the offerer and the fulfiller must first approve
* this contract (or their chosen conduit if indicated)
* before any tokens can be transferred. Also note that
* contract recipients of ERC1155 consideration items must
* implement `onERC1155Received` to receive those items.
* @custom:param parameters Additional information on the fulfilled order.
* Note that the offerer and the fulfiller must
* first approve this contract (or their chosen
* conduit if indicated) before any tokens can be
* transferred. Also note that contract recipients
* of ERC1155 consideration items must implement
* `onERC1155Received` to receive those items.
*
* @return fulfilled A boolean indicating whether the order has been
* successfully fulfilled.
Expand Down Expand Up @@ -131,12 +132,13 @@ contract Consideration is ConsiderationInterface, OrderCombiner {
* the zero bytes in the function selector (0x00000000) which also
* results in earlier function dispatch.
*
* @custom:param parameters Additional information on the fulfilled order. Note
* that the offerer and the fulfiller must first approve
* this contract (or their chosen conduit if indicated)
* before any tokens can be transferred. Also note that
* contract recipients of ERC1155 consideration items must
* implement `onERC1155Received` to receive those items.
* @custom:param parameters Additional information on the fulfilled order.
* Note that the offerer and the fulfiller must
* first approve this contract (or their chosen
* conduit if indicated) before any tokens can be
* transferred. Also note that contract recipients
* of ERC1155 consideration items must implement
* `onERC1155Received` to receive those items.
*
* @return fulfilled A boolean indicating whether the order has been
* successfully fulfilled.
Expand Down Expand Up @@ -636,10 +638,14 @@ contract Consideration is ConsiderationInterface, OrderCombiner {
CalldataStart.pptr()
),
_toCriteriaResolversReturnType(_decodeCriteriaResolvers)(
CalldataStart.pptrOffset(Offset_matchAdvancedOrders_criteriaResolvers)
CalldataStart.pptrOffset(
Offset_matchAdvancedOrders_criteriaResolvers
)
),
_toFulfillmentsReturnType(_decodeFulfillments)(
CalldataStart.pptrOffset(Offset_matchAdvancedOrders_fulfillments)
CalldataStart.pptrOffset(
Offset_matchAdvancedOrders_fulfillments
)
),
_substituteCallerForEmptyRecipient(recipient)
);
Expand Down
Loading
Loading