refactor: Rename newSLE to newSle and trim the SLEBase test comment

Match rawSle() and mutableRawSle(). Cut the comment on the entry
static_asserts to one line, as asked in review of #8348.

include/xrpl/ledger/entries/SLEBase.h

@@ -70,13 +70,19 @@ resolveEntry(ReadView const& view, Keylet const& key)
 /**
  * View-parameterized base class for all ledger entries.
  *
- * SLEBase<ReadView>  — read-only:  holds shared_ptr<SLE const> + ReadView const&
- * SLEBase<ApplyView> — writable:   holds shared_ptr<SLE> + ApplyView& + Keylet,
- *                                   plus insert/update/erase operations
+ * SLEBase<ReadView>  — read-only:  holds shared_ptr<SLE const> + ReadView const*
+ * SLEBase<ApplyView> — writable:   holds shared_ptr<SLE> + ApplyView* + Keylet,
+ *                                   plus insertIntoView/updateView/eraseFromView
+ *                                   operations
  *
  * Write-only members are gated by `requires` clauses, providing compile-time
  * guarantees that read-only entries cannot mutate state.
  *
+ * Read-only entries are copyable, movable, and assignable. Writable entries
+ * are move-only (not copyable or copy-assignable): the view is stored as a
+ * pointer so assignment can reseat it, but a writable entry's shared_ptr<SLE>
+ * is meant to have a single writer at a time.
+ *
  * @tparam EntryType the ledger entry type this entry is statically bound to.
  * Derived per-type entries pass their own type (e.g. ltACCOUNT_ROOT); the
  * generic ReadOnlySLE / WritableSLE aliases leave it at ltANY, which opts out
@@ -102,9 +108,15 @@ public:
     using SlePtrType = std::conditional_t<kIsWritable, SLE::pointer, SLE::const_pointer>;
 
     // View reference type: ApplyView& for writable, ReadView const& for
-    // read-only
+    // read-only. Constructors take this; it is what callers spell out.
     using ViewRefType = std::conditional_t<kIsWritable, ApplyView&, ReadView const&>;
 
+    // View pointer type: what the entry actually stores. A pointer, rather
+    // than ViewRefType itself, so the entry stays assignable -- a stored
+    // reference member cannot be reseated, which is what forced the deleted
+    // assignment operators below.
+    using ViewPtrType = std::conditional_t<kIsWritable, ApplyView*, ReadView const*>;
+
     // Non-virtual by design: these entries are parameterized on the view and
     // entry type, never used polymorphically through a base pointer. A vptr
     // would be 8 bytes of pure overhead on a type meant to be as cheap as the
@@ -115,14 +127,22 @@ public:
     // virtual, never delete a derived entry through an SLEBase*.
     ~SLEBase() = default;
 
+    // Read-only entries are freely copyable and movable, including
+    // assignment: the view is stored as a pointer (see ViewPtrType above), so
+    // reseating it on assignment is well defined. Writable entries stay
+    // move-only, matching the shared_ptr<SLE> they wrap: copying one would
+    // leave two entries thinking they each own the only write access to the
+    // same SLE.
     SLEBase(SLEBase const&)
         requires(!kIsWritable)
     = default;
     SLEBase(SLEBase&&) = default;
     SLEBase&
-    operator=(SLEBase const&) = delete;
+    operator=(SLEBase const&)
+        requires(!kIsWritable)
+    = default;
     SLEBase&
-    operator=(SLEBase&&) = delete;
+    operator=(SLEBase&&) = default;
     SLEBase() = delete;
 
     // --- Constructors that adopt/resolve an SLE (public so the ReadOnlySLE /
@@ -134,7 +154,7 @@ public:
      * Constructor for read-only context (adopt an already-fetched SLE).
      *
      * There is deliberately no writable equivalent: a writable entry needs
-     * a Keylet so that newSLE() can still build an entry when none exists,
+     * a Keylet so that newSle() can still build an entry when none exists,
      * and that cannot be recovered from a null SLE.
      */
     explicit SLEBase(
@@ -142,7 +162,7 @@ public:
         ViewRefType view,
         beast::Journal j = beast::Journal{beast::Journal::getNullSink()})
         requires(!kIsWritable)
