It's hard to tell with your test case/suite structure but do you have tests where the issuer is the depositor? Also you test frozen but I don't see deep freeze or auth or no ripple?

In this branch, there is withdrawal to 3rd party which happens to be issuer

However this branch does not allow the issuer to be also depositor. This is fixed in #5518 and the test to match is

I'm going to suggest a little reorganization here.

Basically, don't load the AccountRoot SLE until after you've verified that the line doesn't exist, and then when you do, specify it as account_, so it's clearer which account you're dealing with.

Even though computing the Keylet for the line is slightly more expensive than computing the Keylet for the account, if the line already exists, or if dstAcct != account_, you'll skip computing the account key entirely, and loading the object entirely.

Thanks, this is good.

this change is for consistency with other transaction types

this change is for consistency with other transaction types

This code is basically duplicated from the account != dstAccount block, other than the result for a missing account.

Would it make more sense to take this code out of the if statement, and check which account before deciding which TER to return?

Something like

    auto const sleDst = ctx.view.read(keylet::account(dstAcct));
    if (sleDst == nullptr)
        return account == dstAcct : tecINTERNAL : tecNO_DST;

    if (sleDst->isFlag(lsfRequireDestTag) &&
        !ctx.tx.isFieldPresent(sfDestinationTag))
        return tecDST_TAG_NEEDED;  // Cannot send without a tag

    // If sending to Account (i.e. not a transfer), we will also create
    // (only if authorized) a trust line or MPToken as needed, in doApply().
    AuthType authType = AuthType::WeakAuth;
    if (account != dstAcct)
    {
        if (sleDst->isFlag(lsfDepositAuth))
        {
            if (!ctx.view.exists(keylet::depositPreauth(dstAcct, account)))
                return tecNO_PERMISSION;
        }
        // The destination account must have consented to receive the asset by
        // creating a RippleState or MPToken
        authType = AuthType::StrongAuth;
    }

Shouldn't also check if dstAcct == account_? A trusdtline or MPToken only should be created if that condition is true.

Good idea, I will do it.

Don't need this. Can just let it fall through.

Don't need this. Can just let it fall through.

dstAcct may be different from account_. I think should only execute std::visit if dstAcct == account_.

Agree, however this condition is not faulty - it just relies on check performed in preclaim. But I like your idea better.

I think can simplify testSequence and testCases by defining the accounts once at the start of teseSequences:

void
testSequences()
{
    using namespace test::jtx;
     Account const issuer{"issuer"};
     Account const owner{"owner"};
     Account const depositor{"depositor"};
     Account const charlie{"charlie"};  // authorized 3rd party
     Account const dave{"dave"};

    auto const testSequence = [this](
                                  std::string const& prefix,
                                  Env& env,
                                  Vault& vault,
                                  PrettyAsset const& asset)

This could be true for some other code when the passed accounts are always the same. For instance, issuer is always `Account{"issuer"}'.

Good idea

This is more efficient:

 if (!view().exists(keylet::line(dstAcct, issue)))

also more explicit - thanks !

This is more efficient:

 if (!view().exists(keylet::mptoken(issue.getMptID(), account_)))

If this list of Account params ever gets too unwieldy, you can simply "recreate" them inside the function. The values are deterministic and cached, so you don't have to worry about wasting resources either.

If this list of Account params ever gets too unwieldy, you can simply "recreate" them inside the function. The values are deterministic and cached, so you don't have to worry about wasting resources either.

I see you have already rewritten them to have the accounts be in an outer scope, which is even simpler.

this change is for consistency with other transaction types

This code is basically duplicated from the account != dstAccount block, other than the result for a missing account.

Would it make more sense to take this code out of the if statement, and check which account before deciding which TER to return?

Something like

    auto const sleDst = ctx.view.read(keylet::account(dstAcct));
    if (sleDst == nullptr)
        return account == dstAcct : tecINTERNAL : tecNO_DST;

    if (sleDst->isFlag(lsfRequireDestTag) &&
        !ctx.tx.isFieldPresent(sfDestinationTag))
        return tecDST_TAG_NEEDED;  // Cannot send without a tag

    // If sending to Account (i.e. not a transfer), we will also create
    // (only if authorized) a trust line or MPToken as needed, in doApply().
    AuthType authType = AuthType::WeakAuth;
    if (account != dstAcct)
    {
        if (sleDst->isFlag(lsfDepositAuth))
        {
            if (!ctx.view.exists(keylet::depositPreauth(dstAcct, account)))
                return tecNO_PERMISSION;
        }
        // The destination account must have consented to receive the asset by
        // creating a RippleState or MPToken
        authType = AuthType::StrongAuth;
    }

This could/should return tesSUCCESS if the line already exists imo.

Also you call isGlobalFrozen here but also call it in preclaim too right? canAddHolding does the same checks as this but I don't see canAddHolding called? Does that mean that this function can return internal?

This could/should return tesSUCCESS if the line already exists imo.

There are callers where it would be a weird problem if the holding already exists. For example, the transactions that create new accounts.

Normally I would suggest an additional flag as a function input parameter to choose the semantics of duplicates handling. In this case, if we want to go this way, #5270 would be a better fit to make a change (I would not want this small PR to be an obstacle for the larger PR to trip on).