Code cleanup

Author: Bronek

include/xrpl/protocol/Protocol.h

@@ -124,7 +124,7 @@ std::uint8_t constexpr vaultStrategyFirstComeFirstServe = 1;
 
 /** Maximum recursion depth for vault shares being put as an asset inside
  * another vault; counted from 0 */
-std::uint8_t constexpr maxFreezeCheckDepth = 5;
+std::uint8_t constexpr maxAssetCheckDepth = 5;
 
 /** A ledger index. */
 using LedgerIndex = std::uint32_t;

include/xrpl/protocol/STBase.h

@@ -93,7 +93,9 @@ struct JsonOptions
 };
 
 template <typename T>
-    requires requires(T const& t) { t.getJson(JsonOptions::none); }
+    requires requires(T const& t) {
+        { t.getJson(JsonOptions::none) } -> std::convertible_to<Json::Value>;
+    }
 Json::Value
 to_json(T const& t)
 {

include/xrpl/protocol/UintTypes.h

@@ -61,8 +61,7 @@ using NodeID = base_uint<160, detail::NodeIDTag>;
 /** MPTID is a 192-bit value representing MPT Issuance ID,
  * which is a concatenation of a 32-bit sequence (big endian)
  * and a 160-bit account */
-// TODO - edhennis - Add a tag
-using MPTID = uint192;
+using MPTID = base_uint<192>;
 
 /** XRP currency. */
 Currency const&

include/xrpl/protocol/detail/sfields.macro

@@ -197,9 +197,9 @@ TYPED_SFIELD(sfVaultID,                  UINT256,   35)
 
 // number (common)
 TYPED_SFIELD(sfNumber,                   NUMBER,     1)
-TYPED_SFIELD(sfAssetsAvailable,           NUMBER,     2)
-TYPED_SFIELD(sfAssetsMaximum,             NUMBER,     3)
-TYPED_SFIELD(sfAssetsTotal,               NUMBER,     4)
+TYPED_SFIELD(sfAssetsAvailable,          NUMBER,     2)
+TYPED_SFIELD(sfAssetsMaximum,            NUMBER,     3)
+TYPED_SFIELD(sfAssetsTotal,              NUMBER,     4)
 TYPED_SFIELD(sfLossUnrealized,           NUMBER,     5)
 
 // currency amount (common)

include/xrpl/protocol/jss.h

@@ -89,7 +89,6 @@ JSS(SettleDelay);                        // in: TransactionSign
 JSS(SendMax);                            // in: TransactionSign
 JSS(Sequence);                           // in/out: TransactionSign; field.
 JSS(SetFlag);                            // field.
-JSS(SharesTotal);                        // out: Vault
 JSS(Signer);                             // field.
 JSS(Signers);                            // field.
 JSS(SigningPubKey);                      // field.
@@ -101,7 +100,6 @@ JSS(TransactionType);                    // in: TransactionSign.
 JSS(TransferRate);                       // in: TransferRate.
 JSS(TxnSignature);                       // field.
 JSS(URI);                                // field.
-JSS(VaultID);                            // field.
 JSS(VoteSlots);                          // out: AMM Vote
 JSS(aborted);                            // out: InboundLedger
 JSS(accepted);               // out: LedgerToJson, OwnerInfo, SubmitTransaction

src/libxrpl/protocol/STLedgerEntry.cpp

@@ -31,7 +31,6 @@
 #include <xrpl/protocol/SField.h>
 #include <xrpl/protocol/STBase.h>
 #include <xrpl/protocol/STLedgerEntry.h>
-#include <xrpl/protocol/STNumber.h>
 #include <xrpl/protocol/STObject.h>
 #include <xrpl/protocol/Serializer.h>
 #include <xrpl/protocol/jss.h>

src/test/jtx/impl/vault.cpp

@@ -50,7 +50,7 @@ Vault::set(SetArgs const& args)
     Json::Value jv;
     jv[jss::TransactionType] = jss::VaultSet;
     jv[jss::Account] = args.owner.human();
-    jv[jss::VaultID] = to_string(args.id);
+    jv[sfVaultID] = to_string(args.id);
     return jv;
 }
 
@@ -60,7 +60,7 @@ Vault::del(DeleteArgs const& args)
     Json::Value jv;
     jv[jss::TransactionType] = jss::VaultDelete;
     jv[jss::Account] = args.owner.human();
-    jv[jss::VaultID] = to_string(args.id);
+    jv[sfVaultID] = to_string(args.id);
     return jv;
 }
 
