supply queue cannot have duplicates

[adhusson]

edit: goal is to make sure the values returned by maxDeposit and maxMint are correct

Supply queue is sorted and checked for duplicates on setSupplyQueue.

• Sort chosen to be easy to audit/prove correct, not for performance. But at these sizes and for an admin function I don't think it matters.
• The original supply queue is logged, not the sorted one
  • if the sorted list is logged, code can be simpler

[Rubilmax]

This was formerly suggested in #284

You can see there the reasoning and some numbers on gas cost

The implementation in this PR is probably better because the input can be chosen to be cheaper

[adhusson]

I looked at the gas with 30 markets in this PR, it's +190k if given without sorting, and +30k if given already sorted.

Thanks for the pointer I missed #284 and https://github.com/cantinasec/review-morpho-blue-1/issues/109. To me the main reason is to make sure the values returned by maxDeposit and maxMint are correct, but it may not be worth the complexity.

[Rubilmax]

I looked at the gas with 30 markets in this PR, it's +190k if given without sorting, and +30k if given already sorted.

Thanks for the pointer I missed #284 and cantinasec/review-morpho-blue-1#109. To me the main reason is to make sure the values returned by maxDeposit and maxMint are correct, but it may not be worth the complexity.

These numbers are low enough to me for an admin function so I believe it is worth being merged

[Rubilmax]

I feel like sorted[j] & sorted[j-1] could be stored as variables not only to be cheaper (max hundred gas) but also to be clearer

[Rubilmax]

Like curr = sorted[j], prev = sorted[j-1]

[adhusson]

I think you mean this but let me know if I misunderstood

        uint256 j;
        Id cur;
        Id prev;
        while (j < length) {
            if (j==0) {
                j++;
            } else {
                cur = sorted[j];
                prev = sorted[j-1];
                if (Id.unwrap(cur) >= Id.unwrap(prev)) {
                    j++;
                } else {
                    (sorted[j], sorted[j - 1]) = (prev, cur);
                    j--;
                }
            }
        }

        prev = Id.wrap(0);
        for (uint256 i; i < length; ++i) {
            if (Id.unwrap(prev) == Id.unwrap(sorted[i])) revert ErrorsLib.DuplicateMarket(sorted[i]);
            prev = sorted[i];
        }

it's -23k gas (out of ~350k)

[Rubilmax]

Indeed! Not sure it's better in the end

The last for loop is maybe too much too, simply naming variables in the while loop could be enough

[adhusson]

to clarify, you're saying it may be worth moving duplicate detection inside the while loop. I think it's correct because:

• Some pair of equal elements in the array must end up with both elements adjacent to each other. Take the leftmost such pair.
• If the pair was already adjacent before sorting and never moved, since the sort looks at all initially adjacent pairs once, they will be detected
• If the last move that made the pair adjacent moved the right element of the pair into position, the next comparison is between the elements of the pair (j--), so they will be detected
• If the last move that made the pair adjacent moved the left element of the pair into position, the next comparison will go left (j--) but the loop will not end until j == length and so the elements will compared and detected.

edit: inlining the equality check into the while loop is not worth the gas because it adds one every equality comparison at every step of the sort -- many of which return false but consume gas

[MathisGD]

is there a consensus on if we need this pr or not? I feel that we can just delegate to the risk manager to not have duplicates, and have maxDeposit and maxMint rely on this trust. wdyt?

[MathisGD]

did we think about having a mapping isInSupplyQueue ? It increases the storage size but costs only to the manager, and prevents having this O(n**2) check
edit: bad idea, because we don't add/remove elements in the queue but we set the whole queue. We would need a transient mapping

[MerlinEgalite]

Just had a call with @MathisGD and @Jean-Grimal. We think this PR adds too much complexity while it should be the responsibility of the allocator not to let duplicate markets in the supplyQueue. We prefer just adding a comment see #368.