Improve STTx construction:

* Remove the ability to construct an empty transaction by type, only
  to then have to add fields to it. Instead, offer a constructor that
  accepts a transaction type and a lambda that can insert fields into
  the STTx during construction.
* Remove now obsolete boost::optional transaction ID.

src/ripple/app/misc/AmendmentTable.h

@@ -272,22 +272,25 @@ public:
         // Inject appropriate pseudo-transactions
         for (auto const& it : actions)
         {
-            STTx trans (ttAMENDMENT);
-            trans.setAccountID (sfAccount, AccountID());
-            trans.setFieldH256 (sfAmendment, it.first);
-            trans.setFieldU32 (sfLedgerSequence, lastClosedLedger->seq() + 1);
+            STTx amendTx (ttAMENDMENT,
+                [&it, seq = lastClosedLedger->seq() + 1](auto& obj)
+                {
+                    obj.setAccountID (sfAccount, AccountID());
+                    obj.setFieldH256 (sfAmendment, it.first);
+                    obj.setFieldU32 (sfLedgerSequence, seq);
 
-            if (it.second != 0)
-                trans.setFieldU32 (sfFlags, it.second);
+                    if (it.second != 0)
+                        obj.setFieldU32 (sfFlags, it.second);
+                });
 
             Serializer s;
-            trans.add (s);
+            amendTx.add (s);
 
         #if ! RIPPLE_PROPOSE_AMENDMENTS
             return;
         #endif
 
-            uint256 txID = trans.getTransactionID();
+            uint256 txID = amendTx.getTransactionID();
             auto tItem = std::make_shared <SHAMapItem> (txID, s.peekData());
             initialPosition->addGiveItem (tItem, true, false);
         }

src/ripple/app/misc/FeeVoteImpl.cpp