-        : view_(view), sle_(std::move(sle)), j_(j)
+        : view_(&view), sle_(std::move(sle)), j_(j)
     {
         XRPL_ASSERT(
             !kIsTyped || !sle_ || sle_->getType() == kEntryType,
@@ -157,7 +177,7 @@ public:
         ViewRefType view,
         beast::Journal j = beast::Journal{beast::Journal::getNullSink()})
         requires(!kIsWritable)
-        : view_(view), sle_(detail::resolveEntry(view, key)), j_(j)
+        : view_(&view), sle_(detail::resolveEntry(view, key)), j_(j)
     {
         XRPL_ASSERT(
             !kIsTyped || key.type == kEntryType,
@@ -181,7 +201,7 @@ public:
     SLEBase(SLEBase<OtherViewT, OtherType> const& other)
         requires(!kIsWritable && IsWritableView<OtherViewT> &&
                  (OtherType == EntryType || EntryType == ltANY))
-        : view_(other.readView()), sle_(other.rawSle()), j_(other.journal())
+        : view_(&other.readView()), sle_(other.rawSle()), j_(other.journal())
     {
     }
 
@@ -193,7 +213,7 @@ public:
         ApplyView& view,
         beast::Journal j = beast::Journal{beast::Journal::getNullSink()})
         requires kIsWritable
-        : view_(view), key_(key), sle_(view_.peek(key)), j_(j)
+        : view_(&view), key_(key), sle_(view_->peek(key)), j_(j)
     {
         XRPL_ASSERT(
             !kIsTyped || key.type == kEntryType,
@@ -279,7 +299,7 @@ public:
      * Returns the keylet identifying this entry.
      *
      * Writable entries keep the keylet they were built from, so it is valid
-     * even before newSLE(). Read-only entries derive it from the SLE, which
+     * even before newSle(). Read-only entries derive it from the SLE, which
      * must therefore exist.
      *
      * @throws std::logic_error for a read-only entry if exists() is false.
@@ -321,7 +341,7 @@ public:
     [[nodiscard]] ReadView const&
     readView() const
     {
-        return view_;
+        return *view_;
     }
 
     /**
@@ -332,17 +352,13 @@ public:
     STLedgerEntry const*
     operator->() const
     {
-        if (!exists())
-            Throw<std::logic_error>("xrpl::SLEBase::operator-> : entry does not exist");
-        return sle_.get();
+        return &sleRef();
     }
 
     STLedgerEntry const&
     operator*() const
     {
-        if (!exists())
-            Throw<std::logic_error>("xrpl::SLEBase::operator* : entry does not exist");
-        return *sle_;
+        return sleRef();
     }
 
     // --- Writable interface (compile-time gated) ---
@@ -371,7 +387,7 @@ public:
     applyView()
         requires kIsWritable
     {
-        return view_;
+        return *view_;
     }
 
     /**
@@ -383,18 +399,14 @@ public:
     operator->()
         requires kIsWritable
     {
-        if (!exists())
-            Throw<std::logic_error>("xrpl::SLEBase::operator-> : entry does not exist");
-        return sle_.get();
+        return &sleRef();
     }
 
     STLedgerEntry&
     operator*()
         requires kIsWritable
     {
-        if (!exists())
-            Throw<std::logic_error>("xrpl::SLEBase::operator* : entry does not exist");
-        return *sle_;
+        return sleRef();
     }
 
     /**
@@ -403,12 +415,12 @@ public:
      * @throws std::logic_error if exists() is false.
      */
     void
-    insert()
+    insertIntoView()
         requires kIsWritable
     {
         if (!exists())
-            Throw<std::logic_error>("xrpl::SLEBase::insert : entry does not exist");
-        view_.insert(sle_);
+            Throw<std::logic_error>("xrpl::SLEBase::insertIntoView : entry does not exist");
+        view_->insert(sle_);
     }
 
     /**
@@ -419,18 +431,18 @@ public:
      * ApplyStateTable or -- worse -- silently succeeding. For an
      * entry that already existed, ApplyStateTable::erase keeps holding this
      * exact SLE and builds the DeletedNode's FinalFields from it, so a write
-     * through the entry after erase() would land in transaction metadata
-     * with no diagnostic at all.
+     * through the entry after eraseFromView() would land in transaction
+     * metadata with no diagnostic at all.
      *
      * @throws std::logic_error if exists() is false.
      */
     void
-    erase()
+    eraseFromView()
         requires kIsWritable
     {
         if (!exists())
-            Throw<std::logic_error>("xrpl::SLEBase::erase : entry does not exist");
-        view_.erase(sle_);
+            Throw<std::logic_error>("xrpl::SLEBase::eraseFromView : entry does not exist");
+        view_->erase(sle_);
         sle_ = nullptr;
     }
 
@@ -438,24 +450,24 @@ public:
      * @throws std::logic_error if exists() is false.
      */
     void
-    update()
+    updateView()
         requires kIsWritable
     {
         if (!exists())
-            Throw<std::logic_error>("xrpl::SLEBase::update : entry does not exist");
-        view_.update(sle_);
+            Throw<std::logic_error>("xrpl::SLEBase::updateView : entry does not exist");
+        view_->update(sle_);
     }
 
     /**
-     * @throws std::logic_error if exists() is true: newSLE() would otherwise
+     * @throws std::logic_error if exists() is true: newSle() would otherwise
      *         silently discard the SLE already held.
      */
     void
-    newSLE()
+    newSle()
         requires kIsWritable
     {
         if (exists())
-            Throw<std::logic_error>("xrpl::SLEBase::newSLE : entry already exists");
+            Throw<std::logic_error>("xrpl::SLEBase::newSle : entry already exists");
         sle_ = std::make_shared<SLE>(key_);
     }
 
@@ -466,7 +478,44 @@ public:
     }
 
 protected:
-    ViewRefType view_;
+    /**
+     * Returns the underlying SLE, for derived entries to call member
+     * functions on without spelling out `(*this)->`:
+     * `this->sleRef().getFieldX(...)` reads like an ordinary method call.
+     * operator-> and operator* are implemented on top of this, so the
+     * "does not exist" throw lives in one place.
+     *
+     * @return The underlying SLE.
+     * @throws std::logic_error if exists() is false.
+     */
+    [[nodiscard]] SLE const&
+    sleRef() const
+    {
+        if (!exists())
+            Throw<std::logic_error>("xrpl::SLEBase::sleRef : entry does not exist");
+        return *sle_;
+    }
+
+    /**
+     * Returns the underlying SLE for write access.
+     *
+     * @return The underlying SLE.
+     * @throws std::logic_error if exists() is false.
+     */
+    [[nodiscard]] SLE&
+    sleRef()
+        requires kIsWritable
+    {
+        if (!exists())
+            Throw<std::logic_error>("xrpl::SLEBase::sleRef : entry does not exist");
+        return *sle_;
+    }
+
+    // Stored as a pointer, not ViewRefType, so the entry stays assignable --
+    // see the assignment operators above. Declared first: the writable
+    // constructor initializes sle_ from view_->peek(key), so view_ must be
+    // constructed before it.
+    ViewPtrType view_;
 
     // Keylet is only meaningful for writable views, which need it to build an
     // SLE that does not exist yet; read-only entries derive it from the SLE.

src/libxrpl/tx/transactors/check/CheckCancel.cpp

@@ -101,7 +101,7 @@ CheckCancel::doApply()
     decreaseOwnerCountForObject(view(), srcId, sleCheck.mutableRawSle(), 1, viewJ);
 
     // Remove check from ledger.
-    sleCheck.erase();
+    sleCheck.eraseFromView();
     return tesSUCCESS;
 }
 

src/libxrpl/tx/transactors/check/CheckCash.cpp

@@ -634,7 +634,7 @@ CheckCash::doApply()
     decreaseOwnerCountForObject(psb, srcId, sleCheck->mutableRawSle(), 1, viewJ);
 
     // Remove check from ledger.
-    sleCheck->erase();
+    sleCheck->eraseFromView();
 
     psb.apply(ctx_.rawView());
     return tesSUCCESS;

src/libxrpl/tx/transactors/check/CheckCreate.cpp

@@ -203,7 +203,7 @@ CheckCreate::doApply()
     auto const seq = ctx_.tx.getSeqProxy();
     CheckEntryW sleCheck(accountID_, seq, view(), j_);
     Keylet const checkKeylet = sleCheck.keylet();
-    sleCheck.newSLE();
+    sleCheck.newSle();
 
     sleCheck->setAccountID(sfAccount, accountID_);
     AccountID const dstAccountId = ctx_.tx[sfDestination];
@@ -219,7 +219,7 @@ CheckCreate::doApply()
     if (auto const expiry = ctx_.tx[~sfExpiration])
         sleCheck->setFieldU32(sfExpiration, *expiry);
 
-    sleCheck.insert();
+    sleCheck.insertIntoView();
 
     auto const viewJ = ctx_.registry.get().getJournal("View");
     // If it's not a self-send (and it shouldn't be), add Check to the

src/libxrpl/tx/transactors/escrow/EscrowCancel.cpp

@@ -163,7 +163,7 @@ EscrowCancel::doApply()
         decreaseOwnerCountForObject(ctx_.view(), sle, escrow.mutableRawSle(), 1, ctx_.journal);
 
     // Remove escrow from ledger
-    escrow.erase();
+    escrow.eraseFromView();
 
     return tesSUCCESS;
 }

src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp

@@ -280,7 +280,7 @@ EscrowCreate::doApply()
     // sequence or ticket.  For more explanation see comments in SeqProxy.h.
     EscrowEntryW escrow(accountID_, ctx_.tx.getSeqProxy(), ctx_.view(), j_);
     Keylet const escrowKeylet = escrow.keylet();
-    escrow.newSLE();
+    escrow.newSle();
     (*escrow)[sfAmount] = amount;
     (*escrow)[sfAccount] = accountID_;
     (*escrow)[~sfCondition] = ctx_.tx[~sfCondition];
@@ -302,7 +302,7 @@ EscrowCreate::doApply()
             (*escrow)[sfTransferRate] = xferRate.value;
     }
 