@@ -70,7 +70,7 @@ Vault::deposit(DepositArgs const& args)
     Json::Value jv;
     jv[jss::TransactionType] = jss::VaultDeposit;
     jv[jss::Account] = args.depositor.human();
-    jv[jss::VaultID] = to_string(args.id);
+    jv[sfVaultID] = to_string(args.id);
     jv[jss::Amount] = to_json(args.amount);
     return jv;
 }
@@ -81,7 +81,7 @@ Vault::withdraw(WithdrawArgs const& args)
     Json::Value jv;
     jv[jss::TransactionType] = jss::VaultWithdraw;
     jv[jss::Account] = args.depositor.human();
-    jv[jss::VaultID] = to_string(args.id);
+    jv[sfVaultID] = to_string(args.id);
     jv[jss::Amount] = to_json(args.amount);
     return jv;
 }
@@ -92,7 +92,7 @@ Vault::clawback(ClawbackArgs const& args)
     Json::Value jv;
     jv[jss::TransactionType] = jss::VaultClawback;
     jv[jss::Account] = args.issuer.human();
-    jv[jss::VaultID] = to_string(args.id);
+    jv[sfVaultID] = to_string(args.id);
     jv[jss::Holder] = args.holder.human();
     if (args.amount)
         jv[jss::Amount] = to_json(*args.amount);

src/xrpld/app/tx/detail/AMMCreate.cpp

@@ -227,8 +227,7 @@ applyCreate(
     auto const ammKeylet = keylet::amm(amount.issue(), amount2.issue());
 
     // Mitigate same account exists possibility
-    auto const maybeAccount =
-        createPseudoAccount(sb, ammKeylet.key, PseudoAccountOwnerType::AMM);
+    auto const maybeAccount = createPseudoAccount(sb, ammKeylet.key, sfAMMID);
     // AMM account already exists (should not happen)
     if (!maybeAccount)
     {

src/xrpld/app/tx/detail/VaultClawback.cpp

@@ -97,7 +97,7 @@ VaultClawback::preclaim(PreclaimContext const& ctx)
             return tecWRONG_ASSET;
 
         std::uint32_t const issueFlags = mptIssue->getFieldU32(sfFlags);
-        if (!(issueFlags & lsfMPTCanClawback) || !(issueFlags & lsfMPTCanLock))
+        if (!(issueFlags & lsfMPTCanClawback))
             return tecNO_PERMISSION;
     }
     else if (asset.holds<Issue>())

src/xrpld/app/tx/detail/VaultCreate.cpp

@@ -56,10 +56,10 @@ VaultCreate::preflight(PreflightContext const& ctx)
             return temMALFORMED;
     }
 
-    if (auto const data = ctx.tx[~sfWithdrawalPolicy])
+    if (auto const withdrawalPolicy = ctx.tx[~sfWithdrawalPolicy])
     {
         // Enforce valid withdrawal policy
-        if (*data != vaultStrategyFirstComeFirstServe)
+        if (*withdrawalPolicy != vaultStrategyFirstComeFirstServe)
             return temMALFORMED;
     }
 
@@ -151,22 +151,21 @@ VaultCreate::doApply()
     // we can consider downgrading them to `tef` or `tem`.
 
     auto const& tx = ctx_.tx;
-    auto const& ownerId = account_;
     auto sequence = tx.getSeqValue();
+    auto owner = view().peek(keylet::account(account_));
+    if (owner == nullptr)
+        return tefINTERNAL;
 
-    auto owner = view().peek(keylet::account(ownerId));
-    auto vault = std::make_shared<SLE>(keylet::vault(ownerId, sequence));
+    auto vault = std::make_shared<SLE>(keylet::vault(account_, sequence));
 
-    if (auto ter = dirLink(view(), ownerId, vault))
+    if (auto ter = dirLink(view(), account_, vault))
         return ter;
-    // Should the next 3 lines be folded into `dirLink`?
     adjustOwnerCount(view(), owner, 1, j_);
     auto ownerCount = owner->at(sfOwnerCount);
     if (mPriorBalance < view().fees().accountReserve(ownerCount))
         return tecINSUFFICIENT_RESERVE;
 
-    auto maybePseudo = createPseudoAccount(
-        view(), vault->key(), PseudoAccountOwnerType::Vault);
+    auto maybePseudo = createPseudoAccount(view(), vault->key(), sfVaultID);
     if (!maybePseudo)
         return maybePseudo.error();
     auto& pseudo = *maybePseudo;
