Make Open function 7683 compliant - #80
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Will build resolve on-top: #82 Given our discussion surrounding what the resolve would actually look like and the fact that this is a read only extension, I decided not to finish it. |
| library StandardOrderType { | ||
| using StandardOrderType for bytes; | ||
|
|
||
| function orderIdentifier( |
There was a problem hiding this comment.
Maybe it would make sense to differentiate the functions when using StandardOrder type or bytes
| function orderIdentifier( | |
| function getOrderIdentifierFromBytes( |
There was a problem hiding this comment.
Same for the functions below
There was a problem hiding this comment.
I prefer reading the bytes as a struct for readability purposes.
This is especially true if we go a route where we have both the struct and the bytes. (i.e. not using bytes on the finalise function).
jsanmigimeno
left a comment
jsanmigimeno
left a comment
There was a problem hiding this comment.
Looks good, just minor comments
|
|
||
| // Collect input tokens | ||
| _openFor(order, signature, address(this)); | ||
| if (msg.sender == sponsor && signature.length == 0) _open(order); |
There was a problem hiding this comment.
Why would this ever be the case? Why wouldn't the sponsor just call open().
There was a problem hiding this comment.
There is an argument to be made that the open call is not needed. As a result, if that function gets removed in future contracts this contract would remain compatible.
| /// @dev This method must emit the Open event | ||
| /// @param order The GaslessCrossChainOrder definition | ||
| /// @param order The order in raw encoding | ||
| /// @param user The user's address |
There was a problem hiding this comment.
Shouldn't the second parameter be 'sponsor'?
There was a problem hiding this comment.
This is directly from the new spec. I would argue that sponsor better represents our usage.
|
|
||
| /// @title IOriginSettler | ||
| /// @notice Standard interface for settlement contracts on the origin chain | ||
| interface IOriginSettler { |
There was a problem hiding this comment.
This interface is not being implemented anywhere, should it be implemented by InputSettlerEscrow?
There was a problem hiding this comment.
Resolve has not been defined yet.
| /// @param order The GaslessCrossChainOrder definition | ||
| /// @param originFillerData Any filler-defined data required by the settler | ||
| /// @param order The order in raw encoding | ||
| /// @param user The user's address |
There was a problem hiding this comment.
Should this be 'sponsor'?
There was a problem hiding this comment.
This is directly from the new spec. I would argue that sponsor better represents our usage.
| /// @param user The user's address | ||
| /// @param signature The user's signature over the order (optional) | ||
| /// @return ResolvedCrossChainOrder hydrated order data including the inputs and outputs of the order | ||
| function resolveFor( |
There was a problem hiding this comment.
Is this function required? It's not implemented anywhere.
Also, the function docstring mentions GaslessCrossChainOrder which doesn't exist anymore.
There was a problem hiding this comment.
This function is not final yet.
3 participants
The goal of this PR is to standardise the
openXinterfaces to 7683 interfaces. The following interfaces has been used for this PR:Note: the implementation treats
openForasopenifsponser == msg.sender && signature.length == 0Each function takes in
abi.encode(StandardOrder). This minimises integration overhead significantly at the cost of slightly increasing the calldata usage. The alternative would have been a custom encoding, however, I would argue that this adds unnecessary integration friction.The OIF repository requires that everything is kept in calldata. To ensure that this is met, a
bytespointer is passed around and then the correct calldata slices are loaded on demand.uint256[2][] inputsandMandateOutput[] outputsare loaded as struct calldata slices for readability and compatibility purposes.Open questions:
bytes orderto finalise?uint256? (Argument: We don't use the bytes length thus saving us 1 stack element)This PR closes: #79