-    escrow.insert();
+    escrow.insertIntoView();
 
     // Add escrow to sender's owner directory
     {

src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp

@@ -342,7 +342,7 @@ EscrowFinish::doApply()
         decreaseOwnerCountForObject(ctx_.view(), account, escrow.mutableRawSle(), 1, ctx_.journal);
 
     // Remove escrow from ledger
-    escrow.erase();
+    escrow.eraseFromView();
     return tesSUCCESS;
 }
 

src/libxrpl/tx/transactors/system/Change.cpp

@@ -173,8 +173,8 @@ Change::applyAmendment()
 
     if (!amendmentObject)
     {
-        amendmentObject.newSLE();
-        amendmentObject.insert();
+        amendmentObject.newSle();
+        amendmentObject.insertIntoView();
     }
 
     STVector256 amendments = amendmentObject->getFieldV256(sfAmendments);
@@ -251,7 +251,7 @@ Change::applyAmendment()
         amendmentObject->setFieldArray(sfMajorities, newMajorities);
     }
 
-    amendmentObject.update();
+    amendmentObject.updateView();
 
     return tesSUCCESS;
 }
@@ -263,8 +263,8 @@ Change::applyFee()
 
     if (!feeObject)
     {
-        feeObject.newSLE();
-        feeObject.insert();
+        feeObject.newSle();
+        feeObject.insertIntoView();
     }
     auto const set = [](FeeSettingsEntryW& feeObject, STTx const& tx, auto const& field) {
         feeObject->at(field) = tx[field];
@@ -288,7 +288,7 @@ Change::applyFee()
         set(feeObject, ctx_.tx, sfReserveIncrement);
     }
 