@@ -201,6 +201,7 @@ FeeVoteImpl::doVoting (Ledger::ref lastClosedLedger,
     std::uint64_t const baseFee = baseFeeVote.getVotes ();
     std::uint32_t const baseReserve = baseReserveVote.getVotes ();
     std::uint32_t const incReserve = incReserveVote.getVotes ();
+    std::uint32_t const feeUnits = target_.reference_fee_units;
 
     // add transactions to our position
     if ((baseFee != lastClosedLedger->fees().base) ||
@@ -212,20 +213,23 @@ FeeVoteImpl::doVoting (Ledger::ref lastClosedLedger,
             "/" << baseReserve <<
             "/" << incReserve;
 
-        STTx trans (ttFEE);
-        trans[sfAccount] = AccountID();
-        trans[sfBaseFee] = baseFee;
-        trans[sfReferenceFeeUnits] = target_.reference_fee_units;
-        trans[sfReserveBase] = baseReserve;
-        trans[sfReserveIncrement] = incReserve;
+        STTx feeTx (ttFEE,
+            [baseFee,baseReserve,incReserve,feeUnits](auto& obj)
+            {
+                obj[sfAccount] = AccountID();
+                obj[sfBaseFee] = baseFee;
+                obj[sfReserveBase] = baseReserve;
+                obj[sfReserveIncrement] = incReserve;
+                obj[sfReferenceFeeUnits] = feeUnits;
+            });
 
-        uint256 txID = trans.getTransactionID ();
+        uint256 txID = feeTx.getTransactionID ();
 
         if (journal_.warning)
             journal_.warning << "Vote: " << txID;
 
         Serializer s;
-        trans.add (s);
+        feeTx.add (s);
 
         auto tItem = std::make_shared<SHAMapItem> (txID, s.peekData ());
 

src/ripple/protocol/STTx.h

@@ -24,7 +24,7 @@
 #include <ripple/protocol/TxFormats.h>
 #include <boost/container/flat_set.hpp>
 #include <boost/logic/tribool.hpp>
-#include <boost/optional.hpp>
+#include <functional>
 
 namespace ripple {
 
@@ -54,10 +54,19 @@ public:
 
     explicit STTx (SerialIter& sit);
     explicit STTx (SerialIter&& sit) : STTx(sit) {}
-    explicit STTx (TxType type);
 
     explicit STTx (STObject&& object);
 
+    /** Constructs a transaction.
+
+        The returned transaction will have the specified type and
+        any fields that the callback function adds to the object
+        that's passed in.
+    */
+    STTx (
+        TxType type,
+        std::function<void(STObject&)> assembler);
+
     STBase*
     copy (std::size_t n, void* buf) const override
     {
@@ -91,7 +100,6 @@ public:
     {
         return getFieldVL (sfSigningPubKey);
     }
-    void setSigningPubKey (const RippleAddress & naSignPubKey);
 
     std::uint32_t getSequence () const
     {
@@ -105,7 +113,10 @@ public:
     boost::container::flat_set<AccountID>
     getMentionedAccounts() const;
 
-    uint256 getTransactionID () const;
+    uint256 getTransactionID () const
+    {
+        return tid_;
+    }
 
     Json::Value getJson (int options) const override;
     Json::Value getJson (int options, bool binary) const;
@@ -132,7 +143,7 @@ private:
     bool checkSingleSign () const;
     bool checkMultiSign () const;
 
-    boost::optional<uint256> tid_;
+    uint256 tid_;
     TxType tx_type_;
 };
 

src/ripple/protocol/impl/STTx.cpp

@@ -35,25 +35,25 @@
 #include <boost/format.hpp>
 #include <array>
 #include <memory>
+#include <type_traits>
 #include <utility>
 
 namespace ripple {
 
-STTx::STTx (TxType type)
-    : STObject (sfTransaction)
-    , tx_type_ (type)
+static
+auto getTxFormat (TxType type)
 {
     auto format = TxFormats::getInstance().findByType (type);
 
     if (format == nullptr)
     {
-        WriteLog (lsWARNING, STTx) <<
-            "Transaction type: " << type;
-        Throw<std::runtime_error> ("invalid transaction type");
+        Throw<std::runtime_error> (
+            "Invalid transaction type " +
+            std::to_string (
+                static_cast<std::underlying_type_t<TxType>>(type)));
     }
 
-    set (format->elements);
-    setFieldU16 (sfTransactionType, format->getType ());
+    return format;
 }
 
 STTx::STTx (STObject&& object)
@@ -61,21 +61,8 @@ STTx::STTx (STObject&& object)
 {
     tx_type_ = static_cast <TxType> (getFieldU16 (sfTransactionType));
 
-    auto format = TxFormats::getInstance().findByType (tx_type_);
-
-    if (!format)
-    {
-        WriteLog (lsWARNING, STTx) <<
-            "Transaction type: " << tx_type_;
-        Throw<std::runtime_error> ("invalid transaction type");
-    }
-
-    if (!setType (format->elements))
-    {
-        WriteLog (lsWARNING, STTx) <<
-            "Transaction not legal for format";
+    if (!setType (getTxFormat (tx_type_)->elements))
         Throw<std::runtime_error> ("transaction not valid");
-    }
 
     tid_ = getHash(HashPrefix::transactionID);
 }
@@ -86,30 +73,33 @@ STTx::STTx (SerialIter& sit)
     int length = sit.getBytesLeft ();
 
     if ((length < Protocol::txMinSizeBytes) || (length > Protocol::txMaxSizeBytes))
-    {
-        WriteLog (lsERROR, STTx) <<
-            "Transaction has invalid length: " << length;
         Throw<std::runtime_error> ("Transaction length invalid");
-    }
 
     set (sit);
     tx_type_ = static_cast<TxType> (getFieldU16 (sfTransactionType));
 
-    auto format = TxFormats::getInstance().findByType (tx_type_);
-
-    if (!format)
-    {
-        WriteLog (lsWARNING, STTx) <<
-            "Invalid transaction type: " << tx_type_;
-        Throw<std::runtime_error> ("invalid transaction type");
-    }
-
-    if (!setType (format->elements))
-    {
-        WriteLog (lsWARNING, STTx) <<
-            "Transaction not legal for format";
+    if (!setType (getTxFormat (tx_type_)->elements))
         Throw<std::runtime_error> ("transaction not valid");
-    }
+
+    tid_ = getHash(HashPrefix::transactionID);
+}
+
+STTx::STTx (
+        TxType type,
+        std::function<void(STObject&)> assembler)
+    : STObject (sfTransaction)
+{
+    auto format = getTxFormat (type);
+
+    set (format->elements);
+    setFieldU16 (sfTransactionType, format->getType ());
+
+    assembler (*this);
+
+    tx_type_ = static_cast<TxType>(getFieldU16 (sfTransactionType));
+
+    if (tx_type_ != type)
+        LogicError ("Transaction type was mutated during assembly");
 
     tid_ = getHash(HashPrefix::transactionID);
 }
@@ -163,15 +153,6 @@ STTx::getSigningHash () const
     return STObject::getSigningHash (HashPrefix::txSign);
 }
 
-uint256
-STTx::getTransactionID () const
-{
-    assert(tid_);
-    if (! tid_)
-        LogicError("digest is undefined");
-    return *tid_;
-}
-
 Blob STTx::getSignature () const
 {
     try
@@ -216,11 +197,6 @@ bool STTx::checkSign(bool allowMultiSign) const
     return sigGood;
 }
 
-void STTx::setSigningPubKey (RippleAddress const& naSignPubKey)
-{
-    setFieldVL (sfSigningPubKey, naSignPubKey.getAccountPublic ());
-}
-
 Json::Value STTx::getJson (int) const
 {
     Json::Value ret = STObject::getJson (0);

src/ripple/protocol/tests/STTx.test.cpp

@@ -38,10 +38,13 @@ public:
         RippleAddress publicAcct = RippleAddress::createAccountPublic (generator, 1);
         RippleAddress privateAcct = RippleAddress::createAccountPrivate (generator, seed, 1);
 
-        STTx j (ttACCOUNT_SET);
-        j.setAccountID (sfAccount, calcAccountID(publicAcct));
-        j.setSigningPubKey (publicAcct);
-        j.setFieldVL (sfMessageKey, publicAcct.getAccountPublic ());
+        STTx j (ttACCOUNT_SET,
+            [&publicAcct](auto& obj)
+            {
+                obj.setAccountID (sfAccount, calcAccountID(publicAcct));
+                obj.setFieldVL (sfMessageKey, publicAcct.getAccountPublic ());
+                obj.setFieldVL (sfSigningPubKey, publicAcct.getAccountPublic ());
+            });
         j.sign (privateAcct);
 
         unexpected (!j.checkSign (true), "Transaction fails signature test");
@@ -92,12 +95,14 @@ public:
         // VFALCO Use PublicKey here
         RippleAddress txnPublicAcct = txnSeed.createAccountPublic (txnGenerator, 1);
 
-        STTx txn (ttACCOUNT_SET);
-        txn.setAccountID (sfAccount, calcAccountID(txnPublicAcct));
-        txn.setSigningPubKey (txnPublicAcct);
-        txn.setFieldVL (sfMessageKey, txnPublicAcct.getAccountPublic ());
-        Blob const emptyBlob;  // Make empty signature for multi-signing
-        txn.setFieldVL (sfSigningPubKey, emptyBlob);
+        STTx txn (ttACCOUNT_SET,
+            [&txnPublicAcct](auto& obj)
+            {
+                obj.setAccountID (sfAccount, calcAccountID(txnPublicAcct));
+                obj.setFieldVL (sfMessageKey, txnPublicAcct.getAccountPublic ());
+                // Make empty signature for multi-signing
+                obj.setFieldVL (sfSigningPubKey, {});
+            });
 
         // Create fields for a SigningAccount
         RippleAddress saSeed;