feat: revise Order event interface #49
Conversation
src/Orders.sol
Outdated
| /// @param amount - The amount of the token to be transferred to the recipient. | ||
| function fulfillSwap(uint256 originChainId, address token, address recipient, uint256 amount) external payable { | ||
| function fill(address recipient, address token, uint256 amount) external payable { |
There was a problem hiding this comment.
why not make this a list of structs too?
There was a problem hiding this comment.
we explored doing this earlier, and at the time we decided it was cleaner code for Fillers to call this function multiple times - once per Output - rather than arrays of structs.
passing arrays when initiating the Order adds new functionality (enabling end users to tie more complex inputs/outputs to an Order than a one-for-one swap), but passing arrays here doesn't add new functionality.
all that said, it probably makes sense to align style between the two contracts now that we've shifted the initiate side...
There was a problem hiding this comment.
when writing this PR I considered doing an array of Outputs here to match style, but I didn't like that the Output struct contains the target chainId, which is self-evident on the target chain itself; I figured that passing extraneous info as calldata && emitting it adds gas costs without benefits in visibility
There was a problem hiding this comment.
all that said, it probably makes sense to align style between the two contracts now that we've shifted the initiate side...
this is what i'm more concerned about. could easily have both single and list?
There was a problem hiding this comment.
explored this in a separate branch: #53
honestly think what's in this branch kinda makes more sense..
maybe at this point let's wrap up this PR and then decide on the other branch as a fast follow?
There was a problem hiding this comment.
@prestwich responded to all your comments - answers are all kind of finicky style things. tried to explain my reasoning clearly, but i'm open to discussion on any of them. lmk what you think.
anna-carroll
force-pushed
the
anna/order-interface
branch
from
96457ef to
b94e158
Compare
per design doc