-    feeObject.update();
+    feeObject.updateView();
 
     JLOG(j_.warn()) << "Fees have been changed";
     return tesSUCCESS;
@@ -332,8 +332,8 @@ Change::applyUNLModify()
     NegativeUNLEntryW negUnlObject(view(), j_);
     if (!negUnlObject)
     {
-        negUnlObject.newSLE();
-        negUnlObject.insert();
+        negUnlObject.newSle();
+        negUnlObject.insertIntoView();
     }
 
     bool const found = [&] {
@@ -406,7 +406,7 @@ Change::applyUNLModify()
         negUnlObject->setFieldVL(sfValidatorToReEnable, validator);
     }
 
-    negUnlObject.update();
+    negUnlObject.updateView();
     return tesSUCCESS;
 }
 

src/tests/libxrpl/ledger/SLEBase.cpp

@@ -77,14 +77,7 @@ template class SLEBase<ApplyView>;
 #pragma push_macro("LEDGER_ENTRY")
 #undef LEDGER_ENTRY
 
-// No explicit instantiation here, neither a definition nor a declaration.
-// A definition would instantiate an entry a second time once its member
-// bodies live in its own .cpp (MSVC rejects that with C4661 under /WX), and a
-// declaration (`extern template`) stops clang from emitting the inline
-// constructors of the header-only entries at -O0, which leaves them undefined
-// at link time in the debug builds. Naming the class in a static_assert is
-// enough to instantiate its definition, which is all this exhaustiveness
-// check needs; the per-type suites exercise the members.
+// A static_assert naming each class instantiates its definition (see #8357).
 #define LEDGER_ENTRY(tag, value, name, ...)                                                    \
     static_assert(                                                                             \
         name##Entry<ReadView>::kEntryType == tag && name##Entry<ApplyView>::kEntryType == tag, \
@@ -277,7 +270,7 @@ TEST_F(SLEBaseTests, writable_lifecycle)
     // A view we never apply, so nothing here reaches the ledger.
     ApplyViewImpl av(&env_.getClosedLedger(), TapNone);
 
-    // Entry that does not exist yet: newSLE() -> insert().
+    // Entry that does not exist yet: newSle() -> insertIntoView().
     {
         TicketEntryW ticket(keylet::ticket(alice_.id(), SeqProxy::rawTicket(1)), av);
         EXPECT_FALSE(ticket.exists());
@@ -285,20 +278,20 @@ TEST_F(SLEBaseTests, writable_lifecycle)
         EXPECT_EQ(ticket.type(), ltTICKET);
         EXPECT_EQ(ticket.keylet().type, ltTICKET);
 
-        ticket.newSLE();
+        ticket.newSle();
         EXPECT_TRUE(ticket.exists());
-        ticket.insert();
-        ticket.update();
+        ticket.insertIntoView();
+        ticket.updateView();
 
         // Erasing an entry inserted in this same view drops it outright.
-        ticket.erase();
+        ticket.eraseFromView();
         EXPECT_FALSE(ticket.exists());
     }
 
-    // Entry that already exists: update() is what promotes it from a bare
+    // Entry that already exists: updateView() is what promotes it from a bare
     // peek to a real change. ApplyViewImpl::size() counts Insert, Modify and
     // Erase but not Cache, so it shows the difference: building the entry
-    // only peeks, and the write is invisible to the view until update().
+    // only peeks, and the write is invisible to the view until updateView().
     {
         ApplyViewImpl fresh(&env_.getClosedLedger(), TapNone);
 
@@ -309,11 +302,11 @@ TEST_F(SLEBaseTests, writable_lifecycle)
         account->setFieldU32(sfSequence, account->getFieldU32(sfSequence) + 1);
         EXPECT_EQ(fresh.size(), 0);
 
-        account.update();
+        account.updateView();
         EXPECT_EQ(fresh.size(), 1);
 
-        // update() is idempotent: the entry is already a Modify.
-        account.update();
+        // updateView() is idempotent: the entry is already a Modify.
+        account.updateView();
         EXPECT_EQ(fresh.size(), 1);
     }
 
@@ -325,7 +318,7 @@ TEST_F(SLEBaseTests, writable_lifecycle)
         AccountRootEntryW account(alice_.id(), av);
         EXPECT_TRUE(account.exists());
 
-        account.erase();
+        account.eraseFromView();
         EXPECT_FALSE(account.exists());
     }
 }
