This comment is not true. after should never be null. Deletion is indicated by isDelete.

• If before is empty, then the object is being created
• If isDelete is true, the the object is being deleted
• Otherwise it's a modification of an object.

I think the correct assert is more like

after != nullptr && (before != nullptr || !isDelete)

This should probably be a "permission"

Thanks, added mustModifyVault (and a TODO to add mayModifyVault which I think you will do)

Remember, the whole purpose of the invariants are to catch things that might be wrong in transactors, so something like "Transactor makes this condition impossible" is the wrong way to look at this. All of these failure conditions should be impossible.

Also, this is absolutely testable using the framework in Invariants_test.cpp. Do a test case that manually creates, modifies, or deletes a vault, but have the transaction type be, I dunno, ttPayment.

Thank you, you are almost right - except I have to use a transaction type which passes the if in the outer scope i.e. one of the Vault transactions.

Thank you, you are almost right - except I have to use a transaction type which passes the if in the outer scope i.e. one of the Vault transactions.

In that case, you should probably check condition this outside of that scope, which it looks like you've already done. Nice!

This could probably also be a permission.

This whole class has a "fatal flaw". (Well, not really fatal, but very significant.)

You're assuming that only one Vault will be affected. What if there are more? You're going to, for example, overwrite befaultVault_ here.

You've got the same problem when you're looking up assets and shares in finalize, where you return the first matching item found. What if there are multiple matches?

It should be impossible, but that is what invariants are here to confirm.

All of these structures and functions should be using collections (vector, set, whatever) instead of single values. You could have a simple check after each one that's like

if (beforeVault_.size() > 1)
{
    XRPL_ASSERT(enforce, ...);
    return !enforce;
}

Thanks, that's great feedback. Making it work is actually simple (since optional is basically a collection of at-most-one)

Thanks, that's great feedback. Making it work is actually simple (since optional is basically a collection of at-most-one)

I figured it would be. 😄

This comment is not true anymore. There's no case above that returns success if afterVault_ is empty, and this test does.

Thank you, I cleaned this up in c73dc5f