@@ -206,7 +205,7 @@ VaultCreate::doApply()
 
     vault->at(sfFlags) = txFlags & tfVaultPrivate;
     vault->at(sfSequence) = sequence;
-    vault->at(sfOwner) = ownerId;
+    vault->at(sfOwner) = account_;
     vault->at(sfAccount) = pseudoId;
     vault->at(sfAsset) = asset;
     vault->at(sfAssetsTotal) = Number(0);

src/xrpld/app/tx/detail/VaultDeposit.cpp

@@ -83,7 +83,7 @@ VaultDeposit::preclaim(PreclaimContext const& ctx)
     if (isFrozen(ctx.view, account, share))
         return tecLOCKED;
 
-    if ((vault->getFlags() & tfVaultPrivate) && account != vault->at(sfOwner))
+    if (vault->isFlag(tfVaultPrivate) && account != vault->at(sfOwner))
     {
         auto const maybeDomainID = sleIssuance->at(~sfDomainID);
         // Since this is a private vault and the account is not its owner, we
@@ -176,7 +176,7 @@ VaultDeposit::doApply()
         }
 
         // If the vault is private, set the authorized flag for the vault owner
-        if (vault->getFlags() & tfVaultPrivate)
+        if (vault->isFlag(tfVaultPrivate))
         {
             if (auto const err = MPTokenAuthorize::authorize(
                     view(),

src/xrpld/ledger/View.h

@@ -155,6 +155,12 @@ isFrozen(
     MPTIssue const& mptIssue,
     int depth = 0);
 
+/**
+ * isFrozen check is recursive for MPT shares in a vault, descending to assets
+ * in the vault. These assets could be themselves MPT shares in another vault.
+ * For this reason we limit depth of check, up to maxAssetCheckDepth. If this
+ * depth is exceeded, we return true (i.e. the asset is considered frozen).
+ */
 [[nodiscard]] inline bool
 isFrozen(
     ReadView const& view,
@@ -502,14 +508,19 @@ dirLink(ApplyView& view, AccountID const& owner, std::shared_ptr<SLE>& object);
 AccountID
 pseudoAccountAddress(ReadView const& view, uint256 const& pseudoOwnerKey);
 
-// Which of the owner-object fields should we set: sfAMMID, sfVaultID
-enum class PseudoAccountOwnerType : int { AMM, Vault };
-
+/**
+ *
+ * Create pseudo-account, storing pseudoOwnerKey into ownerField.
+ *
+ * The list of valid ownerField is maintained in View.cpp and the caller to
+ * this function must perform necessary amendment check(s) before using a
+ * field. The amendment check is **not** performed in createPseudoAccount.
+ */
 [[nodiscard]] Expected<std::shared_ptr<SLE>, TER>
 createPseudoAccount(
     ApplyView& view,
     uint256 const& pseudoOwnerKey,
-    PseudoAccountOwnerType type);
+    SField const& ownerField);
 
 // Returns true iff sleAcct is a pseudo-account.
 //
@@ -691,12 +702,6 @@ transferXRP(
     STAmount const& amount,
     beast::Journal j);
 
-struct TokenDescriptor
-{
-    std::shared_ptr<SLE const> token;
-    std::shared_ptr<SLE const> issuance;
-};
-
 /** Check if the account lacks required authorization.
  *
  *   Return tecNO_AUTH or tecNO_LINE if it does
@@ -711,6 +716,13 @@ requireAuth(ReadView const& view, Issue const& issue, AccountID const& account);
  *   from preclaim, the user should convert result tecEXPIRED to tesSUCCESS and
  *   proceed to also check permissions with enforceMPTokenAuthorization inside
  *   doApply. This will ensure that any expired credentials are deleted.
+ *
+ *   requireAuth check is recursive for MPT shares in a vault, descending to
+ *   assets in the vault. These assets could be themselves MPT shares in another
+ *   vault. For this reason we limit depth of check, up to maxAssetCheckDepth.
+ *   This function will return tecKILLED if maximum depth is exceeded.
+ *   Transaction VaultCreate checks for tecKILLED to prevent such vaults being
+ *   created.
  */
 [[nodiscard]] TER
 requireAuth(
@@ -728,7 +740,7 @@ requireAuth(
  *   in order for the transactor to proceed to doApply.
  *
  *   This function will create MPToken (if needed) on the basis of any
- *   non-expired credentals and will delete any expired credentials, indirectly
+ *   non-expired credentials and will delete any expired credentials, indirectly
  *   via verifyValidDomain, as per DomainID (if set in MPTokenIssuance).
  *
  *   The caller does NOT need to ensure that DomainID is actually set - this