Thoughts on the interaction of STAmount and STNumber with expanded mantissas (an offshoot of #6025)

[ximinez]

Description

Before Lending Protocol (#5270) was merged in to develop, I considered DRAFT: Expand Number to support the full integer range #6025 to be more-or-less done.

Integrating the two together has been more "interesting" than I expected. There have been a few hard-coded test case values that need to be updated, but those aren't a big deal.

The challenge has come in one particular check:

ripple.tx.Loan Lifecycle: 1000 IOU Scale interest to: 0 Loan overpayment allowed - Impair and Default
#233 failed: Loan_test.cpp(312)

The test on line 312 is:

                    env.test.BEAST_EXPECT(
                        vaultSle->at(sfAssetsAvailable) ==
                        env.balance(vaultPseudo, broker.asset).number());

This is checking that "Assets Available" in the Vault is the same as the actual balance of the Vault pseudo-account for that asset. In this case, the values are:

• Assets Available, which is an STNumber: 999025.0003706351120
• Pseudo-account balance, which is an STAmount: 999025.0003706351000

The values are identical to 16 digits of precision. STAmount is designed to have 16 digits of precision for IOU values. STNumber, however, has been expanded to 19 digits of precision for all values as part of #6025.

tl;dr

STNumber is too precise to represent IOU values.

Solutions

I can think of two general solutions to this problem:

Solution 1: Expand STAmount, too

Basically apply the same STNumber expansion to STAmount.

There are several reasons not to do this, the biggest of which is UGH.

Also:

1. STAmount is far more pervasive across the system. Messing with it has a much higher level of risk than messing with STNumber.
2. The serialization format for STAmount uses some of the bits of the 64-bit mantissa to encode extra information for XRP and IOU values. Storing the entire 64-bit number would require redesigning the serialization at least for those types. This is a very complex and risky task, which would also require backward-compatibility for existing ledger values. (There are no STNumbers on ledger.)
3. The serialized STAmount would also be bigger, requiring more resources across the entire network. There are a lot of STNumbers STAmounts on the mainnet ledger.

Solution 2: A bit of a redesign

I propose these changes to the design of the Vault, LoanBroker, and Loan objects and transactions.

1. I mentioned above that Assets Available is supposed to match the pseudo-account balance. If they're required to always stay in sync, then one is redundant. Because balances are needed system-wide, get rid of sfAssetsAvailable.
   • As far as I can tell, it serves no functional purpose, except maybe the convenience of skipping a lookup to get the pseudo-account balance.
   • Most transactions that are going to be interacting with sfAssetsAvailable are going to be using the real balance in some way anyway, and loading the balance for those that don't is only going to require loading one more object, a relatively small cost.
   • The field will no longer be available to RPC queries, but any such queries where it might be relevant could return the balance as a composite field.
2. Conceptually, an STAmount is an STNumber combined with an STIssue. The Vault object has the sfAsset field, which is an STIssue, and the sfAssetsTotal field, which is an STNumber. Combine STIssue sfAsset with STNumber sfAssetsTotal into a single STAmount sfAssetsTotal field in the Vault object.
   • This will give us the desired rounding automatically.
   • Accessing a Vault's asset type would be a simple change from sle[sfAssets] to sle[sfAssetsTotal].asset().
   • There would be no undesired side effects from changing sfAssetsTotal because it is only defined in the Vault object, no transaction layouts, and only referenced by Vault and Loan transactors - which are still behind amendment gates.
3. That leaves about 15 STNumber fields, all of which are only used by Vault, LoanBrokers, and Loans. Any that are part of a Loan have already been thoroughly vetted for rounding safety and agreement with the Vault. Audit STNumber fields that represent "real" asset amounts, and convert them into and back out of an STAmount any time they are stored or altered.
   • That will force them to be rounded appropriately for IOUs, MPTs, and XRP if they aren't already so that they fit into the expected accuracy.
   • Looking at the fields list, I think the only ones that will need any attention are sfLossUnrealized, sfCoverAvailable, sfDebtTotal, and maybe sfAssetsMaximum.
   • Absolutely do not mess with sfPeriodicPayment.

Feedback

I'm putting this proposal out before implementing it so I can get feedback from interested parties. Is there anything else I should consider? Is there a use for sfAssetsAvailable that I don't know about, or a need for it to be able to differ from the pseudo-account balance? Do you have any better or simpler ideas? I'd love to hear them.

[Tapanito]

Thanks for putting this proposal together, @ximinez.

Regarding sfAssetsAvailable: Removing this seems fine for now since it currently just mirrors the pseudo-account balance. However, this does limit the Vault's future flexibility—specifically, it won't be able to hold the same asset for different purposes (like the original design sharing the pseudo-account for depositor funds and first-loss capital).

That said, we can work around this in the future (e.g., for Lending 2.0) by assigning a distinct pseudo-account for each tracked value rather than sharing one.

Regarding Precision: We need to make sure we address DebtTotal, UnrealizedLoss, and CoverAvailable. They will suffer from the same precision mismatch. In a worst-case scenario where AssetsAvailable is zero, DebtTotal, UnrealizedLoss, and AssetsTotal could technically be equal but still fail checks due to the digit difference.

[gregtatcam]

Solution 2, item 1 - "This will give us the desired rounding automatically.". Aren't we going to loose precision by moving SFAssetsTotal from STNumber to STAmount?

[ximinez]

Thanks for putting this proposal together, @ximinez.

YW!

That said, we can work around this in the future (e.g., for Lending 2.0) by assigning a distinct pseudo-account for each tracked value rather than sharing one.

Pseudo-accounts are way more expensive than a couple of STNumber fields, so given the choice I'd rather keep AssetsAvailable, and keep it rounded off.

Alternatively, I could get rid of AssetsAvailable now, and we could add it back later if we need it for future tracking. By then, we should be more in the habit of rounding properly. I like this option a little less because adding it back may require adding back a bunch of tracking and unit tests that are already written and done.

Regarding Precision: We need to make sure we address DebtTotal, UnrealizedLoss, and CoverAvailable. They will suffer from the same precision mismatch. In a worst-case scenario where AssetsAvailable is zero, DebtTotal, UnrealizedLoss, and AssetsTotal could technically be equal but still fail checks due to the digit difference.

Agreed. That was step 3 of proposed solution 2.

[vlntb]

If I had to pick between Solution 1 and Solution 2, I’d still lean towards 2: it feels safer and faster to ship, while 1 is more of a long-term, fundamental change to how the system works.

A lighter-weight variant of Solution 2 (instead of touching all ~15 STNumber fields) would be to add small conversion helpers and use them only where the conversion is actually needed. Smth like:

STAmount toAmount(STNumber const& n, Issue const& issue);
STNumber toNumber(STAmount const& a);

This way we don’t need to expand STAmount globally (Solution 1), and we also don’t have to immediately redesign every field in Vault/Loan – we just enforce STAmount-level precision at the boundaries where a STNumber is meant to represent “real” on-ledger value.

[ximinez]

Solution 2, item 1 - "This will give us the desired rounding automatically.". Aren't we going to loose precision by moving SFAssetsTotal from STNumber to STAmount?

No and yes.

No, in that we're essentially keeping the status quo on what we have now for IOUs while improving it for XRP and MPTs. Right now, Numbers have the same precision as STAmounts representing IOUs. After this change, some numbers (those representing IOUs) will still have the same precision as an IOU STAmount. However, XRP and MPT values will gain precision - the precision that STAmount currently has for those types.

Yes, in that we're adding precision to the class, and intermediate values will use that precision, but we'll round back down for IOUs before storing them.

Overall, since any Numbers that represent IOUs need to be rounded off to match the IOUs, I think it's worth it.

[Tapanito]

Agreed. That was step 3 of proposed solution 2.

Since we have to address precision difference for LoanBroker anyway, does it make a big difference if we address it by removing assetsAvailable in the Vault?

Pseudo-accounts are way more expensive than a couple of STNumber fields, so given the choice I'd rather keep AssetsAvailable, and keep it rounded off.

Alternatively, I could get rid of AssetsAvailable now, and we could add it back later if we need it for future tracking. By then, we should be more in the habit of rounding property. I like this option a little less because adding it back may require adding back a bunch of tracking and unit tests that are already written and done.

Another impact, is that removing this field will break all QE and performance tests. I'm not against it, but I'm not a big fan of removing this field.

[ximinez]

If I had to pick between Solution 1 and Solution 2, I’d still lean towards 2: it feels safer and faster to ship, while 1 is more of a long-term, fundamental change to how the system works.

👍

A lighter-weight variant of Solution 2 (instead of touching all ~15 STNumber fields) would be to add small conversion helpers and use them only where the conversion is actually needed. Smth like:

STAmount toAmount(STNumber const& n, Issue const& issue);
STNumber toNumber(STAmount const& a);

Fortunately, those conversions basically already exist.
STAmount has a ctor that takes an Assetish value and a Number:

    template <AssetType A>
    STAmount(A const& asset, Number const& number)

It also has an automatic conversion to Number:

    operator Number() const;

In fact, I was thinking that the conversions would look something like:

auto someNumberFieldProxy = sle->at(sfSomeNumberField);
auto const asset = vault->at(sfAsset);
[... do some stuff ...]
someNumberFieldProxy = STAmount{asset, someNumberFieldProxy);

Though I'd probably put that line in a helper function that does the rounding in place.

void roundToAsset(Asset const& asset, Number& number);

(There's already a roundToAsset, so maybe a different name, though the different signatures differentiate them pretty well.)

This way we don’t need to expand STAmount globally (Solution 1), and we also don’t have to immediately redesign every field in Vault/Loan – we just enforce STAmount-level precision at the boundaries where a STNumber is meant to represent “real” on-ledger value.

👍

[ximinez]

Since we have to address precision difference for LoanBroker anyway, does it make a big difference if we address it by removing assetsAvailable in the Vault?

No.

Another impact, is that removing this field will break all QE and performance tests. I'm not against it, but I'm not a big fan of removing this field.

That's a really good point. Now that you mention it, changing the type of stAssetsTotal would cause problems there, too. If this was toward the beginning of the project, it would be a viable path, but given the time constraints, I wouldn't want to make extra work for another team, too.

[ximinez]

@gregtatcam @Tapanito @vlntb Thanks so much for your feedback!!!

The way I'm going to proceed for right now is to go with Solution 2, but completely skip steps 1 and 2. Those were cool ideas, but they're too risky, and have too many downstream consequences, to change at this late stage.

I'll proceed with step 3: Audit STNumber fields that represent "real" asset amounts, and convert them into and back out of an STAmount any time they are stored or altered.

[Tapanito]

@ximinez can we close this issue?