escrow offer: duration => max duration #153

kutoft · 2025-07-31T13:26:32Z

high level problem:
offers in general are not working as well as we hoped they would.

mm offers are difficult as they are very time sensitive and so mm are reluctant to set a standing offer. The offers mm wants to support are also tied to deribit expiries instead of standard duration times, thus making them all very difficult to reuse.
escrow offers are easier for suppliers to keep standing offers on, but since mm offers have such custom durations, matching an escrow offer to a mm offer becomes extremely difficult and unlikely.

approach:
This change focuses on helping make escrow offers more flexible to support the uniqueness of the mm offers that we see happening.

by changing the escrow offer duration to a max duration, escrow offers are now capable of supporting the custom durations that occur from mm offers.
this change means that the escrow will inherit the duration from the mm offer, ensuring that the durations always match.

note:
think of this PR as a conversation starter and assume there are plenty of discussions, reviews and code changes most likely needed in order to complete this effort (should we all agree on the direction).

src/EscrowSupplierNFT.sol

src/LoansNFT.sol

src/interfaces/IEscrowSupplierNFT.sol

…ated event to include maxDuration.

kutoft · 2025-07-31T13:59:07Z

src/EscrowSupplierNFT.sol

        // The offer is reduced (which is used to repay the previous supplier)
        // A new escrow ID is minted.
-        newEscrowId = _startEscrow(offerId, previousEscrow.escrowed, newFees, newLoanId);
+        newEscrowId = _startEscrow(offerId, previousEscrow.escrowed, newFees, newLoanId, previousEscrow.duration);


I am not confident we should reuse the old duration here. but not sure how else to handle it.

I think switchEscrow should take a newDuration argument for the new position being created. It's used during a roll, and if provider durations aren't stable anymore, it makes sense that a new roll implementation will also be needed, and it will create a position with some newDuration.

In Loans, the newDuration can be loaded from the taker position using _takerId(newLoadId) after the roll.

If we add this now, it would work with current rolls, and with new rolls when it is needed, without having to redeploy escrows twice and migrate their users.

artdgn

A great first draft! Noted a few issues and suggestions for changes.

Additional notes for further work on this:

version strings need to be updated for the touched contracts
changelog entries
audits briefing (for any known issues)
branch unit test coverage back to 100% if is reduced by the end
deploy script changes
fork test changes

artdgn · 2025-08-01T00:28:58Z

src/EscrowSupplierNFT.sol

        return nextTokenId;
    }

+


nit: forge fmt

artdgn · 2025-08-01T00:34:42Z

src/EscrowSupplierNFT.sol

        uint lateFeeAPR,
-        uint minEscrow
+        uint minEscrow,
+        uint maxDuration


info: better to keep the same order everywhere for easier visual comparison (types used, nothing skipped, etc) between inputs and assignments later.

src/EscrowSupplierNFT.sol

artdgn · 2025-08-01T00:43:37Z

src/EscrowSupplierNFT.sol

