revert on failed status update after authorizeOrder call #30
Conversation
src/core/lib/OrderCombiner.sol
Outdated
| bool revertOnInvalid | ||
| ) internal pure returns (bool revertOnFailedUpdate) { | ||
| assembly { | ||
| revertOnFailedUpdate := or(revertOnInvalid, gt(orderType, 1)) |
There was a problem hiding this comment.
Just explaining in my own words to check my understanding: we want to always provide revertOnInvalid to _updateStatus because at this point, for restricted (and contract, technically) orders, we already called the pre-hook, which could have modified state. If we fail updating the status (eg order was already filled or cannot apply the supplied fraction) for an order in the fulfillAvailable case, we want to revert rather than simply mark as "unavailable" and continue, because the authorizeOrder call is stateful, and we won't call the complementary validateOrder call for "unavailable" orders (and the supplied spent/received items would not technically be correct in either case).
| @@ -507,7 +511,10 @@ contract OrderCombiner is OrderFulfiller, FulfillmentApplier { | |||
| orderHash, | |||
| advancedOrder.numerator, | |||
| advancedOrder.denominator, | |||
| revertOnInvalid | |||
| _revertOnFailedUpdate( | |||
There was a problem hiding this comment.
if we get here _checkRestrictedAdvancedOrderAuthorization must have returned isValid=true (otherwise we continue the loop). Then if it's a restricted order and the update fails we always revert.
I get the idea: authorizeOrder was called and therefore we cannot skip this order anymore because we would also skip the corresponding validateOrder call later, so we revert.
An edge-case is that _checkRestrictedAdvancedOrderAuthorization can return true for restricted orders even if no authorizeOrder call was done, i.e., when the caller is the zone itself. So it seems to be more restrictive than it needs to be as skipping in the case of caller == zone should still work fine? Not saying that we have to fix this edge-case, just pointing it out.
And of course, if a fulfillAvailable overfills an order and updateStatus returns false, you're reverting the entire tx now instead of just skipping the orders that would overfill. Doesn't this kind of go against what fulfillAvailable was designed for? Let's say I want to buy up to 5 NFTs, so I create 5 orders buying 1 each, but there are only 4 available now. It would revert?
(If the fix for the other issue ends up simulating the updated order fill sizes already in the validation step, you might be able to detect early which orders can be skipped, essentially updateStatus calls should then never fail and it would fix this problem too.)
There was a problem hiding this comment.
That's a good point about the edge case — will try adding it in (hopefully it doesn't exceed stack depth!)
I agree that it's not optimal to revert the whole batch in these cases, but in practice this will only occur when fulfilling the same order multiple times; and there's still a workaround if you have the zone itself track the remaining fill fraction using tstore
If we come up with an effective way to track fill fractions in flight then we can always roll this change back, but IMHO it's more important to guarantee that the authorizeOrder invariants are upheld than that orders will always be skipped if possible
|
Merging, but leaving a note that if we take a different approach with |
No description provided.