Refactor to remove Head and Tail address

[Tristan22400]

Fixes : #85

[MathisGD]

I think that insertSorted can be completely refactored with this new design. See morpho-org/morpho-optimizers#1255

[QGarchery]

While I love the idea, it would also require a big refactor of the verification done with Certora. Is it worth to invest some time into it ? @morpho-org/onchain-team

[MerlinEgalite]

While I love the idea, it would also require a big refactor of the verification done with Certora. Is it worth to invest some time into it ? @morpho-org/onchain-team

If it would take you 1 day or something I'm against and we can forget about it

[QGarchery]

While I love the idea, it would also require a big refactor of the verification done with Certora. Is it worth to invest some time into it ? @morpho-org/onchain-team

If it would take you 1 day or something I'm against and we can forget about it

Yes probably one or two days to do it: I know well how it works now but it still requires to refactor almost all the invariants

[MathisGD]

this should work too, idk what's best

Suggested change

	while (numberOfIterations < maxIterations && next != address(0) && list.accounts[next].value >= value) {
	next = list.accounts[next].next;
	unchecked {
	++numberOfIterations;
	}
	}
	unchecked {
	while (numberOfIterations++ < maxIterations && next != address(0) && list.accounts[next].value >= value) {
	next = list.accounts[next].next;
	}
	}

[makcandrov]

In this implementation, on the last iteration, numberOfIterations will also be incremented. At the end of the loop, numberOfIterations will be one greater than in the current implementation. To replicate the same behavior as the current implementation, the comparison numberOfIterations == maxIterations would need to be changed to numberOfIterations == maxIterations + 1

[makcandrov]

To minimize storage reads (which is the heaviest operation), I would have written it like this:

uint256 numberOfIterations;
address prevId = address(0);
address nextId = list.accounts[address(0)].next; // If not added at the end of the list `id` will be inserted before `next`.
Account storage nextAccount = list.accounts[nextId];

unchecked {
    while (true) {
        if (++numberOfIterations == maxIterations) {
            nextAccount = list.accounts[address(0)];
            prevId = nextAccount.prev;
            break;
        } else if (nextId == address(0) || nextAccount.value < value) {
            break;
        } else {
            prevId = nextId;
            nextId = nextAccount.next;
            nextAccount = list.accounts[nextId];
        }
    }
}

list.accounts[id] = Account(prevId, nextId, value);
list.accounts[prevId].next = id;
nextAccount.prev = id;

gas diff

[MathisGD]

this works too, but maybe less readable

Suggested change

	if (numberOfIterations == maxIterations) {
	next = address(0);
	}
	assembly {
	next := mul(next, iszero(eq(numberOfIterations, maxIterations)))
	}

[makcandrov]

To minimize storage reads (which is the heaviest operation), I would have written it like this:

uint256 numberOfIterations;
address prevId = address(0);
address nextId = list.accounts[address(0)].next; // If not added at the end of the list `id` will be inserted before `next`.
Account storage nextAccount = list.accounts[nextId];

unchecked {
    while (true) {
        if (++numberOfIterations == maxIterations) {
            nextAccount = list.accounts[address(0)];
            prevId = nextAccount.prev;
            break;
        } else if (nextId == address(0) || nextAccount.value < value) {
            break;
        } else {
            prevId = nextId;
            nextId = nextAccount.next;
            nextAccount = list.accounts[nextId];
        }
    }
}

list.accounts[id] = Account(prevId, nextId, value);
list.accounts[prevId].next = id;
nextAccount.prev = id;

gas diff

[makcandrov]

this function can be optimized:

function remove(List storage list, address id) internal {
    Account storage account = list.accounts[id];
    if (id == address(0)) revert AddressIsZero();
    if (account.value == 0) revert AccountDoesNotExist();

    address prevId = account.prev;
    address nextId = account.next;
    list.accounts[prevId].next = nextId;
    list.accounts[nextId].prev = prevId;

    account.prev = address(0);
    account.next = address(0);
    account.value = 0;
}

gas diff

The line if (id == address(0)) revert AddressIsZero(); can be removed, but the error thrown would no longer be accurate in the case of a null address.

[Tristan22400]

I will implement but not sure we will merge the PR as mentioned above

[MerlinEgalite]

can you remove changes to the test @Tristan22400 please?

[QGarchery]

I think we do not want to merge because of this discussion

[MerlinEgalite]

Agree with you @QGarchery. Sorry about that @Tristan22400 I'm closing this PR