List active sales

[emizzle]

Allows a host to retrieve its active slots. Once a slot is freed or paid out, the active slot is removed. If the request fails, all the slots for the host are removed for that request.

NOTE: While active slots are removed once a slot is freed, if the maxSlotLoss is reached and the request is considered failed, the remaining active slots for the host iterated and removed. This requires calling EnumerableSet.Bytes32.values() which could potentially run out of gas in the case of an unbounded iteration. However, in this particular case, it should be bounded by the number of slots in the request minus maxSlotLoss, which triggers the failure of the request.

[markspanbroek]

While active slots are removed once a slot is freed, if the maxSlotLoss is reached and the request is considered failed, the remaining active slots for the host will not be removed.

Would it be correct to phrase the problem as follows? When a request fails, we need to remove all slots from their respective activeSlots[host] set. Because we don't know the amount of slots beforehand, this could lead to a loop that has too high a gas cost to be executed.

This seems similar to the problem we had with account locks. There we also need to be able to unlock an unbounded amount of locks when a request finishes. Perhaps we can solve the active slots problem in a similar way that we solved the active locks problem in AccountLocks?

This would entail running a cleanup function (removeInactiveSlots) whenever we want to add a new slot to the list. We would also have to ensure that the list never grows to a size whereby this cleanup would take up too much time. Perhaps we could generalize the code in AccountLocks such that we can use it for both account locks and slot lists?

[emizzle]

There may not be an issue with an unbounded list as the upper bound will always be the number of slots in the request minus maxSlotLoss. The issue is more around the inefficiencies (in gas costs) of listing all active slots (in the EnumerableSet.Bytes32 data structure) and then removing them. This requires a call to activeSlots.values which copies the "entire storage to memory":

WARNING: This operation will copy the entire storage to memory, which can be quite expensive. This is designed to mostly be used by view accessors that are queried without any gas fees. Developers should keep in mind that this function has an unbounded cost, and using it as part of a state-changing function may render the function uncallable if the set grows to a point where copying to memory consumes too much gas to fit in a block.

Perhaps the gas consumed may actually be reasonable considering the size of our EnumerableSet has a small bound?

Personally, I find that adding an additional layer like a generalised AccountLocks may make the code more difficult to reason about. Perhaps there is another solution, such as modifying the mySlots function to only return activeSlots for a request that isn't failed?

[emizzle]

The OpenZeppelin documentation suggests using an array of EnumerableSets, and incrementing an index counter as an effective means of clearing it. This issue in the repo points to an example of how to implement it. It turns out that an array panics, so a mapping of EnumerableSets was used instead.

[emizzle]

I realise now that this removes all slots for all requests, not just the request that failed. Working on a fix...

[emizzle]

I reverted the mapping index improvement, and opted to iterate over the values of the EnumerableSet.Bytes32. This has the potential to run out of gas in an unbounded iteration, however we are bounded by our slots - maxSlotLoss, so it should hopefully not occur. Thoughts?

[markspanbroek]

I reverted the mapping index improvement, and opted to iterate over the values of the EnumerableSet.Bytes32.

If I understand the code correctly, then we're only cleaning up the active slots for one host, not for the other hosts that occupy a slot in the failed request.

This has the potential to run out of gas in an unbounded iteration, however we are bounded by our slots - maxSlotLoss, so it should hopefully not occur. Thoughts?

Users can choose how the value of slots and maxSlotLoss, so they can easily choose values that lead to out of gas problems.

[emizzle]

If I understand the code correctly, then we're only cleaning up the active slots for one host, not for the other hosts that occupy a slot in the failed request.

Yes, that's correct and it is now changed so that all hosts' slots in that request are removed from activeSlots.

Users can choose how the value of slots and maxSlotLoss, so they can easily choose values that lead to out of gas problems.

Fair point, I was considering this myself, however my conclusion was that we would be able to limit users from using unreasonable parameters quite easily.

After more thought, and the considerations above, the index mapping for activeSlots was reinstated, however the data structure is a bit different. The main difference comes from a change in mySlots: it now requires a RequestId as a parameter (which can be obtained using myRequests. This allowed for the activeSlots mapping to be keyed on RequestId, which gives a mapping of ActiveSlotId => EnumerableSet.Bytes32. ActiveSlotId is a keccak hash of address + activeSlotIdx, which not only allows us to retrieve all active slot ids per request per address, but also allows us to clear all active slot ids for a request by incrementing the activeSlotIdx.

requestId => [ keccak(address + activeSlotIdx) => EnumerableSet.Bytes32 ]

The downside of this is that the caller requires a requestId to call mySlots. This can be obtained using the myRequests() call, but unfortunately adds yet another step to restoring state on the codex side.

[emizzle]

myRequests() only returns requests that you've submitted yourself. So we'd need to keep track of the requests that host participates in elsewhere. Perhaps we could then also create a getter that read all requests, and for all request their slots. We wouldn't have to worry about gas cost since that would be a view function.

When restoring the state in a restarted node, we'd only need to get requests that we've submitted ourselves, no?

[emizzle]

The large mapping piece was moved to a separate library (without tests yet) with a generalised API. I'm not entirely sure how we can use this library to replace AccountLocks, as there is an expiry piece that won't be handled.

I could be mistaken, but I feel that AccountLocks could be replaced by activeRequests which is a mapping of address => EnumerableSet.Bytes32 (RequestId). Collateral could simply require there are no requests for an address, or no requests that aren't expired, before allowing withdrawal. Collateral doesn't have access to activeRequests however, because it's a property of Marketplace and Marketplace inherits from Collateral.

[emizzle]

Latest commit adds ability to get all requests, which will be nice to list all requests > addresses > slots. However, this still doesn't work for AccountLocks because of inheritance. Ideas?

I realise this latest commit was a bit hasty. In this fashion, requests wouldn't be known until slots are filled. I have another idea of how to add a structure using SetMap for all requests. But again, I'm not entirely sure how we can extrapolate this to handle AccountLocks.

[markspanbroek]

When restoring the state in a restarted node, we'd only need to get requests that we've submitted ourselves, no?

The Purchasing module would need to get all requests that it submitted itself. The Sales module would need to get all requests where it filled a slot.

The large mapping piece was moved to a separate library

Very nice!

I could be mistaken, but I feel that AccountLocks could be replaced by activeRequests

Good idea, that could simplify things a lot.

Collateral doesn't have access to activeRequests

Collateral only needs to check whether a withdrawal is allowed. We could make Collateral abstract, and define a virtual function in Collateral that Marketplace could then implement:

function _isWithdrawAllowed(address account) internal virtual view returns (bool);

But it would be perfectly ok to leave the rework of AccountLocks for a later PR.

[markspanbroek]

Closing because alternative #24 was merged.