SIP-7: Zone Interface for Server-Signed Orders

[ryanio]

Please use this thread to discuss SIP-7: Interface for Server-Signed Orders

[frankisawesome]

#15 cross posting this here @ryanio good to get your thoughts too :)

[frankisawesome]

We've implemented a reference implementation of SIP-7 with substandard 3 and 4. We are in the process of getting an audit for this contract but before that we'd like to get some thoughts from you too @ryanio . I think this could also serve as a better reference contract than the currently linked one on the SIP - main discrepancy is the current one does not support SIP-6 (in SIP-7 it's listed as a prerequisite) and we like the reference contracts in seaport that do not use assembly.

https://github.com/immutable/seaport-1.4/blob/main/contracts/zones/ImmutableSignedZone.sol

[frankisawesome]

Another thought after we've gone through the SIP7 more times
It's unclear to the reader that because SIP6 is required, the first byte in extraData needs to be the version byte, and it doesn't help that the table detailing extraData fields start counting from byte 0 when the fulfiller field should start from byte 1. I'm suggesting we should update the table and include the SIP6 byte as the first byte in extraData and shift everything by one byte, and also mention the minimum byte length for extraData should be 93, not 92.

We've also made more changes on our reference contract and are still looking for feedback :)
https://github.com/immutable/immutable-seaport/blob/main/contracts/zones/ImmutableSignedZone.sol

cc @0age / @ryanio 🙇

[ryanio]

thanks for the detailed writeup frank!

This is the current extraData layout on our Signed Zone (SIP-7):

extraData: 0x0001d01398A24E32E62242Db8b6091718084425e010000000063f7b7f072948a57b877e26a188f62e25ebb9173e70704208cde92b724294562b794541aabec0797cae63ca60d41a7d2dc99a8fb5a4c339fa5e9fa4ac404a964cb8edcdf000000000000000000000000000000000000000000000000000000000000000000

0x00 -- SIP6Version
01d01398A24E32E62242Db8b6091718084425e01 -- fulfiller
0000000063f7b7f0 -- expiration
72948a57b877e26a188f62e25ebb9173e70704208cde92b724294562b794541aabec0797cae63ca60d41a7d2dc99a8fb5a4c339fa5e9fa4ac404a964cb8edcdf -signature
00 -- SIP7Version
0000000000000000000000000000000000000000000000000000000000000000 -- Token Id

so yes we are starting with the SIP-6 version byte.

While I do think it would be helpful to add it to the table and shift everything by one byte, if you use a different SIP-6 substandard version then the positioning of the layout would change again. SIPs aside from 6 should not make any hard assumptions about the precise location / contents of “context” as a whole. If it would be helpful to add this info to the SIP-7 document to make it more clear for implementers, would be great if you wanted to open a PR!

adding a few sentences about the "SIP7Version" aka substandard id byte here: #16

[frankisawesome]

This is helpful as we've gotten confused whether SIP7 version is required. With this what happens if the context is using two substandards at the same time?

[ryanio]

hm i'm not sure if we thought about supporting multiple substandards at the same time, what's an example of you wanting or needing to do that?

[frankisawesome]

@ryanio The example is our use case for substandard 3 and 4 - we use 3 for arbitrary tip (fee) enforcement and 4 for batch/bulk orders with the ability to fail all if required orders are not all filled.

[frankisawesome]

Right now in our implementation we've just opted not to use a SIP7 version byte in context and just cocat the consideration hash (32 bytes) followed by bytes32[] of order hashes so it works out.

[lfportal]

PR for context / discussion: #24

Hi team, we're operating a zone that currently supports substandards 3 & 4:

• Substandard 3:
  • The server applies a number of fees, expected to be provided as extra consideration items during fulfillment
  • The zone validates the substandard 3 data against the actual received items hash to verify all expected fees were paid
• Substandard 4:
  • During a bulk fulfilment (e.g. shopping cart experience), the server allows clients to specify that the fulfillment is all-or-nothing

We have a couple of difficulties supporting our cases:

1. Substandard 3 works fine IF where the fulfiller's desired amount to be filled matches the actual fulfilled amount. This may not be the case where the actual fulfilled amount is less than the original request amount - during the fulfillAvailableAdvancedOrder flow. i.e. substandard 3 is an "exact-or-reject" flow.
2. The SIP-7 specification behaviour is ambiguous (as we understand it) for zones that support multiple substandards - in particular where it comes to encoding of the context argument.

To address <1>, we'd like to propose a new substandard 6, similar to substandard 3, but for "best-efforts" fulfillment where the actual fulfilled amount is less than the requested fulfillment amount. Behaviour as follows:

• Server calculates ReceivedItems hash using item quantities representing complete / total / 100% fulfillment of original order (i.e. numerator: 1, denominator: 1), regardless of fulfiller's desired amount
• Encode substandard 6 data as (uint256, bytes32) where:
  • uint256 represents the originalFirstOfferItemAmount of the first offer item
  • bytes32 represents the server calculated hash
• Zone takes the originalFirstOfferItemAmount / firstSpentItemAmount ratio and scales the ReceivedItem amounts (as if fulfilling 100% of an order) before calculating the hash and comparing to the server calculated hash

To address <2>, we'd like to propose the following clarifications:

• The encoding of multiple substandards requires each substandard segment to begin with the corresponding SIP-7 substandard identifier
• Zones may support multiple substandards, but may not require all substandards be provided in context
  • This allows us to support use cases where substandards may be optional - Substandard 4 in our case

[0age]

One thing that could use additional specification here is how to derive the hash for the received items (eg how to encode the array before hashing it, or if each element should be hashed independently and then an array of hashes gets hashed, etc) — I’d lean towards just flattening the array into item type 1 ++ token 1 ++ identifier 1 ++ amount 1 ++ recipient 1 ++ item type 2 + …

[lfportal]

In our existing implementation of substandard 3 we opted for the latter (hashing individual elements, then hashing the array of hashes), similar to how consideration items are hashed during order hashing.

Some questions:

• Was the original intention for substandard 3 encoding to flatten the array like you're suggesting?
• Is the intended benefit of flattening the array - gas optimisation?

To give a concrete idea of what we're currently doing, modified for substandard 6:

    bytes internal constant CONSIDERATION_BYTES =
        abi.encodePacked("Consideration(", "ReceivedItem[] consideration", ")");

    bytes internal constant RECEIVED_ITEM_BYTES =
        abi.encodePacked(
            "ReceivedItem(",
            "uint8 itemType,",
            "address token,",
            "uint256 identifier,",
            "uint256 amount,",
            "address recipient",
            ")"
        );

    bytes32 internal constant RECEIVED_ITEM_TYPEHASH = keccak256(RECEIVED_ITEM_BYTES);

    bytes32 internal constant CONSIDERATION_TYPEHASH =
        keccak256(abi.encodePacked(CONSIDERATION_BYTES, RECEIVED_ITEM_BYTES));

    // ratioNumerator: actual spent amount of first offer item
    // ratioDenominator: original amount of first offer item (extracted from substandard 6 data)
    function _deriveConsiderationHash(ReceivedItem[] calldata consideration, uint256 ratioNumerator, uint256 ratioDenominator) internal pure returns (bytes32) {
        uint256 numberOfItems = consideration.length;
        bytes32[] memory considerationHashes = new bytes32[](numberOfItems);
        for (uint256 i; i < numberOfItems; i++) {
            considerationHashes[i] = keccak256(
                abi.encode(
                    RECEIVED_ITEM_TYPEHASH,
                    consideration[i].itemType,
                    consideration[i].token,
                    consideration[i].identifier,
                    // scale amount by multiplying by inverse of ratio
                    Math.mulDiv(consideration[i].amount, ratioDenominator, ratioNumerator),
                    consideration[i].recipient
                )
            );
        }
        return keccak256(abi.encode(CONSIDERATION_TYPEHASH, keccak256(abi.encodePacked(considerationHashes))));
    }

If we were to flatten the array, is this the following what you'd expect?

    // ratioNumerator: actual spent amount of first offer item
    // ratioDenominator: original amount of first offer item (extracted from substandard 6 data)
    function _deriveConsiderationHash(ReceivedItem[] calldata consideration, uint256 ratioNumerator, uint256 ratioDenominator) internal pure returns (bytes32) {
        uint256 numberOfItems = consideration.length;
        bytes memory encodedReceivedItems;

        for (uint256 i; i < numberOfItems; i++) {
            encodedReceivedItems = abi.encodePacked(
                encodedReceivedItems,
                consideration[i].itemType,
                consideration[i].token,
                consideration[i].identifier,
                // scale amount by multiplying by inverse of ratio
                Math.mulDiv(consideration[i].amount, ratioDenominator, ratioNumerator),
                consideration[i].recipient
            );
        }

        return keccak256(encodedReceivedItems);
    }

In this version, I'm dropping the type hash, and performing packed encoding:

• Is this hash collision a problem?
  • I suspect this is not a problem given the outer payload SignedOrder is EIP-712 typed data encoded for signing, so possibility of re-use here is irrelevant

[0age]

Yeah no danger of hash collision as it’s all fixed-size elements, and you’re correct that the primary motivation is to improve gas efficiency! If the payload being signed was a proper eip-712 payload there’s a clear benefit to using an analogous hashing methodology, but given it already deviates quite notably I feel like standardizing around the direct “flat” method makes more sense

[lfportal]

Thanks for clarifying. I'll update the PR and drop a link here to our implementation once we're ready - likely sometime next week.

[lfportal]

@0age took a while longer, here are the encoding clarifications: #27

[lfportal]

@0age or @ryanio could I get a review on this one? #27