Escrow Input Settler

[reednaa]

Updates the escrow input settler with the latest changes already implemented on InputSettlerCompact and gets rid of legacy logic.

Notable changes:

• Large InputSettlerBase refactor with the goal of reducing code duplication, improving code reuse, and reducing total code-size.
• Split Purchase Logic from base implementation. (Goal of simplifying Multichain Inputs #49)
• Removed support for 7683.
• Refunds implemented.

Missing features:

• Testing
• Correct Permit2 call
• Documentation
• Import Cleanup
• Unify Mandate and Permit2Witness types through StandardOrderType.
  • Is it okay that openDeadline === fillDeadline?

[codecov]

Codecov Report

❌ Patch coverage is 88.81579% with 17 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/input/escrow/InputSettlerEscrow.sol	86.81%	12 Missing ⚠️
src/input/InputSettlerBase.sol	89.79%	5 Missing ⚠️

📢 Thoughts on this report? Let us know!

[reednaa]

I managed to reduce the witness of permit2 to the same size as TheCompact however, the timestamps of Mandate and Permit2Witness represent 2 different things:

• Mandate: Contains fillDeadline since expires is encoded in the Claim as expires.
• Permit2Witness: Contains expires since fillDeadline is encoded in PermitBatchWitnessTransferFrom as openDeadline.

Ideally we would use the same logic to hash these but how do manage the typestring?

[reednaa]

Self review comment:
Looks good. The PR unfortunately touches many sections that are not relevant to its goal and those should have been moved out into separate PRs. However, the PR gets rid of many legacy components in a fairly natural way.

I am happy with the PR and only 1 unsolved point remains.

Once merged, the major outstanding issues on #49 Multichain Inputs should be resolved.

[luiz-lvj]

LGTM @reednaa!
I just left a few comments.

Is it okay that openDeadline === fillDeadline?

I think this is fairly ok, there are edge cases where solvers would not have time to fill the intents, but with fillDeadline === openDeadline +1 wouldn't make a difference.

A few points for discussion:

• Why the choice to remove ERC7683 for now?
• I believe the purchase logic should be less integrated directly on OIF. Imo _allowExternalClaimant makes sense on BaseInputSettler but instead of inheriting from it in order to create InputSettlerPurchase, we should have an independent contract, e.g. PurchaseRouter, in order to do it

[luiz-lvj]

Should we validate that order.expires > order.fillDeadline? otherwise

Suggested change

	if(order.expires <= order.fillDeadline) {
	revert();
	}

[reednaa]

I see the argument but I don't see why it needs to not be allowed. It would also add a difference to the Compact order which I generally don't like.

[reednaa]

believe the purchase logic should be less integrated directly on OIF. Imo _allowExternalClaimant makes sense on BaseInputSettler but instead of inheriting from it in order to create InputSettlerPurchase, we should have an independent contract, e.g. PurchaseRouter, in order to do it

_allowExternalClaimant is used even without the purchase logic. So it is needed on the base implementation regardless of whether we remove the purchase logic. It is the one covering the AllowOpen structure / finaliseWithSignature

Why the choice to remove ERC7683 for now?

I don't want this PR to be about 7683 and I don't believe that the resolved orders is likely to remain similar. As a result, I choose the simple route of removing the associated logic instead of maintaining a broken implementation of it. If it is needed, I am open to making a second PR that adds these resolved orders back.

However, until we have a further spec for 7683 description how the flow works, I am more in favour of simplifying the logic so that in the future it becomes easier & lower lift to add support for it to both compact and escrow in 1 swoop.

[luiz-lvj]

I don't want this PR to be about 7683 and I don't believe that the resolved orders is likely to remain similar. As a result, I choose the simple route of removing the associated logic instead of maintaining a broken implementation of it. If it is needed, I am open to making a second PR that adds these resolved orders back.

However, until we have a further spec for 7683 description how the flow works, I am more in favour of simplifying the logic so that in the future it becomes easier & lower lift to add support for it to both compact and escrow in 1 swoop.

It makes sense, I think the 7683 integration should come in a later PR when we have more clarity

[reednaa]

Yeah. And even with that said, I have been trying to make the function interfaces as easily convertible as possible. As you notice, they match except the order is a struct not bytes.

[luiz-lvj]

LGTM!