fixInnerObjTemplate: set inner object template (#4906)

Add `STObject` constructor to explicitly set the inner object template.
This allows certain AMM transactions to apply in the same ledger:

There is no issue if the trading fee is greater than or equal to 0.01%.
If the trading fee is less than 0.01%, then:
- After AMM create, AMM transactions must wait for one ledger to close
  (3-5 seconds).
- After one ledger is validated, all AMM transactions succeed, as
  appropriate, except for AMMVote.
- The first AMMVote which votes for a 0 trading fee in a ledger will
  succeed. Subsequent AMMVote transactions which vote for a 0 trading
  fee will wait for the next ledger (3-5 seconds). This behavior repeats
  for each ledger.

This has no effect on the ultimate correctness of AMM. This amendment
will allow the transactions described above to succeed as expected, even
if the trading fee is 0 and the transactions are applied within one
ledger (block).
This commit is contained in:
Gregory Tsipenyuk
2024-02-07 16:58:12 -05:00
committed by tequ
parent c29a632d0c
commit 71ffc69819
13 changed files with 363 additions and 31 deletions

View File

@@ -138,6 +138,9 @@ std::uint16_t
getTradingFee(ReadView const& view, SLE const& ammSle, AccountID const& account)
{
using namespace std::chrono;
assert(
!view.rules().enabled(fixInnerObjTemplate) ||
ammSle.isFieldPresent(sfAuctionSlot));
if (ammSle.isFieldPresent(sfAuctionSlot))
{
auto const& auctionSlot =
@@ -287,9 +290,10 @@ initializeFeeAuctionVote(
Issue const& lptIssue,
std::uint16_t tfee)
{
auto const& rules = view.rules();
// AMM creator gets the voting slot.
STArray voteSlots;
STObject voteEntry{sfVoteEntry};
STObject voteEntry = STObject::makeInnerObject(sfVoteEntry, rules);
if (tfee != 0)
voteEntry.setFieldU16(sfTradingFee, tfee);
voteEntry.setFieldU32(sfVoteWeight, VOTE_WEIGHT_SCALE_FACTOR);
@@ -297,7 +301,15 @@ initializeFeeAuctionVote(
voteSlots.push_back(voteEntry);
ammSle->setFieldArray(sfVoteSlots, voteSlots);
// AMM creator gets the auction slot for free.
auto& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot);
// AuctionSlot is created on AMMCreate and updated on AMMDeposit
// when AMM is in an empty state
if (rules.enabled(fixInnerObjTemplate) &&
!ammSle->isFieldPresent(sfAuctionSlot))
{
STObject auctionSlot = STObject::makeInnerObject(sfAuctionSlot, rules);
ammSle->set(std::move(auctionSlot));
}
STObject& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot);
auctionSlot.setAccountID(sfAccount, account);
// current + sec in 24h
auto const expiration = std::chrono::duration_cast<std::chrono::seconds>(

View File

@@ -173,8 +173,18 @@ applyBid(
return {tecINTERNAL, false};
STAmount const lptAMMBalance = (*ammSle)[sfLPTokenBalance];
auto const lpTokens = ammLPHolds(sb, *ammSle, account_, ctx_.journal);
if (!ammSle->isFieldPresent(sfAuctionSlot))
ammSle->makeFieldPresent(sfAuctionSlot);
auto const& rules = ctx_.view().rules();
if (!rules.enabled(fixInnerObjTemplate))
{
if (!ammSle->isFieldPresent(sfAuctionSlot))
ammSle->makeFieldPresent(sfAuctionSlot);
}
else
{
assert(ammSle->isFieldPresent(sfAuctionSlot));
if (!ammSle->isFieldPresent(sfAuctionSlot))
return {tecINTERNAL, false};
}
auto& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot);
auto const current =
duration_cast<seconds>(

View File

@@ -104,6 +104,7 @@ applyVote(
Number den{0};
// Account already has vote entry
bool foundAccount = false;
auto const& rules = ctx_.view().rules();
// Iterate over the current vote entries and update each entry
// per current total tokens balance and each LP tokens balance.
// Find the entry with the least tokens and whether the account
@@ -119,7 +120,7 @@ applyVote(
continue;
}
auto feeVal = entry[sfTradingFee];
STObject newEntry{sfVoteEntry};
STObject newEntry = STObject::makeInnerObject(sfVoteEntry, rules);
// The account already has the vote entry.
if (account == account_)
{
@@ -158,7 +159,7 @@ applyVote(
{
auto update = [&](std::optional<std::uint8_t> const& minPos =
std::nullopt) {
STObject newEntry{sfVoteEntry};
STObject newEntry = STObject::makeInnerObject(sfVoteEntry, rules);
if (feeNew != 0)
newEntry.setFieldU16(sfTradingFee, feeNew);
newEntry.setFieldU32(
@@ -199,6 +200,10 @@ applyVote(
}
}
assert(
!ctx_.view().rules().enabled(fixInnerObjTemplate) ||
ammSle->isFieldPresent(sfAuctionSlot));
// Update the vote entries and the trading/discounted fee.
ammSle->setFieldArray(sfVoteSlots, updatedVoteSlots);
if (auto const fee = static_cast<std::int64_t>(num / den))

View File

@@ -74,7 +74,7 @@ namespace detail {
// Feature.cpp. Because it's only used to reserve storage, and determine how
// large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than
// the actual number of amendments. A LogicError on startup will verify this.
static constexpr std::size_t numFeatures = 89;
static constexpr std::size_t numFeatures = 90;
/** Amendments that this server supports and the default voting behavior.
Whether they are enabled depends on the Rules defined in the validated
@@ -377,6 +377,7 @@ extern uint256 const fixDisallowIncomingV1;
extern uint256 const featureDID;
extern uint256 const fixFillOrKill;
extern uint256 const fixNFTokenReserve;
extern uint256 const fixInnerObjTemplate;
} // namespace ripple

View File

@@ -35,6 +35,8 @@ enum SOEStyle {
soeREQUIRED = 0, // required
soeOPTIONAL = 1, // optional, may be present with default value
soeDEFAULT = 2, // optional, if present, must not have default value
// inner object with the default fields has to be
// constructed with STObject::makeInnerObject()
};
//------------------------------------------------------------------------------

View File

@@ -43,6 +43,7 @@
namespace ripple {
class STArray;
class Rules;
inline void
throwFieldNotFound(SField const& field)
@@ -102,6 +103,9 @@ public:
STObject(SerialIter&& sit, SField const& name);
explicit STObject(SField const& name);
static STObject
makeInnerObject(SField const& name, Rules const& rules);
iterator
begin() const;
@@ -339,7 +343,7 @@ public:
set(std::unique_ptr<STBase> v);
void
set(STBase* v);
set(STBase&& v);
void
setFieldU8(SField const& field, unsigned char);

View File

@@ -483,6 +483,7 @@ REGISTER_FIX (fixDisallowIncomingV1, Supported::yes, VoteBehavior::De
REGISTER_FEATURE(DID, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FIX(fixFillOrKill, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FIX (fixNFTokenReserve, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FIX(fixInnerObjTemplate, Supported::yes, VoteBehavior::DefaultNo);
// The following amendments are obsolete, but must remain supported
// because they could potentially get enabled.

View File

@@ -25,6 +25,9 @@ namespace ripple {
InnerObjectFormats::InnerObjectFormats()
{
// inner objects with the default fields have to be
// constructed with STObject::makeInnerObject()
add(sfEmitDetails.jsonName.c_str(),
sfEmitDetails.getCode(),
{{sfEmitGeneration, soeREQUIRED},

View File

@@ -18,7 +18,9 @@
//==============================================================================
#include <ripple/basics/Log.h>
#include <ripple/protocol/Feature.h>
#include <ripple/protocol/InnerObjectFormats.h>
#include <ripple/protocol/Rules.h>
#include <ripple/protocol/STAccount.h>
#include <ripple/protocol/STArray.h>
#include <ripple/protocol/STBlob.h>
@@ -57,6 +59,19 @@ STObject::STObject(SerialIter& sit, SField const& name, int depth) noexcept(
set(sit, depth);
}
STObject
STObject::makeInnerObject(SField const& name, Rules const& rules)
{
STObject obj{name};
if (rules.enabled(fixInnerObjTemplate))
{
if (SOTemplate const* elements =
InnerObjectFormats::getInstance().findSOTemplateBySField(name))
obj.set(*elements);
}
return obj;
}
STBase*
STObject::copy(std::size_t n, void* buf) const
{
@@ -646,16 +661,22 @@ STObject::getFieldArray(SField const& field) const
void
STObject::set(std::unique_ptr<STBase> v)
{
auto const i = getFieldIndex(v->getFName());
set(std::move(*v.get()));
}
void
STObject::set(STBase&& v)
{
auto const i = getFieldIndex(v.getFName());
if (i != -1)
{
v_[i] = std::move(*v);
v_[i] = std::move(v);
}
else
{
if (!isFree())
Throw<std::runtime_error>("missing field in templated STObject");
v_.emplace_back(std::move(*v));
v_.emplace_back(std::move(v));
}
}

View File

@@ -200,6 +200,9 @@ doAMMInfo(RPC::JsonContext& context)
}
if (voteSlots.size() > 0)
ammResult[jss::vote_slots] = std::move(voteSlots);
assert(
!ledger->rules().enabled(fixInnerObjTemplate) ||
amm->isFieldPresent(sfAuctionSlot));
if (amm->isFieldPresent(sfAuctionSlot))
{
auto const& auctionSlot =

View File

@@ -4789,6 +4789,106 @@ private:
}
}
void
testFixDefaultInnerObj()
{
testcase("Fix Default Inner Object");
using namespace jtx;
FeatureBitset const all{supported_amendments()};
auto test = [&](FeatureBitset features,
TER const& err1,
TER const& err2,
TER const& err3,
TER const& err4,
std::uint16_t tfee,
bool closeLedger,
std::optional<std::uint16_t> extra = std::nullopt) {
Env env(*this, features);
fund(env, gw, {alice}, XRP(1'000), {USD(10)});
AMM amm(
env,
gw,
XRP(10),
USD(10),
{.tfee = tfee, .close = closeLedger});
amm.deposit(alice, USD(10), XRP(10));
amm.vote(VoteArg{.account = alice, .tfee = tfee, .err = ter(err1)});
amm.withdraw(WithdrawArg{
.account = gw, .asset1Out = USD(1), .err = ter(err2)});
// with the amendment disabled and ledger not closed,
// second vote succeeds if the first vote sets the trading fee
// to non-zero; if the first vote sets the trading fee to >0 && <9
// then the second withdraw succeeds if the second vote sets
// the trading fee so that the discounted fee is non-zero
amm.vote(VoteArg{.account = alice, .tfee = 20, .err = ter(err3)});
amm.withdraw(WithdrawArg{
.account = gw, .asset1Out = USD(2), .err = ter(err4)});
};
// ledger is closed after each transaction, vote/withdraw don't fail
// regardless whether the amendment is enabled or not
test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 0, true);
test(
all - fixInnerObjTemplate,
tesSUCCESS,
tesSUCCESS,
tesSUCCESS,
tesSUCCESS,
0,
true);
// ledger is not closed after each transaction
// vote/withdraw don't fail if the amendment is enabled
test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 0, false);
// vote/withdraw fail if the amendment is not enabled
// second vote/withdraw still fail: second vote fails because
// the initial trading fee is 0, consequently second withdraw fails
// because the second vote fails
test(
all - fixInnerObjTemplate,
tefEXCEPTION,
tefEXCEPTION,
tefEXCEPTION,
tefEXCEPTION,
0,
false);
// if non-zero trading/discounted fee then vote/withdraw
// don't fail whether the ledger is closed or not and
// the amendment is enabled or not
test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 10, true);
test(
all - fixInnerObjTemplate,
tesSUCCESS,
tesSUCCESS,
tesSUCCESS,
tesSUCCESS,
10,
true);
test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 10, false);
test(
all - fixInnerObjTemplate,
tesSUCCESS,
tesSUCCESS,
tesSUCCESS,
tesSUCCESS,
10,
false);
// non-zero trading fee but discounted fee is 0, vote doesn't fail
// but withdraw fails
test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 9, false);
// second vote sets the trading fee to non-zero, consequently
// second withdraw doesn't fail even if the amendment is not
// enabled and the ledger is not closed
test(
all - fixInnerObjTemplate,
tesSUCCESS,
tefEXCEPTION,
tesSUCCESS,
tesSUCCESS,
9,
false);
}
void
testCore()
{
@@ -4815,6 +4915,7 @@ private:
testClawback();
testAMMID();
testSelection();
testFixDefaultInnerObj();
}
void

View File

@@ -57,6 +57,67 @@ public:
}
};
struct CreateArg
{
bool log = false;
std::uint16_t tfee = 0;
std::uint32_t fee = 0;
std::optional<std::uint32_t> flags = std::nullopt;
std::optional<jtx::seq> seq = std::nullopt;
std::optional<jtx::msig> ms = std::nullopt;
std::optional<ter> err = std::nullopt;
bool close = true;
};
struct DepositArg
{
std::optional<Account> account = std::nullopt;
std::optional<LPToken> tokens = std::nullopt;
std::optional<STAmount> asset1In = std::nullopt;
std::optional<STAmount> asset2In = std::nullopt;
std::optional<STAmount> maxEP = std::nullopt;
std::optional<std::uint32_t> flags = std::nullopt;
std::optional<std::pair<Issue, Issue>> assets = std::nullopt;
std::optional<jtx::seq> seq = std::nullopt;
std::optional<std::uint16_t> tfee = std::nullopt;
std::optional<ter> err = std::nullopt;
};
struct WithdrawArg
{
std::optional<Account> account = std::nullopt;
std::optional<LPToken> tokens = std::nullopt;
std::optional<STAmount> asset1Out = std::nullopt;
std::optional<STAmount> asset2Out = std::nullopt;
std::optional<IOUAmount> maxEP = std::nullopt;
std::optional<std::uint32_t> flags = std::nullopt;
std::optional<std::pair<Issue, Issue>> assets = std::nullopt;
std::optional<jtx::seq> seq = std::nullopt;
std::optional<ter> err = std::nullopt;
};
struct VoteArg
{
std::optional<Account> account = std::nullopt;
std::uint32_t tfee = 0;
std::optional<std::uint32_t> flags = std::nullopt;
std::optional<jtx::seq> seq = std::nullopt;
std::optional<std::pair<Issue, Issue>> assets = std::nullopt;
std::optional<ter> err = std::nullopt;
};
struct BidArg
{
std::optional<Account> account = std::nullopt;
std::optional<std::variant<int, IOUAmount, STAmount>> bidMin = std::nullopt;
std::optional<std::variant<int, IOUAmount, STAmount>> bidMax = std::nullopt;
std::vector<Account> authAccounts = {};
std::optional<std::uint32_t> flags = std::nullopt;
std::optional<jtx::seq> seq = std::nullopt;
std::optional<std::pair<Issue, Issue>> assets = std::nullopt;
std::optional<ter> err = std::nullopt;
};
/** Convenience class to test AMM functionality.
*/
class AMM
@@ -91,13 +152,20 @@ public:
std::optional<std::uint32_t> flags = std::nullopt,
std::optional<jtx::seq> seq = std::nullopt,
std::optional<jtx::msig> ms = std::nullopt,
std::optional<ter> const& ter = std::nullopt);
std::optional<ter> const& ter = std::nullopt,
bool close = true);
AMM(Env& env,
Account const& account,
STAmount const& asset1,
STAmount const& asset2,
ter const& ter,
bool log = false);
bool log = false,
bool close = true);
AMM(Env& env,
Account const& account,
STAmount const& asset1,
STAmount const& asset2,
CreateArg const& arg);
/** Send amm_info RPC command
*/
@@ -189,6 +257,9 @@ public:
std::optional<std::uint16_t> const& tfee = std::nullopt,
std::optional<ter> const& ter = std::nullopt);
IOUAmount
deposit(DepositArg const& arg);
IOUAmount
withdraw(
std::optional<Account> const& account,
@@ -200,14 +271,15 @@ public:
IOUAmount
withdrawAll(
std::optional<Account> const& account,
std::optional<STAmount> const& asset1OutDetails = std::nullopt)
std::optional<STAmount> const& asset1OutDetails = std::nullopt,
std::optional<ter> const& ter = std::nullopt)
{
return withdraw(
account,
std::nullopt,
asset1OutDetails,
asset1OutDetails ? tfOneAssetWithdrawAll : tfWithdrawAll,
std::nullopt);
ter);
}
IOUAmount
@@ -230,6 +302,9 @@ public:
std::optional<jtx::seq> const& seq,
std::optional<ter> const& ter = std::nullopt);
IOUAmount
withdraw(WithdrawArg const& arg);
void
vote(
std::optional<Account> const& account,
@@ -239,6 +314,9 @@ public:
std::optional<std::pair<Issue, Issue>> const& assets = std::nullopt,
std::optional<ter> const& ter = std::nullopt);
void
vote(VoteArg const& arg);
void
bid(std::optional<Account> const& account,
std::optional<std::variant<int, IOUAmount, STAmount>> const& bidMin =
@@ -251,6 +329,9 @@ public:
std::optional<std::pair<Issue, Issue>> const& assets = std::nullopt,
std::optional<ter> const& ter = std::nullopt);
void
bid(BidArg const& arg);
AccountID const&
ammAccount() const
{

View File

@@ -57,7 +57,8 @@ AMM::AMM(
std::optional<std::uint32_t> flags,
std::optional<jtx::seq> seq,
std::optional<jtx::msig> ms,
std::optional<ter> const& ter)
std::optional<ter> const& ter,
bool close)
: env_(env)
, creatorAccount_(account)
, asset1_(asset1)
@@ -65,7 +66,7 @@ AMM::AMM(
, ammID_(keylet::amm(asset1_.issue(), asset2_.issue()).key)
, initialLPTokens_(initialTokens(asset1, asset2))
, log_(log)
, doClose_(true)
, doClose_(close)
, lastPurchasePrice_(0)
, bidMin_()
, bidMax_()
@@ -85,7 +86,8 @@ AMM::AMM(
STAmount const& asset1,
STAmount const& asset2,
ter const& ter,
bool log)
bool log,
bool close)
: AMM(env,
account,
asset1,
@@ -96,7 +98,29 @@ AMM::AMM(
std::nullopt,
std::nullopt,
std::nullopt,
ter)
ter,
close)
{
}
AMM::AMM(
Env& env,
Account const& account,
STAmount const& asset1,
STAmount const& asset2,
CreateArg const& arg)
: AMM(env,
account,
asset1,
asset2,
arg.log,
arg.tfee,
arg.fee,
arg.flags,
arg.seq,
arg.ms,
arg.err,
arg.close)
{
}
@@ -470,6 +494,22 @@ AMM::deposit(
return deposit(account, jv, assets, seq, ter);
}
IOUAmount
AMM::deposit(DepositArg const& arg)
{
return deposit(
arg.account,
arg.tokens,
arg.asset1In,
arg.asset2In,
arg.maxEP,
arg.flags,
arg.assets,
arg.seq,
arg.tfee,
arg.err);
}
IOUAmount
AMM::withdraw(
std::optional<Account> const& account,
@@ -574,6 +614,21 @@ AMM::withdraw(
return withdraw(account, jv, seq, assets, ter);
}
IOUAmount
AMM::withdraw(WithdrawArg const& arg)
{
return withdraw(
arg.account,
arg.tokens,
arg.asset1Out,
arg.asset2Out,
arg.maxEP,
arg.flags,
arg.assets,
arg.seq,
arg.err);
}
void
AMM::vote(
std::optional<Account> const& account,
@@ -595,6 +650,12 @@ AMM::vote(
submit(jv, seq, ter);
}
void
AMM::vote(VoteArg const& arg)
{
return vote(arg.account, arg.tfee, arg.flags, arg.seq, arg.assets, arg.err);
}
void
AMM::bid(
std::optional<Account> const& account,
@@ -609,6 +670,9 @@ AMM::bid(
if (auto const amm =
env_.current()->read(keylet::amm(asset1_.issue(), asset2_.issue())))
{
assert(
!env_.current()->rules().enabled(fixInnerObjTemplate) ||
amm->isFieldPresent(sfAuctionSlot));
if (amm->isFieldPresent(sfAuctionSlot))
{
auto const& auctionSlot =
@@ -663,6 +727,20 @@ AMM::bid(
submit(jv, seq, ter);
}
void
AMM::bid(BidArg const& arg)
{
return bid(
arg.account,
arg.bidMin,
arg.bidMax,
arg.authAccounts,
arg.flags,
arg.seq,
arg.assets,
arg.err);
}
void
AMM::submit(
Json::Value const& jv,
@@ -698,20 +776,30 @@ bool
AMM::expectAuctionSlot(auto&& cb) const
{
if (auto const amm =
env_.current()->read(keylet::amm(asset1_.issue(), asset2_.issue()));
amm && amm->isFieldPresent(sfAuctionSlot))
env_.current()->read(keylet::amm(asset1_.issue(), asset2_.issue())))
{
auto const& auctionSlot =
static_cast<STObject const&>(amm->peekAtField(sfAuctionSlot));
if (auctionSlot.isFieldPresent(sfAccount))
assert(
!env_.current()->rules().enabled(fixInnerObjTemplate) ||
amm->isFieldPresent(sfAuctionSlot));
if (amm->isFieldPresent(sfAuctionSlot))
{
auto const slotFee = auctionSlot[sfDiscountedFee];
auto const slotInterval = ammAuctionTimeSlot(
env_.app().timeKeeper().now().time_since_epoch().count(),
auctionSlot);
auto const slotPrice = auctionSlot[sfPrice].iou();
auto const authAccounts = auctionSlot.getFieldArray(sfAuthAccounts);
return cb(slotFee, slotInterval, slotPrice, authAccounts);
auto const& auctionSlot =
static_cast<STObject const&>(amm->peekAtField(sfAuctionSlot));
if (auctionSlot.isFieldPresent(sfAccount))
{
// This could fail in pre-fixInnerObjTemplate tests
// if the submitted transactions recreate one of
// the failure scenarios. Access as optional
// to avoid the failure.
auto const slotFee = auctionSlot[~sfDiscountedFee].value_or(0);
auto const slotInterval = ammAuctionTimeSlot(
env_.app().timeKeeper().now().time_since_epoch().count(),
auctionSlot);
auto const slotPrice = auctionSlot[sfPrice].iou();
auto const authAccounts =
auctionSlot.getFieldArray(sfAuthAccounts);
return cb(slotFee, slotInterval, slotPrice, authAccounts);
}
}
}
return false;