Seaport 1.1 #492
Seaport 1.1 #492
Conversation
Update CONTRIBUTORS.md
Revert on dirty bits (unused item parameters)
gas: add unchecked for ConduitController#updateChannel
Added ENS name of JasperAlexander
Assign basicOrderType to a variable rather than loading twice
Gas opt: skip `remaining` calculation when not necessary
implement nits from readthrough
Ensure revert on non-zero token / identifier (native) and identifier (erc20) in transferred item params
Add Foundry test for failures when spending pre-approved items from null address with invalid signature
…-reverts Test that "fulfill" methods revert on Orders with Native offer items
…-pass run one final comment pass over the token transferrer
include 1.1 contributors list in Seaport natspec
| if (item.itemType == ConduitItemType.ERC20) { | ||
| // Transfer ERC20 token. | ||
| // Transfer ERC20 token. Note that item.identifier is ignored and |
There was a problem hiding this comment.
Is there a particular reason for this?
There was a problem hiding this comment.
Avoiding a redundant check mainly, basically letting channels make the call as to whether they want to do this safety check or not
There was a problem hiding this comment.
Avoiding a redundant check mainly,
Where is this checked before hand? The conduit can be used independently, can't it be?
| @@ -198,8 +213,13 @@ contract ConduitController is ConduitControllerInterface { | |||
| revert NewPotentialOwnerIsZeroAddress(conduit); | |||
| } | |||
|
|
|||
| // Ensure the new potential owner is not already set. | |||
| if (newPotentialOwner == _conduits[conduit].potentialOwner) { | |||
There was a problem hiding this comment.
This only checks for the same owner being proposed twice, but allows proposing a new one while a proposal is active. Is that the goal?
There was a problem hiding this comment.
I think that’s fine, the current owner is still in control at that stage
There was a problem hiding this comment.
It does feel like a not very useful check though.
| if (item.itemType == ConduitItemType.NATIVE) { | ||
| revert InvalidItemType(); | ||
| } else if (item.itemType == ConduitItemType.ERC20) { | ||
| _performERC20Transfer( |
There was a problem hiding this comment.
This allows item.identifier to be anything.
| invalidNativeOfferItemErrorBuffer := shl( | ||
| 1, | ||
| gt( | ||
| // Take the remainder of the selector modulo a magic value. |
There was a problem hiding this comment.
👀
Which (two) selectors is this comparing? This seems like an extremely fragile piece of code should new external code paths allowed to reach this function.
For the reader: the shr is reading the selector (top 32 bits) and MagicModulus = 69, MagicRemainder = 0x1d.
There was a problem hiding this comment.
It seems to be reachable from Consideration.matchOrders, Consideration.matchAdvancedOrders, Consideration.fulfillAvailableOrders, Consideration.fulfillAvailableAdvancedOrders.
That is 4 selectors this code is comparing. Note that not only new external functions are risky here, but also any parameter change to the above would change the selectors.
There was a problem hiding this comment.
Found a prior conversation on the topic: https://github.com/ProjectOpenSea/seaport/pull/464/files#r893417290
There was a problem hiding this comment.
My recommendation would be to pass down a isMatchOrder flag.
There was a problem hiding this comment.
Yeah we’ll definitely replace this pattern with one like you suggest should any aspect of the interface change
| assembly { | ||
| // Use the second bit of the error buffer to indicate whether the | ||
| // current function is not matchAdvancedOrders or matchOrders. | ||
| invalidNativeOfferItemErrorBuffer := shl( |
There was a problem hiding this comment.
Likely no need for assembly on this and it clearly shows how risky it is to the reader:
unchecked {
invalidNativeOfferItemErrorBuffer = ((uint32(msg.selector) % NonMatchSelector_MagicModulus) > NonMatchSelector_MagicRemainder) ? 2 : 0;
}Ok, for this need to have the optimiser support branchless ternary ops.
There was a problem hiding this comment.
I do like including this as part of the documentation to demonstrate what’s happening here!
| @@ -184,7 +193,7 @@ contract ConsiderationBase is ConsiderationEventsAndErrors { | |||
| "bytes32 zoneHash,", | |||
| "uint256 salt,", | |||
| "bytes32 conduitKey,", | |||
| "uint256 nonce", | |||
| "uint256 counter", | |||
There was a problem hiding this comment.
Documentation still contains old name (nonce instead of coutner)
This PR highlights changes from Seaport 1.0 to Seaport 1.1 and is meant to facilitate review and discussion of those changes.