Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(AMM): prevent orphaned objects, inconsistent ledger state: (updates XLS-30) #4626

Closed
wants to merge 20 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
da44bc8
Cleanup AMM account owner directory on AMM account deletion
gregtatcam Jul 17, 2023
5cead5b
Disallow check create to AMM
gregtatcam Jul 17, 2023
eaef4b6
Fix unconstrained entries in AuthAccount
gregtatcam Jul 17, 2023
b28101f
Allow SetTrust on AMM only for LP tokens and other changes
gregtatcam Jul 18, 2023
cbd7144
Add AMMDelete to handle amortized deletion and other changes to addre…
gregtatcam Jul 23, 2023
40317c5
Fix missing AMMDelete transactor files
gregtatcam Jul 24, 2023
9cf3fd7
Merge remote-tracking branch 'origin/develop' into amm-fixes
gregtatcam Jul 24, 2023
75af923
Update api changelog for AMM feature
gregtatcam Jul 24, 2023
cd88481
Maintain AMM trustlines count in AMM root account and other changes a…
gregtatcam Jul 28, 2023
9ddff74
Check no directory left after AMM trustlines are deleted and other mi…
gregtatcam Jul 29, 2023
2b1c8c6
Disallow clawback out of AMM account
gregtatcam Jul 31, 2023
c5e3c89
Disallow AMM create if issuer has clawback enabled and other changes
gregtatcam Aug 1, 2023
b63e696
Remove lsfAMM flag and use sfAMMID instead, plus minor refactoring
gregtatcam Aug 2, 2023
2efa0f8
Rall-back lsfAllowTrustLineClawback flag and other changes
gregtatcam Aug 4, 2023
fa0d9c4
Address auditor's feedback
gregtatcam Aug 7, 2023
7e2c454
Update tecAMM_ACCOUNT error message
gregtatcam Aug 11, 2023
c07bb67
Merge branch 'develop' into amm-fixes
manojsdoshi Aug 18, 2023
484e01f
Add unit-tests to verify CLOB/AMM offer and strand selection logic
gregtatcam Aug 7, 2023
cfa3201
Address reviewer's feedback
gregtatcam Aug 11, 2023
a34d29b
Add comments to stress that the selection tests would have to be upda…
gregtatcam Aug 11, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions API-CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,28 @@ Additions are intended to be non-breaking (because they are purely additive).
- Adds the [Clawback transaction type](https:/XRPLF/XRPL-Standards/blob/master/XLS-39d-clawback/README.md#331-clawback-transaction), containing these fields:
- `Account`: The issuer of the asset being clawed back. Must also be the sender of the transaction.
- `Amount`: The amount being clawed back, with the `Amount.issuer` being the token holder's address.
- Adds [AMM](https:/XRPLF/XRPL-Standards/discussions/78) ([#4294](https:/XRPLF/rippled/pull/4294), [#4626](https:/XRPLF/rippled/pull/4626)) feature:
- Adds `amm_info` API to retrieve AMM information for a given tokens pair.
- Adds `AMMCreate` transaction type to create `AMM` instance.
- Adds `AMMDeposit` transaction type to deposit funds into `AMM` instance.
- Adds `AMMWithdraw` transaction type to withdraw funds from `AMM` instance.
- Adds `AMMVote` transaction type to vote for the trading fee of `AMM` instance.
- Adds `AMMBid` transaction type to bid for the Auction Slot of `AMM` instance.
- Adds `AMMDelete` transaction type to delete `AMM` instance.
- Adds `sfAMMID` to `AccountRoot` to indicate that the account is `AMM`'s account. `AMMID` is used to fetch `ltAMM`.
- Adds `lsfAMMNode` `TrustLine` flag to indicate that one side of the `TrustLine` is `AMM` account.
- Adds `tfLPToken`, `tfSingleAsset`, `tfTwoAsset`, `tfOneAssetLPToken`, `tfLimitLPToken`, `tfTwoAssetIfEmpty`,
`tfWithdrawAll`, `tfOneAssetWithdrawAll` which allow a trader to specify different fields combination
for `AMMDeposit` and `AMMWithdraw` transactions.
- Adds new transaction result codes:
- tecUNFUNDED_AMM: insufficient balance to fund AMM. The account does not have funds for liquidity provision.
- tecAMM_BALANCE: AMM has invalid balance. Calculated balances greater than the current pool balances.
- tecAMM_FAILED: AMM transaction failed. Fails due to a processing failure.
- tecAMM_INVALID_TOKENS: AMM invalid LP tokens. Invalid input values, format, or calculated values.
- tecAMM_EMPTY: AMM is in empty state. Transaction expects AMM in non-empty state (LP tokens > 0).
- tecAMM_NOT_EMPTY: AMM is not in empty state. Transaction expects AMM in empty state (LP tokens == 0).
- tecAMM_ACCOUNT: AMM account. Clawback of AMM account.
- tecINCOMPLETE: Some work was completed, but more submissions required to finish. AMMDelete partially deletes the trustlines.

## XRP Ledger version 1.11.0

Expand Down
1 change: 1 addition & 0 deletions Builds/CMake/RippledCore.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,7 @@ target_sources (rippled PRIVATE
src/ripple/app/rdb/impl/Wallet.cpp
src/ripple/app/tx/impl/AMMBid.cpp
src/ripple/app/tx/impl/AMMCreate.cpp
src/ripple/app/tx/impl/AMMDelete.cpp
src/ripple/app/tx/impl/AMMDeposit.cpp
src/ripple/app/tx/impl/AMMVote.cpp
src/ripple/app/tx/impl/AMMWithdraw.cpp
Expand Down
20 changes: 20 additions & 0 deletions src/ripple/app/misc/AMMUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,26 @@ ammAccountHolds(
AccountID const& ammAccountID,
Issue const& issue);

/** Delete trustlines to AMM. If all trustlines are deleted then
* AMM object and account are deleted. Otherwise tecIMPCOMPLETE is returned.
*/
TER
deleteAMMAccount(
Sandbox& view,
Issue const& asset,
Issue const& asset2,
beast::Journal j);

/** Initialize Auction and Voting slots and set the trading/discounted fee.
*/
void
initializeFeeAuctionVote(
ApplyView& view,
std::shared_ptr<SLE>& ammSle,
AccountID const& account,
Issue const& lptIssue,
std::uint16_t tfee);

} // namespace ripple

#endif // RIPPLE_APP_MISC_AMMUTILS_H_INLCUDED
118 changes: 118 additions & 0 deletions src/ripple/app/misc/impl/AMMUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -188,4 +188,122 @@ ammAccountHolds(
return STAmount{issue};
}

static TER
deleteAMMTrustLines(
Sandbox& sb,
AccountID const& ammAccountID,
std::uint16_t maxTrustlinesToDelete,
beast::Journal j)
{
return cleanupOnAccountDelete(
sb,
keylet::ownerDir(ammAccountID),
[&](LedgerEntryType nodeType,
uint256 const&,
std::shared_ptr<SLE>& sleItem) -> TER {
// Should only have the trustlines
if (nodeType != LedgerEntryType::ltRIPPLE_STATE)
{
JLOG(j.error())
<< "deleteAMMTrustLines: deleting non-trustline "
<< nodeType;
return tecINTERNAL;
}

// Trustlines must have zero balance
if (sleItem->getFieldAmount(sfBalance) != beast::zero)
{
JLOG(j.error())
<< "deleteAMMTrustLines: deleting trustline with "
"non-zero balance.";
return tecINTERNAL;
}

return deleteAMMTrustLine(sb, sleItem, ammAccountID, j);
},
j,
maxTrustlinesToDelete);
}

TER
deleteAMMAccount(
Sandbox& sb,
Issue const& asset,
Issue const& asset2,
beast::Journal j)
{
auto ammSle = sb.peek(keylet::amm(asset, asset2));
if (!ammSle)
{
JLOG(j.error()) << "deleteAMMAccount: AMM object does not exist "
<< asset << " " << asset2;
return tecINTERNAL;
}

auto const ammAccountID = (*ammSle)[sfAccount];
auto sleAMMRoot = sb.peek(keylet::account(ammAccountID));
if (!sleAMMRoot)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the account root somehow doesn't exist, this is a log-worthy event but should the transaction be rolled back? If the account is missing we can still delete the AMM object... in fact we definitely should? The delete transactor should double as a "fix up" transactor. If an AMM ever gets into a bad state, somehow, then deleting it should repair the situation as much as possible.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Counterpoint: If the account root doesn't exist, something has gone very, very wrong. Continuing with the deletion process may make things worse. Arguably, the ledger and transaction history should be examined manually to determine what happened and what's going on. The solution may require a fix amendment not just to clean up the ledger, but to prevent the issue from happening again.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Counterpoint: If the account root doesn't exist, something has gone very, very wrong. Continuing with the deletion process may make things worse. Arguably, the ledger and transaction history should be examined manually to determine what happened and what's going on. The solution may require a fix amendment not just to clean up the ledger, but to prevent the issue from happening again.

This would be an ungraceful failure mode. Half an AMM, if it happened, would probably crash the ledger or block an orderbook. You can't clean it up because of this pedantic check, even after restarting the whole network. Now you need to wait for an amendment and code update. You should seek to build graceful failure modes into production systems.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the ledger is corrupt, we're already in a failure mode. Better to stop than to continue in an state that may not be correct.

https://xrpl.org/intro-to-consensus.html#consensus-protocol-properties

These properties are sometimes summarized as the following principles, in order of priority: Correctness, Agreement, Forward Progress.

If correctness is lost, then we do not want to make forward progress. This is not just me being stubborn, this is baked into the design of the ledger from the beginning.

{
JLOG(j.error()) << "deleteAMMAccount: AMM account does not exist "
<< to_string(ammAccountID);
return tecINTERNAL;
}

if (auto const ter =
deleteAMMTrustLines(sb, ammAccountID, maxDeletableAMMTrustLines, j);
ter != tesSUCCESS)
return ter;

auto const ownerDirKeylet = keylet::ownerDir(ammAccountID);
if (sb.exists(ownerDirKeylet) && !sb.emptyDirDelete(ownerDirKeylet))
{
ximinez marked this conversation as resolved.
Show resolved Hide resolved
JLOG(j.error()) << "deleteAMMAccount: cannot delete root dir node of "
<< toBase58(ammAccountID);
return tecINTERNAL;
}
ximinez marked this conversation as resolved.
Show resolved Hide resolved

sb.erase(ammSle);
sb.erase(sleAMMRoot);

return tesSUCCESS;
}

void
initializeFeeAuctionVote(
ApplyView& view,
std::shared_ptr<SLE>& ammSle,
AccountID const& account,
Issue const& lptIssue,
std::uint16_t tfee)
{
// AMM creator gets the voting slot.
STArray voteSlots;
STObject voteEntry{sfVoteEntry};
if (tfee != 0)
voteEntry.setFieldU16(sfTradingFee, tfee);
voteEntry.setFieldU32(sfVoteWeight, VOTE_WEIGHT_SCALE_FACTOR);
voteEntry.setAccountID(sfAccount, account);
voteSlots.push_back(voteEntry);
ammSle->setFieldArray(sfVoteSlots, voteSlots);
// AMM creator gets the auction slot for free.
auto& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot);
auctionSlot.setAccountID(sfAccount, account);
// current + sec in 24h
auto const expiration = std::chrono::duration_cast<std::chrono::seconds>(
view.info().parentCloseTime.time_since_epoch())
.count() +
TOTAL_TIME_SLOT_SECS;
auctionSlot.setFieldU32(sfExpiration, expiration);
auctionSlot.setFieldAmount(sfPrice, STAmount{lptIssue, 0});
// Set the fee
if (tfee != 0)
ammSle->setFieldU16(sfTradingFee, tfee);
else if (ammSle->isFieldPresent(sfTradingFee))
ammSle->makeFieldAbsent(sfTradingFee);
if (auto const dfee = tfee / AUCTION_SLOT_DISCOUNTED_FEE_FRACTION)
auctionSlot.setFieldU16(sfDiscountedFee, dfee);
else if (auctionSlot.isFieldPresent(sfDiscountedFee))
auctionSlot.makeFieldAbsent(sfDiscountedFee);
}

} // namespace ripple
3 changes: 2 additions & 1 deletion src/ripple/app/paths/impl/BookStep.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,8 @@ class BookStep : public StepImp<TIn, TOut, BookStep<TIn, TOut, TDerived>>
, ownerPaysTransferFee_(ctx.ownerPaysTransferFee)
, j_(ctx.j)
{
if (auto const ammSle = ctx.view.read(keylet::amm(in, out)))
if (auto const ammSle = ctx.view.read(keylet::amm(in, out));
ammSle && ammSle->getFieldAmount(sfLPTokenBalance) != beast::zero)
ammLiquidity_.emplace(
ctx.view,
(*ammSle)[sfAccount],
Expand Down
11 changes: 7 additions & 4 deletions src/ripple/app/tx/impl/AMMBid.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,10 @@ AMMBid::preclaim(PreclaimContext const& ctx)
return terNO_AMM;
}

auto const lpTokensBalance = (*ammSle)[sfLPTokenBalance];
if (lpTokensBalance == beast::zero)
return tecAMM_EMPTY;

if (ctx.tx.isFieldPresent(sfAuthAccounts))
{
for (auto const& account : ctx.tx.getFieldArray(sfAuthAccounts))
Expand All @@ -114,7 +118,6 @@ AMMBid::preclaim(PreclaimContext const& ctx)
JLOG(ctx.j.debug()) << "AMM Bid: account is not LP.";
return tecAMM_INVALID_TOKENS;
}
auto const lpTokensBalance = (*ammSle)[sfLPTokenBalance];

auto const bidMin = ctx.tx[~sfBidMin];

Expand Down Expand Up @@ -204,10 +207,10 @@ applyBid(
Number const& burn) -> TER {
auctionSlot.setAccountID(sfAccount, account_);
auctionSlot.setFieldU32(sfExpiration, current + TOTAL_TIME_SLOT_SECS);
if (fee == 0)
auctionSlot.makeFieldAbsent(sfDiscountedFee);
else
if (fee != 0)
auctionSlot.setFieldU16(sfDiscountedFee, fee);
else if (auctionSlot.isFieldPresent(sfDiscountedFee))
auctionSlot.makeFieldAbsent(sfDiscountedFee);
ximinez marked this conversation as resolved.
Show resolved Hide resolved
auctionSlot.setFieldAmount(
sfPrice, toSTAmount(lpTokens.issue(), minPrice));
if (ctx_.tx.isFieldPresent(sfAuthAccounts))
Expand Down
58 changes: 22 additions & 36 deletions src/ripple/app/tx/impl/AMMCreate.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ AMMCreate::preclaim(PreclaimContext const& ctx)
auto isLPToken = [&](STAmount const& amount) -> bool {
if (auto const sle =
ctx.view.read(keylet::account(amount.issue().account)))
return (sle->getFlags() & lsfAMM);
return sle->isFieldPresent(sfAMMID);
return false;
};

Expand All @@ -184,7 +184,21 @@ AMMCreate::preclaim(PreclaimContext const& ctx)
return tecAMM_INVALID_TOKENS;
}

return tesSUCCESS;
// Disallow AMM if the issuer has clawback enabled
auto clawbackDisabled = [&](Issue const& issue) -> TER {
if (isXRP(issue))
return tesSUCCESS;
if (auto const sle = ctx.view.read(keylet::account(issue.account));
!sle)
return tecINTERNAL;
else if (sle->getFlags() & lsfAllowTrustLineClawback)
return tecNO_PERMISSION;
return tesSUCCESS;
};

if (auto const ter = clawbackDisabled(amount.issue()); ter != tesSUCCESS)
return ter;
return clawbackDisabled(amount2.issue());
}

static std::pair<TER, bool>
Expand Down Expand Up @@ -247,54 +261,26 @@ applyCreate(
// A user can only receive LPTokens through affirmative action -
// either an AMMDeposit, TrustSet, crossing an offer, etc.
sleAMMRoot->setFieldU32(
sfFlags, lsfAMM | lsfDisableMaster | lsfDefaultRipple | lsfDepositAuth);
sfFlags, lsfDisableMaster | lsfDefaultRipple | lsfDepositAuth);
// Link the root account and AMM object
sleAMMRoot->setFieldH256(sfAMMID, ammKeylet.key);
sb.insert(sleAMMRoot);

// Calculate initial LPT balance.
auto const lpTokens = ammLPTokens(amount, amount2, lptIss);

// Create ltAMM
auto ammSle = std::make_shared<SLE>(ammKeylet);
if (ctx_.tx[sfTradingFee] != 0)
ammSle->setFieldU16(sfTradingFee, ctx_.tx[sfTradingFee]);
ammSle->setAccountID(sfAccount, *ammAccount);
ammSle->setFieldAmount(sfLPTokenBalance, lpTokens);
auto const& [issue1, issue2] = std::minmax(amount.issue(), amount2.issue());
ammSle->setFieldIssue(sfAsset, STIssue{sfAsset, issue1});
ammSle->setFieldIssue(sfAsset2, STIssue{sfAsset2, issue2});
// AMM creator gets the voting slot.
STArray voteSlots;
STObject voteEntry{sfVoteEntry};
if (ctx_.tx[sfTradingFee] != 0)
voteEntry.setFieldU16(sfTradingFee, ctx_.tx[sfTradingFee]);
voteEntry.setFieldU32(sfVoteWeight, 100000);
voteEntry.setAccountID(sfAccount, account_);
voteSlots.push_back(voteEntry);
ammSle->setFieldArray(sfVoteSlots, voteSlots);
// AMM creator gets the auction slot for free.
auto& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot);
auctionSlot.setAccountID(sfAccount, account_);
// current + sec in 24h
auto const expiration =
std::chrono::duration_cast<std::chrono::seconds>(
ctx_.view().info().parentCloseTime.time_since_epoch())
.count() +
TOTAL_TIME_SLOT_SECS;
auctionSlot.setFieldU32(sfExpiration, expiration);
auctionSlot.setFieldAmount(sfPrice, STAmount{lpTokens.issue(), 0});
// AMM creator gets the auction slot and the voting slot.
initializeFeeAuctionVote(
ctx_.view(), ammSle, account_, lptIss, ctx_.tx[sfTradingFee]);
sb.insert(ammSle);

// Add owner directory to link the root account and AMM object.
if (auto const page = sb.dirInsert(
keylet::ownerDir(*ammAccount),
ammSle->key(),
describeOwnerDir(*ammAccount));
!page)
{
JLOG(j_.debug()) << "AMM Instance: failed to insert owner dir";
return {tecDIR_FULL, false};
}

// Send LPT to LP.
auto res = accountSend(sb, *ammAccount, account_, lpTokens, ctx_.journal);
if (res != tesSUCCESS)
Expand Down
Loading