Payments contract first draft #1

aarshkshah1992 · 2025-02-07T12:42:24Z

This is the first draft for the payments contract MVP. It has the following features:

ERC-20 deposits
ERC-20 withdrawals
rail creation
operator approvals
operator termination
rail payment modification
rail lockup modification
lazy rail settlement
proxy pattern for contract upgrade
arbitration
rate change queuing
avoiding debt creation during lockup or rate modification

All rail modifications and creation respect the operator approval.

The following features which will be added in a subsequent PR:

Escrow
Payment Fees

v1.9.6

README.md

Stebalien · 2025-02-13T01:14:35Z

src/Payments.sol

+        payer.lockupRate = payer.lockupRate - oldRate + rate;
+
+        // Init hasLockupStarted flag and lockupStart for the first rail with non-zero rate
+        if (rate > 0 && !payer.hasLockupStarted) {


Take a look at the "Lockup can be settled..." part of: https://www.notion.so/filecoindev/Payments-Design-Iteration-17ddc41950c180468806c7e0097c2e14?pvs=4#185dc41950c1805da8d4ce567e343307

lockupStart is probably misnamed. Really, it means "the last time we settled lockup". I.e., it's when the lockup rate starts applying.

My point is, I don't think we need hasLockupStarted. We always adjust lockupStart.

Ah, I see you've implemented accumulateLockupRate. You don't need this code here, you just need to call accumulateLockupRate before changing the rate (e.g., at the very top of this function). The first time this is called, that will accumulate nothing because the rate will be 0, but it will initialize lockupStart.

@Stebalien Yeah this makes sense. If totalLockupRate is 0, the lockupstart does not matter.

I'm fixing naming across this contract now for all state. I don't think these names accurately reflect what they're doing. lockupBase for eg dosen't reflect that it's doing three things:

Locking up a fixed security deposit

Locking up accrued payments for services already taken by the client but not yet settled by the provider

Locking up a portion of payments for future services that have yet to be provided to provide some sort of "guarantee" to the provider.

Stebalien · 2025-02-13T20:21:12Z

src/Payments.sol

+        // 1. Fixed lockups from all rails associated with this account.
+        // 2. Accumulated rate-based lockups that have been converted to fixed lockups across all rails.
+        // 3. Funds reserved for future payments based on current payment rates and lockup periods across all rails.
+        uint256 requiredTotalLockedFunds;


I've updated the design doc. I've named this "lockup current" because it covers funds that are actually locked, not the current target locked. I didn't use the word required because the actually locked funds may be less than the "target" (required).

Stebalien · 2025-02-18T17:13:27Z

src/Payments.sol

+    uint256 private _nextRailId;
+
+    // token => owner => Account
+    mapping(address => mapping(address => Account)) public accounts;


We're going to need two features WRT account management:

A way to efficiently (ish) determine the status of an account. Total funds across all tokens, etc. so we can display it in some webui.

Ideally a way to efficiently change the owner address? That's complicated.

Even if we don't support (2), we'll need some top-level account abstraction to support (1).

@Stebalien

Can't we implement those getters as view functions that can be invoked off-chain on RPC providers ? We can add some state to the contract that makes those view/getter functions slightly less compute intensive but IMO making it a view function that can be invoked off-chain means we can offload the worrying about efficiency to client <> RPC provider relationships without having to add too much state to the contract.

Ok, we can solve (1) by:

Adding a mapping(address => address[]) of account owners to tokens.

Optionally: changing token => owner => Account to owner => token => Account. This isn't strictly necessary, but it mirrors the mapping in (1).

As discussed, we can drop (2) and let the user handle indirection through smart contracts.

Actually, maybe we want a single owner => Account mapping where Account contains:

The client's operator approvals (map of operator => Approval).

An array of approved client operators.

An array of client rail IDs.

etc...

Resolution: we're still exploring the API, let's build that first then figure out what these data structures should look at.

src/Payments.sol

Stebalien · 2025-02-18T18:49:53Z

src/Payments.sol

+    function approveOperator(
+        address token,
+        address operator,
+        address arbiter,


Are you sure we want to approve a specific arbiter? I can see the appeal but, in that case... I think we can just let the client decide if they're ok with an arbiter on a per-rail basis.

@Stebalien You mean the client creates the rail OR that a client has to approve a rail that has been created by an operator ? Both have different UX implications.

See my comment at https://github.com/FilOzone/payments/pull/1/files#r1961082248.

An operator can have many different arbitrers for different rails.

So remove this.

The answer here is to trust the operator.

Stebalien · 2025-02-18T19:02:53Z

src/Payments.sol

+
+        Account storage toAccount = accounts[token][to];
+        require(toAccount.ownerAddress != address(0), "to account does not exist");
+        require(toAccount.funds > 0, "to account has no funds");


They don't need funds, do they?

@Stebalien I mean not strictly speaking. We can get rid of this yeah.

Stebalien · 2025-02-18T19:03:22Z

src/Payments.sol

+
+        Account storage fromAccount = accounts[token][from];
+        require(fromAccount.ownerAddress != address(0), "from account does not exist");
+        require(fromAccount.funds > 0, "from account has no funds");


Technically, the sender doesn't need any funds either.

@Stebalien Technically no but I'd like to keep this so that rails are only created with clients who have interacted with the contract before. Just think it's better that way.

Well, this goes back to who can create a rail. IMO, the answer is: the client.

It would be nice if a client can go through the entire signup flow without having to deposit any funds until they actually start using the service.

We agree that neither from or to needs funds.

Stebalien · 2025-02-18T19:08:56Z

src/Payments.sol

+        if (approval.arbitrer != address(0)) {
+            require(arbiter == approval.arbitrer, "arbiter mismatch");
+        }


TBD: need to discuss approved operators and who, precisely, is allowed to call createRail.

Stebalien · 2025-02-18T19:10:34Z

src/Payments.sol

+
+        uint256 railId = _nextRailId++;
+
+        Rail storage rail = rails[railId];


OT: Ethereum's "everything exists, it just defaults to zero" gets me every time. It's kind of cool but also really disconcerting.

@Stebalien What's "OT" ?

"Off topic". I.e., "nothing to do with my review, ignore this, I just wanted to say it."

Stebalien · 2025-02-18T19:59:59Z

.gitignore

+!/broadcast
+/broadcast/*/31337/
+/broadcast/**/dry-run/


Why not just ignore all of /broadcast?

Stebalien · 2025-02-18T19:59:59Z

src/Payments.sol

+    struct ArbitrationResult {
+        bool approved;
+        uint256 modifiedAmount;
+        uint256 settledUntil;


We should consider adding an arbitrary "note" field. Could be useful to explain why the full amount isn't approved.

Added this and also now returning this to the caller of settleRail

Stebalien · 2025-02-18T19:59:59Z

src/Payments.sol

+
+interface IArbiter {
+    struct ArbitrationResult {
+        bool approved;


Do we need approved? IMO, we should just revert if we're not approving anything, but I'm not really a solidity expert.

@Stebalien This is for the Arbitrer contract to gracefully let us know that it is/is not rejecting the payment.

The Payment contract does revert the settlement if it sees a disapproval from the Arbitrer but doing it this way lets us decide what we wanna do when we see a rejection from the Arbitrer.

We can change this so that the Arbitrer reverts rather than returning this approved flag and then handle the revert in the Payment contract as follows:

// Low-level call with error handling (bool success, bytes memory returnData) = IArbitrer(Y).call(....); if (!success) { // Handle the error }

but we need to write some additional encoding/decoding code here for serialisation.

No opinion either way. Wdyt ?

The other option is to not gracefully handle any reverts from the Arbitrer contract at all and simply revert right away (in Solidity, when contract X calls contract Y and contract Y reverts -> the call to X reverts right away unless you handle it as above).

The way we've implemented it here -> any revert from Arbitrer will bubble up right away and the settlement will revert but atleast for payment refusals -> we get to handle it gracefully and do more stuff if we want to before we revert.

The Arbitrer MUST revert if it wants to reject payment.

if arbitrer wants to reject and NOT revert, you can just set settleUpto to the start epoch.

src/Payments.sol

Stebalien · 2025-02-18T19:59:59Z

src/Payments.sol

+        uint256 paymentRate;
+        uint256 lockupPeriod;
+        uint256 lockupFixed;
+        uint256 lastSettledAt;


Hm. Now that I see this, I wonder if we should call this "settledUpTo" instead of "lastSettledAt".

Also renamed the corresponding ArbitrationResult field to settledUpTo

Stebalien · 2025-02-18T19:59:59Z

src/Payments.sol

+    }
+
+    struct OperatorApproval {
+        bool isApproved;


Do we need this field?

@Stebalien Yeah it's the easiest way to track the fact that an operator has been disapproved. Solidity HashMap/Array deletes aren't clean/gas cheap to do.

Also, if operators create rails, it makes sense to:

Approve the operator with no allowance.

Let the operator create rails.

Then give them allowance.

(IMO)

Stebalien · 2025-02-18T19:59:59Z

src/Payments.sol

+
+    struct OperatorApproval {
+        bool isApproved;
+        address arbitrer;


Do we need this field either? Isn't it implicit?

@Stebalien My idea was to have the client have a say in who gets to be an arbitrer.

Stebalien · 2025-02-18T20:21:43Z

src/Payments.sol

+    /// @notice Returns the minimum of two numbers.
+    function min(uint256 a, uint256 b) internal pure returns (uint256) {
+        return a < b ? a : b;
+    }


Make this a free function?

src/Payments.sol

Stebalien · 2025-02-18T20:21:44Z

src/Payments.sol

+    }
+    function _settleWithRateChanges(


Suggested change

}

function _settleWithRateChanges(

}

function _settleWithRateChanges(

Stebalien · 2025-02-18T20:21:44Z

src/Payments.sol

+        uint256 oldLockup = rail.lockupFixed + (rail.paymentRate * rail.lockupPeriod);
+        uint256 newLockup = lockupFixed + (rail.paymentRate * period);
+
+        // assert that operator allownace is respected


Suggested change

// assert that operator allownace is respected

// assert that operator allowance is respected

Stebalien · 2025-02-18T22:11:20Z

src/Payments.sol

+                segmentStart,
+                segmentEnd
+            );
+            require(arbResult.approved, "arbitrer refused payment");


We need to validate that:

arbResult.settledUntil <= settledUntil.

arbResult.modifiedAmount <= rate * (arbResult.settledUntil - segmentStart).

For 1, it should be arbResult.settledUntil <=segmentEnd, right ? Yeah making this change.

Stebalien · 2025-02-18T22:11:21Z

src/Payments.sol

+
+        settleAccountLockup(payer);
+
+        uint256 settlementTargetEpoch = min(untilEpoch, payer.lockupLastSettledAt + rail.lockupPeriod);


Need to check that untilEpoch is at or before the current epoch.

Stebalien · 2025-02-19T03:44:36Z

src/Payments.sol

+                if (nextChange.untilEpoch > currentEpoch && nextChange.untilEpoch <= targetEpoch) {
+                    nextBoundary = nextChange.untilEpoch;
+                    segmentRate = nextChange.rate;
+                }


I've spent a lot of time staring at this code and I'm still not sure if it's right. Let's talk through it tomorrow.

I agree. It is involved.

Let me try to simplify it more today before we sync up.

Co-authored-by: Steven Allen <steven@stebalien.com>

…ents into feat/payments-init-deposit

aarshkshah1992 · 2025-02-19T16:26:58Z

src/Payments.sol

+        bool approved;
+        uint256 modifiedAmount;
+        uint256 settleUpto;
+        string note;


Can leave a note if successful too. For better UX, it's a catch all field for arbitrer to send us info.

aarshkshah1992 · 2025-02-19T16:31:13Z

src/Payments.sol

+
+    }
+
+    function updateRailArbiter(uint256 railId, address newArbiter) external validateRailActive(railId) onlyRailOperator(railId) {


Do we really need this ?

aarshkshah1992 · 2025-02-19T16:46:19Z

src/Payments.sol

+        address token,
+        address from,
+        address to,
+        address operator,


we don't need this as the msg.sender will/should ALWAYS be the operator.

aarshkshah1992 · 2025-02-19T17:10:37Z

src/Payments.sol

+    }
+
+    // TODO: Revisit
+    function terminateOperator(address operator) external  {


who calls this ?

answer: always the client.

@Stebalien Please can you review this function ?

aarshkshah1992 · 2025-02-19T17:16:25Z

src/Payments.sol

@@ -0,0 +1,612 @@
+// SPDX-License-Identifier: UNLICENSED


Write down all the getter APIs. That will determine state design.

Try to make it as easy as possible to access basic state without an Indexer.

Strike a balance between on-chain state and off-chain compute than an RPC provider needs to do to serve Getter requests.

Storing the keys of a Hasmap in an a set if you need iteration. Do it only when needed.

aarshkshah1992 · 2025-02-19T17:23:22Z

src/Payments.sol

+    }
+
+    // TODO: implement
+    function terminateRail() public {


An operator can always do this by setting everything to 0.

If a client wants to terminate a rail -> they go the operator and get them to modify the rates and lockups for the rail to 0.

aarshkshah1992 · 2025-02-19T17:25:27Z

src/Payments.sol

+    }
+
+    struct Rail {
+        bool isActive;


We need this to track rail termination !

aarshkshah1992 · 2025-02-19T17:27:35Z

src/Payments.sol

+
+        Rail storage paymentRail = rails[railId];
+        Account storage payer = accounts[paymentRail.token][paymentRail.from];
+        Account storage payee = accounts[paymentRail.token][paymentRail.to];


Does the solidity compiler optimise this so we only lookup the map once ?

aarshkshah1992 · 2025-02-19T17:28:39Z

src/Payments.sol

+        settleAccountLockup(payer);
+
+        // Determine the maximum allowed epoch we can settle to based on the payer's lockupPeriod.
+        uint256 maxSettlementEpoch = min(untilEpoch, payer.lockupLastSettledAt + paymentRail.lockupPeriod);


can read payer.lockupLastSettledAt from return value.

aarshkshah1992 · 2025-02-19T17:29:58Z

src/Payments.sol

+        uint256 currentPaymentRate = paymentRail.paymentRate;
+
+        // Retrieve any queued rate changes for this rail.
+        RateChangeQueue.Queue storage rateQueue = railRateChangeQueues[railId];


We can put the railRateChangeQueues in the Rail itself.

Stebalien · 2025-02-19T17:33:26Z

src/Payments.sol

+        }
+
+        // Revert if no progress was made (i.e., the final epoch did not advance).
+        require(settledUntil > epochStart, "failed to settle: no progress made in settlement");


Need to support settling nothing to handle partial settlements. E.g., we're going through the queue settling segment by segment and then we hit a segment where we can settle nothing. We need to accept that and settle everything so far without reverting.

aarshkshah1992 · 2025-02-19T17:34:36Z

src/Payments.sol

+        uint256 railId,
+        Rail storage rail,
+        RateChangeQueue.Queue storage rateQueue,
+        uint256 initialRate,


this is the current rail rate. fix naming.

aarshkshah1992 · 2025-02-19T17:35:25Z

src/Payments.sol

+        uint256 activeRate = initialRate;
+        note = "";
+
+        while (currentEpoch <= targetEpoch) {


currentEpoch < targetEpoch is what we need here. if it is equal, we are done and we should exit the loop.

Stebalien · 2025-02-19T17:37:05Z

src/Payments.sol

+                RateChangeQueue.RateChange memory upcomingChange = rateQueue.peek();
+                bool isWithinRange = (
+                    upcomingChange.untilEpoch >= currentEpoch &&
+                    upcomingChange.untilEpoch <= targetEpoch


This second condition won't be true if I'm trying to settle up to some historical epoch in the past.

We need to remove this assert.

Stebalien · 2025-02-19T17:38:50Z

src/Payments.sol

+
+            // Otherwise, we have fully settled up to nextBoundary.
+            currentEpoch = segmentEndEpoch;
+            note = arbNote;


It would be kind of nice to somehow join these notes together when non-empty, but that isn't critical. We mostly care about the last note anyways.

Stebalien · 2025-02-19T17:39:32Z

src/Payments.sol

+            // Dequeue the rate change we just passed (if it was indeed used this round).
+            if (!rateQueue.isEmpty()) {
+                // Peek again to confirm it's the one we used.
+                RateChangeQueue.RateChange memory frontChange = rateQueue.peek();


How would this not be the case?

if the queue is non-empty AND next element has expired -> until epoch is in the past or < targetEpoch -> pop.

actually -> we can JUST dequeue if we reach here.

aarshkshah1992 · 2025-02-19T17:49:39Z

src/Payments.sol

+                    upcomingChange.untilEpoch <= targetEpoch
+                );
+                require(isWithinRange, "rate queue is in an invalid state");
+                nextBoundary = upcomingChange.untilEpoch;


min(next, target)

nextBoundary = min(nextBoundary, target)

Stebalien · 2025-02-19T17:51:33Z

src/Payments.sol

+                railId,
+                rail,
+                currentEpoch,
+                nextBoundary,


pass min(nextBoundary, targetEpoch)

Stebalien · 2025-02-19T17:52:22Z

src/Payments.sol

+        uint256 activeRate = initialRate;
+        note = "";
+
+        while (currentEpoch <= targetEpoch) {


fix comparison.

aarshkshah1992 · 2025-02-19T17:54:15Z

src/Payments.sol

+        return (settledAmount, settledUntil, note);
+    }
+
+    function _settleWithRateChanges(


mark the function as nonReEntrant.

Infact all functions that modify rail state should have a per-rail lock.

Audit and REALLY think through it.

aarshkshah1992 · 2025-02-19T18:34:44Z

src/Payments.sol

+     * @return settledUntil The final epoch that effectively got settled.
+     * @return note An optional note returned by the arbitrator.
+     */
+    function _settleSegment(


per-rail settlement lock

aarshkshah1992 · 2025-02-19T18:46:13Z

src/Payments.sol

+    }
+
+    // railId => RateChangeQueue
+    mapping(uint256 => RateChangeQueue.Queue) public railRateChangeQueues;


move this to the Rail struct.

Stebalien · 2025-02-20T02:49:17Z

The re-entency issue:

We discussed a few approaches:

Lock everything preventing both read and write access to the entire contract while settling a rail. This would restrict access to important information, e.g., information about the rail.
Lock the rail only. Unfortunately, if we lock read access, we'd still be restricting information about the rail. Furthermore, if we don't adjust payee/payer funds while settling and still allow, e.g., the arbiter to see those fields, the arbiter won't see the "full" effects of the last section of payment it arbitrated.

We settled on:

Locking modifications (preventing settling, changing lockup, etc.) to the rail while settling.
Updating the rail, the payee, and the payer after each call to the arbiter such that the arbiter always sees the payment contract in the correct state. Effectively, we move most of the rail settlement logic into the "settle segment" function.

Thinking on this more, there are a few more solutions (which I don't like but I figured I'd mention for completeness).

We could not lock anything, record important state before calling the arbiter, call the arbiter, then revert if "something has changed". This "works" but is strictly more brittle and doesn't provide any benefit.
We could allow payment/lockup changes as long as we're really careful as those changes can only affect future time periods. But I don't think it's worth it and re-entrant modifications are likely always bugs.

Stebalien · 2025-02-20T02:51:22Z

We also discussed how this relates to withdrawal. IMO, the difference between this and withdrawal is that all the "withdraw" call can do is revert. It can't cause us to change our minds and withdraw less than we initially intended so there are no funky intermediate states.

aarshkshah1992 added 7 commits February 7, 2025 10:20

chore: forge init

8ecfd15

forge install: forge-std

1c522f9

v1.9.6

inital PR

181ab6d

draft v0

91fc07d

payments first draft

3c0ecfa

GH CI

cae897b

config file

1bd5f44

aarshkshah1992 changed the title ~~[WIP] Payments contract and proxy scaffold~~ Payments contract first draft Feb 12, 2025

Stebalien self-requested a review February 13, 2025 00:59

Stebalien reviewed Feb 13, 2025

View reviewed changes

aarshkshah1992 added 3 commits February 13, 2025 12:23

fix README

0f03047

address review

751010a

clean up code and variable naming and docs

48c31bd

Stebalien reviewed Feb 13, 2025

View reviewed changes

aarshkshah1992 added 3 commits February 17, 2025 12:34

change account variable names

e539233

change account lookup settlement logic

a614cb4

first draft

509097a

Stebalien reviewed Feb 18, 2025

View reviewed changes

Stebalien reviewed Feb 19, 2025

View reviewed changes

aarshkshah1992 and others added 8 commits February 19, 2025 11:05

add note to arbitration result

8fda600

changes as per review

cb98a0e

Apply suggestions from code review

36e3cb5

Co-authored-by: Steven Allen <steven@stebalien.com>

rename to settledUpto

8fc55a2

Merge branch 'feat/payments-init-deposit' of github.com:FilOzone/paym…

28aa585

…ents into feat/payments-init-deposit

fix arbitrer var name

c759f41

better revert messages for account lockup

f36be7f

improve rate change logic

ce58e17

simplify rate queue settlement

277b541