I would actually split these if blocks into 4 separate functions, each checking their own condition. When named properly that would improve readability.

IMO it's a lot of code repetition. I added comments and broke the if expression, which checks the invariant. It should be a lot better now.

Maybe should add //clang-format off for this file and revert this change? I personally don't really see this as a problem though

Makes sense to add // clang-format off

This file does not have an extension expected by clang-format, so it will not be automatically formatted by it. No need for //clang-format off here.

My VS code setup has application of clang-format on save. So when I add new feature to this file, it reformats it on save regardless of it's extension.

I need to update my local tooling, which runs clang-format on every changed file. Just need to add an exception for *.macro.

Maybe worth to put on the top of the file (same as other free functions)?

maybe worth using auto const& to avoid copying the rules?

nit: this comment doesn't bring any value

I appreciate deduplication, but I find this code too difficult to read. Lambda + callback is too much for my taste.

I'd suggest something like this:

void
ValidAMM::visitEntry(
    bool isDelete,
    std::shared_ptr<SLE const> const& before,
    std::shared_ptr<SLE const> const& after)
{
    if (isDelete)
        return;

    // Check if pool changed
    auto ammPoolChanged = [&](std::shared_ptr<SLE const> const& sle) {
        auto const type = sle->getType();
        return sle->isFieldPresent(sfBalance) &&
            ((type == ltRIPPLE_STATE && sle->getFlags() & lsfAMMNode) ||
             (type == ltACCOUNT_ROOT && sle->isFieldPresent(sfAMMID)));
    };

    // Check if entry is AMM
    auto isAMM = [](std::shared_ptr<SLE const> const& sle) {
        auto const type = after->getType();
        return type == ltAMM;
    };

    if (after)
    {
        if (isAMM(after))
        {
            ammAccount_ = after->getAccountID(sfAccount);
            lptAMMBalanceAfter_ = after->getFieldAmount(sfLPTokenBalance);
        }
        else if (ammPoolChanged(after))
        {
            ammPoolChanged_ = true;
        }
    }

    if (before)
    {
        if (isAMM(before))
        {
            lptAMMBalanceBefore_ = before->getFieldAmount(sfLPTokenBalance);
        }
        else if (ammPoolChanged(before))
        {
            ammPoolChanged_ = true;
        }
    }
}

I also wonder if ammPoolChanged should be checked for before. To me it seems like both lsfAMMNode and sfAMMID flags can be only set and not removed. And if this is a deletion, we shortcut early. Also I've not found a single occurence of isFieldPresent(sfBalance), whole codebase assumes it's always present.

So I assume it's sufficient to check these flags only for after and not check sfBalance flag existence (assuming it's always present for both trust line and amm node), thus we could simplify the code further

I'd suggest a shortcut here.
if (result != tesSUCCESS)
return true;

ammAccount_ is checked on every if/else statement, maybe take it out? Is that even possible to have a situation where ammAccount_ would not be present? Should it be an assert of some sort?

It's not checked on ttAMM_VOTE and if it's present in ttPAYMENT then it's an error. I think in other cases should return false. I.e., consider it as variant failure.

I'm not convinced that these callbacks are required here. Callback is not even a correct term here.

All those callbacks are either identical or calculable from one another:

auto amtNoRoundCb = [&] { return tokensAdj / ePrice; };
    auto amtProdCb = [&] { return tokensAdj / ePrice; };

or

auto tokNoRoundCb = [&] {
        return lptAMMBalance * **(lptAMMBalance + ae * (f - 2)) /
            (lptAMMBalance * f - ae);**
    };
    auto tokProdCb = [&] {
        return **(lptAMMBalance + ae * (f - 2)) / (lptAMMBalance * f - ae)**;
    };
where tokNoRoundCb() == tokProdCb() * lptAMMBalance;

So if we don't want to do calculation twice, we can do that without lambdas relatively easy.

Overall I would suggest revisiting all such callback taking functions. They're probably not needed and could be simplified/refactored into more easy readable code

It's not that straightforward. First, deposit/withdraw logic is different. And I'd rather keep this logic in respective deposit/withdraw function than have a complicated logic in one function. Second, an important thing is expression evaluation order. Let's take tokNoRoundCb and tokProdCb. Conceptually tokNoRoundCb = A * B / C and tokProdCb = B / C. We could represent tokNoRoundCb = A * tokProdCb. But A * B / C is evaluated as (A * B) / C and we need to keep this evaluation order because it's pre-amendment code. If we change it to A * (B / C) then the result might be different due to the rounding. We must have the same result when this amendment is disabled.

I would like to see unit tests checking this invariant. I understand it's probably going to take a lot of effort thought.

While the current version of the code is definitely much more readable that previous, I would still suggest splitting finalize into a couple of different functions each covering certain scenario (or group of scenarios). That would make writing and reading tests much easier for this invariant

It is pretty hard to unit-test this invariant. First, the rounding change in this PR has mitigated the invariant failure. There are some unit-tests, which would have failed have it not been for this fix. They don't fail because there is no an invariant check for deposit/withdraw pre-amendment but the error messages are still logged. Second, there is a fudge factor - an allowance for a small error. This makes it really difficult to make this invariant fail. One option is to remove this fudge factor but then there will be deposit/withdraw failures because of some pretty tiny differences and it might not even be an edge case so I'm reluctant to do this.

the codes I suggested are already existing codes so it does not have to be added.

Leaving up to your judgement which one is more suitable