@@ -415,25 +408,25 @@ TEST_F(SLEBaseTests, throws_on_missing_writable_entry)
 
     EXPECT_THROW(std::ignore = missing.operator->(), std::logic_error);
     EXPECT_THROW(std::ignore = (*missing).getType(), std::logic_error);
-    EXPECT_THROW(missing.insert(), std::logic_error);
-    EXPECT_THROW(missing.update(), std::logic_error);
-    EXPECT_THROW(missing.erase(), std::logic_error);
+    EXPECT_THROW(missing.insertIntoView(), std::logic_error);
+    EXPECT_THROW(missing.updateView(), std::logic_error);
+    EXPECT_THROW(missing.eraseFromView(), std::logic_error);
 
     // keylet() and key() are the exception: a writable entry keeps the keylet
-    // it was built from, so they stay valid before newSLE().
+    // it was built from, so they stay valid before newSle().
     EXPECT_EQ(missing.key(), keylet::account(bob_.id()).key);
 
-    // newSLE() is the inverse -- it throws when the entry *does* exist,
+    // newSle() is the inverse -- it throws when the entry *does* exist,
     // rather than silently dropping the SLE already held.
-    missing.newSLE();
+    missing.newSle();
     EXPECT_TRUE(missing.exists());
-    EXPECT_THROW(missing.newSLE(), std::logic_error);
+    EXPECT_THROW(missing.newSle(), std::logic_error);
 
     // And once erased, the entry is empty again and throws as before.
-    missing.insert();
-    missing.erase();
+    missing.insertIntoView();
+    missing.eraseFromView();
     EXPECT_FALSE(missing.exists());
-    EXPECT_THROW(missing.update(), std::logic_error);
+    EXPECT_THROW(missing.updateView(), std::logic_error);
 }
 
 }  // namespace test