Frozen IOUs should still be transferable to/from the issuer

[vlntb]

High Level Overview of Change

The freeze logic was incorrectly blocking IOU issuers from transacting with their own frozen currency. This violated the XRP Ledger protocol, which specifies that "Counterparties of the frozen issuer can still send and receive payments directly to and from the issuing address."

Context of Change

Three issues were fixed:

1. isFrozen() function was returning true for issuers checking their own currency during global freeze, causing the issuer to be treated as frozen. The early return prevented reaching the later issuer exemption check.
2. LoanBrokerCoverDeposit::preclaim() was using accountHolds() with FreezeHandling::fhZERO_IF_FROZEN for all accounts, including issuers. Since issuers don't have a "balance" of their own tokens (infinite issuance ability), the balance check would return 0 and fail with tecINSUFFICIENT_FUNDS.
3. VaultWithdraw::preclaim() had checkFrozen() calls that were always executed, even when the destination or submitter was the asset issuer. Now skips freeze checks when either the destination or submitter is the IOU issuer.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Refactor (non-breaking change that only restructures code)
• Performance (increase or change in throughput and/or latency)
• Tests (you added tests for code that already exists, or your new feature included in this PR)
• Documentation update
• Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
• Release

API Impact

• Public API: New feature (new methods and/or new fields)
• Public API: Breaking change (in general, breaking changes should only impact the next api_version)
• libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
• Peer protocol change (must be backward compatible or bump the peer protocol version)

[ximinez]

There are a bunch of unit test failures

[ximinez]

This doesn't look right. Only the broker owner (owner) should be able to deposit cover. Not because of the freeze, but because of the LP rules. The CI results confirm that - there are many unit test failures.

[vlntb]

Done

[ximinez]

Should also verify that the issuer can still send payments.

        // Verify issuer can still send payments (uses isFrozen internally)
        env(pay(issuer, holder, USD(10)));
        env.close();

I see a lot of changes where the loan broker deposits and withdraws, but I would like to see a simple payment from the issuer here in testIsFrozenDirectly

[vvysokikh1]

should these tests be part of testGlobalFreeze in this same file?

Also, I wonder why those tests did not catch this obvious fault?

[shawnxie999]

why? isn't the issuer always allowed to transfer their own asset regardless if the asset is frozen or not?

[shawnxie999]

i'm not sure about these lending specific freeze rules. What's the rationale for this approach compared to the old one, where we added the check directly in the existing freeze checking helper (this seemed to be more elegant and simpler).

also, IIRC, freeze check should always exempt the issuer, not just for lending. so is having lending specific freeze-helpers really necessary?

[ximinez]

There's no diplomatic way to say this. The changes since my last review
(8011c43..4c8d9b5) are almost entirely the wrong way to go about this.

An not-necessarily-exhaustive list of the problems:

1. None of the new functions in View.h/cpp should have been added.
   • Refer to
     https://xrpl.org/docs/concepts/tokens/fungible-tokens/freezes. We
     should not be changing any of those rules for either existing or
     new transactions. It's also true for pseudo-accounts - even though
     they are not acting on their own, they are still sending directly
     to or from the issuer if the issuer is the one acting via
     a VaultDeposit, or LoanBrokerCoverWithdraw, etc.
   • Instead, the existing corresponding functions should be modified to
     include changes if the SAV or LP amendments are enabled. They
     should not be rewritten, but only add the parameters and checks
     needed to determine if either sender or receiver is the issuer.
   • Modifying the existing functions is not changing the rules - it's
     centralizing them.
2. I don't think any of the Invariant changes are correct either. We're
   not changing the existing freeze rules. Specifically, I don't think
   pseudo-accounts should be treated any differently as far as freezes
   are concerned.
3. In LoanBrokerCoverDeposit:
   • I don't understand why you moved the
     account != sleBroker->at(sfOwner) check to later in the function.
     If the account doesn't have permission, we shouldn't waste time
     reading the Vault object before returning.
   • The change from checkFrozen to isIndividualFrozen skips the
     global freeze check, which is only correct if the account is the
     issuer. If you make the changes above, then it will automatically
     do the right thing.
   • It's probably correct to change the accountSpendable call to
     ignore freeze, because with the changes above, any real freezes
     will be caught before we get to that point.
4. In LoanBrokerCoverWithdraw:
   • Again, moving the account != sleBroker->at(sfOwner) check doesn't
     make sense.
   • If the dstAcct is not the issuer, then you should absolutely be
     checking if the pseudoaccount is frozen. It should not be able to
     send funds to anyone except the issuer.
   • Again, it's probably correct to change the accountSpendable call to
     ignore freeze, because with the changes above, any freezes that
     should cause failure will be caught before we get to that point.
5. LoanSet is fine. It's just part of the project rename.
6. There is no reason to make any changes to the Payment transaction,
   particularly one that takes existing, correct, functionality away
   from the issuer.
7. VaultClawback looks fine.
8. VaultDeposit and VaultWithdraw should go back to using the
   corrected isFrozen (or converted to checkFrozen). But I am
   a little confused why you would need to check if the shares of the
   vault (vaultShare) are frozen, because they are issued by the
   pseudoaccount, which can't change the MPTIssuance, and I don't think
   there's any other mechanism to do that, is there?
   • Though with the updated isFrozen / checkFrozen, VaultWithdraw
     won't need to check if either account is the issuer.

[github-actions]

This PR has conflicts, please resolve them in order for the PR to be reviewed.

[godexsoft]

We recently merged a refactor to develop that enables clang-tidy's readability-identifier-naming. Your branch now has heavy conflicts that are largely mechanical. Below is a workflow that aligns your branch's naming with develop before merging, which should minimize the merge conflicts.

One-time setup

If you don't already have clang-tidy working in your env, on macOS:

brew install llvm@21
# Follow brew's hint to put $(brew --prefix llvm@21)/bin on PATH so run-clang-tidy is found.

Workflow on your branch (before merging develop)

1. Grab the new .clang-tidy from develop without pulling anything else. Sync your fork on GitHub first, then:

git remote -v   # should show 'upstream' among others; if not:
# git remote set-url upstream git@github.com:XRPLF/rippled.git
git fetch upstream
git checkout upstream/develop -- .clang-tidy

2. Reconfigure conan/cmake so compile_commands.json is fresh.

3. Apply renames for the files modified in your PR:

git diff --name-only $(git merge-base HEAD upstream/develop) HEAD \
  | grep -E '\.(cpp|h|hpp|ipp)$' \
  | xargs run-clang-tidy -p build -fix -allow-no-checks
# or -p .build, or whatever your build dir is called

4. Build + test, then commit as a single dedicated commit:

cmake --build build -j8
git commit -am "refactor: Align identifier naming with develop"

5. Now merge develop:

git merge upstream/develop

Extra

Run clang-tidy once more after the merge to catch any stragglers introduced from develop's side:

run-clang-tidy -p build -fix -allow-no-checks src tests
# or -p .build, or whatever your build dir is called