+     * @param offerId The ID of the offer to update
+     * @param newMaxDuration The new max duration in seconds
+     */
+    function updateOfferMaxDuration(uint offerId, uint newMaxDuration) external {


low: currently almost all stored values are immutable in all contracts, with the exception of offered amounts. Making the maxDuration modifiable adds another exception to this "values are immutable once set" rule, making it more difficult to ensure and review safe use of loaded / derived values. An example of this is the med issue with not validating duration during switchEscrow below (if it wasn't mutable, it would be low).

This also creates a slight (mostly negligible) race condition (frontrunning risk) between updating maxDuration and taking offers. It already exists for offer amounts, but is unavoidable.

It's better that if different max duration is needed, a new offer is created instead. I don't think there are any drawbacks to this (other than requiring two transactions instead of one, can be batched, is an edge case need anyway). Less code -> less mutability, less bugs, cheaper audits, less tests to write.

Additionally, this mutability creates a need to index another event and add correct handling of offer mutability in more places (backed, fronted, integrations)

artdgn · 2025-08-01T01:08:00Z

src/EscrowSupplierNFT.sol

        // The offer is reduced (which is used to repay the previous supplier)
        // A new escrow ID is minted.
-        newEscrowId = _startEscrow(offerId, previousEscrow.escrowed, newFees, newLoanId);
+        newEscrowId = _startEscrow(offerId, previousEscrow.escrowed, newFees, newLoanId, previousEscrow.duration);


I think switchEscrow should take a newDuration argument for the new position being created. It's used during a roll, and if provider durations aren't stable anymore, it makes sense that a new roll implementation will also be needed, and it will create a position with some newDuration.

In Loans, the newDuration can be loaded from the taker position using _takerId(newLoadId) after the roll.

If we add this now, it would work with current rolls, and with new rolls when it is needed, without having to redeploy escrows twice and migrate their users.

artdgn · 2025-08-01T01:12:19Z

src/EscrowSupplierNFT.sol

    {
+        // Check if duration is within the offer's max duration
+        Offer memory offer = getOffer(offerId);
+        require(duration <= offer.maxDuration, "escrow: duration exceeds offer's max duration");


med: this check needs to be in the validations in _startEscrow() instead. Currently it's not checked during switchEscrow. In this version of the code (with the ability to udpate maxDuration) it means a roll can ignore a new maxDuration value. If modifying duration is removed, in a future version of the code - if switchEscrow takes a newDuration as recommended (instead of using previous value) - it means that switchEscrow won't check the newDuration vs. maxDuration.

src/LoansNFT.sol

artdgn · 2025-08-01T01:22:32Z

src/LoansNFT.sol

    // ----- Conditional escrow mutative methods ----- //

-    function _conditionalOpenEscrow(bool usesEscrow, uint escrowed, EscrowOffer memory offer, uint fees)
+    function _conditionalOpenEscrow(bool usesEscrow, uint escrowed, EscrowOffer memory offer, uint fees, ProviderOffer memory providerOffer)


nit: forge fmt

artdgn · 2025-08-01T01:25:13Z

src/interfaces/IEscrowSupplierNFT.sol

        uint escrowed;
        uint feesHeld;
        uint withdrawable;
+        uint32 duration; // duration for this escrow


info: can fit into the first slot (after bool realeased), saving gas.

artdgn · 2025-08-01T01:34:06Z

src/EscrowSupplierNFT.sol

     * @param gracePeriod The maximum grace period duration in seconds
     * @param lateFeeAPR The annual late fee rate in basis points
     * @param minEscrow The minimum escrow amount. Protection from dust mints.
+     * @param maxDuration The duration in seconds for the escrow (calculated from loan expiration)


info: natspec outdated

… duration checks to _startEscrow so its used by rolls as well. remove mutation of maxDuration. get newDuration in switchEscrow from taker position.

init max duration idea

049f3a7

kutoft requested a review from carlosdimatteo July 31, 2025 13:39

carlosdimatteo requested a review from artdgn July 31, 2025 13:40

carlosdimatteo reviewed Jul 31, 2025

View reviewed changes

src/EscrowSupplierNFT.sol Show resolved Hide resolved

src/LoansNFT.sol Show resolved Hide resolved

src/interfaces/IEscrowSupplierNFT.sol Show resolved Hide resolved

tweaks to keep code as close to existing as possible, update OfferCre…

6ab8e62

…ated event to include maxDuration.

kutoft commented Jul 31, 2025

View reviewed changes

artdgn suggested changes Aug 1, 2025

View reviewed changes

kutoft added 2 commits August 6, 2025 14:06

fix param order and specs. add minDuration to offer. move min and max…

4ffe4d3

… duration checks to _startEscrow so its used by rolls as well. remove mutation of maxDuration. get newDuration in switchEscrow from taker position.

move duration to first slot to save on gas

6fd3c3e

escrow offer: duration => max duration #153

Are you sure you want to change the base?

escrow offer: duration => max duration #153

Uh oh!

Conversation

kutoft commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artdgn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

artdgn Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artdgn Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kutoft commented Jul 31, 2025 •

edited

Loading

artdgn left a comment •

edited

Loading

artdgn Aug 1, 2025 •

edited

Loading

artdgn Aug 1, 2025 •

edited

Loading