Update amm_info to fetch AMM by amm account and add owner directory for AMM object #4682

gregtatcam · 2023-08-30T20:03:32Z

High Level Overview of Change

Add back the owner directory, which was deleted by #4626.
Update amm_info to fetch AMM object by amm account id instead of the assets pair.

Context of Change

#4626 removed the AMM object directory entry and added AMMID. This was done for efficiency consideration because it could take up to 32 iterations to get AMM object. This change broke account_objects for amm type. This PR adds back the owner directory and also updates amm_info to fetch AMM object via amm account id instead of the asset pair.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Tests (You added tests for code that already exists, or your new feature included in this PR)

Before / After

account_objects works with amm type. amm_info can be used with either the asset pair or amm account id to retrieve AMM object.

Test Plan

AccountObjects is extended for amm type tests.
AMMInfo is extended to test for amm_info with amm account id.

…AMM object.

ximinez

Incremental "first impressions" review.

ximinez · 2023-08-30T20:34:17Z

src/ripple/protocol/jss.h

@@ -160,6 +160,7 @@ JSS(alternatives);           // out: PathRequest, RipplePathFind
 JSS(amendment_blocked);      // out: NetworkOPs
 JSS(amendments);             // in: AccountObjects, out: NetworkOPs
 JSS(amm);                    // out: amm_info
+JSS(amm_account);            // in: amm_info


This is going to cause confusion. I think it would make much more sense to reuse account to provide the AMM account ID, and peer (currently account) to designate the second / user account. This is how account_lines does it, so it'll be familiar to people.

Unfortunately, it will require updating any documentation and libraries, but I think it makes way more sense than introducing a new similarly-named parameter.

I thought that was pretty clear. But I'm not good with names. @mDuo13 what do you think about the name change?

Both seem fine to me. EDIT: Unless I'm misunderstanding something—What's the difference between account and amm_account in this request?

EDIT: Unless I'm misunderstanding something—What's the difference between account and amm_account in this request?

The way I understand it,

amm_account is the r... address of the account that is part of the AMM itself. It's an alternative to asset and asset2 as a way to look up the AMM.

account is the r... address of (possibly) a liquidity provider for the AMM.

Yes, this is correct.

Expanding on this for future readers and tech writers / documenters:

amm_account is AMM account ID. It allows users to fetch AMM Object with AMM account ID instead of using the asset pair.

Either amm_account or asset and asset2 have to be included.

account is a liquidity provider (LP) account. If account is included, then lp_token is set to that LP's token balance.

account is an r... address and is kind of like the "perspective" account.

It is like the peer in account_lines, or the taker in book_offers.

src/ripple/app/misc/impl/AMMUtils.cpp

seelabs

Left some suggestions about code cleanup. Functionality looks good.

src/ripple/app/misc/impl/AMMUtils.cpp

src/ripple/app/tx/impl/AMMCreate.cpp

src/ripple/ledger/impl/View.cpp

src/ripple/rpc/handlers/AMMInfo.cpp

seelabs · 2023-08-31T03:28:10Z

src/ripple/rpc/handlers/AMMInfo.cpp

+    uint256 ammID;
+    bool const isAssets =
+        params.isMember(jss::asset) && params.isMember(jss::asset2);
+    bool const isAMMAccount = params.isMember(jss::amm_account);


If the variables are made optional we can remove the isAssets and isAMMAccount and just check if the optional is set.

seelabs · 2023-08-31T03:28:29Z

src/ripple/rpc/handlers/AMMInfo.cpp


-    if (!params.isMember(jss::asset) || !params.isMember(jss::asset2))
+    if ((isAssets && isAMMAccount) || (!isAssets && !isAMMAccount))


You could just check for equality.

seelabs · 2023-08-31T03:31:03Z

src/ripple/rpc/handlers/AMMInfo.cpp

+            return result;
+        }
+        else
+            issue1 = *i;


I see this pattern in this code a lot - where the error condition doesn't use the "if local" variable, and we handle the error condition first. I think this is simpler to read by handling the non-error case in the main branch and removing the ; !i section. Here and other places as well.

seelabs · 2023-08-31T03:33:03Z

src/ripple/rpc/handlers/AMMInfo.cpp

+            return result;
+        }
+        else
+            ammID = sle->getFieldH256(sfAMMID);


I think this code is clearer without the cascading local variables declared in the if statements. That pattern is OK if it prevents the declared variables from polluting the parent scope. Here the parent scope can be small.

seelabs · 2023-08-31T03:34:34Z

src/ripple/rpc/handlers/AMMInfo.cpp

+    {
+        issue1 = (*amm)[sfAsset];
+        issue2 = (*amm)[sfAsset2];
+    }


This whole section is used to get the accountID, issue1 issue2 and amm. Consider re-writing this with a lambda. Something like this (this incorporates the comments above as well):

Json::Value doAMMInfo(RPC::JsonContext& context) { auto const& params(context.params); Json::Value result; std::shared_ptr<ReadView const> ledger; result = RPC::lookupLedger(ledger, context); if (!ledger) return result; struct ValuesFromContextParams { std::optional<AccountID> accountID; Issue issue1; Issue issue2; std::shared_ptr<SLE const> amm; }; auto getValuesFromContextParams = [&]() -> Expected<ValuesFromContextParams, error_code_i> { std::optional<AccountID> accountID; std::optional<Issue> issue1; std::optional<Issue> issue2; std::optional<uint256> ammID; if (params.isMember(jss::asset) && params.isMember(jss::asset2)) { if (auto const i = getIssue(params[jss::asset], context.j)) issue1 = *i; else return Unexpected(i.error()); if (auto const i = getIssue(params[jss::asset2], context.j)) issue2 = *i; else return Unexpected(i.error()); } if (params.isMember(jss::amm_account)) { auto const id = getAccount(params[jss::amm_account], result); if (!id) return Unexpected(rpcACT_MALFORMED); auto const sle = ledger->read(keylet::account(*id)); if (!sle) return Unexpected(rpcACT_MALFORMED); ammID = sle->getFieldH256(sfAMMID); } if (issue1.has_value() == ammID.has_value()) return Unexpected(rpcINVALID_PARAMS); if (params.isMember(jss::account)) { accountID = getAccount(params[jss::account], result); if (!accountID || !ledger->read(keylet::account(*accountID))) return Unexpected(rpcACT_MALFORMED); } auto const ammKeylet = [&]() { if (issue1 && issue2) return keylet::amm(*issue1, *issue2); assert(ammID); return keylet::amm(*ammID); }(); auto const amm = ledger->read(ammKeylet); if (!amm) return Unexpected(rpcACT_NOT_FOUND); if (!issue1 && !issue2) { issue1 = (*amm)[sfAsset]; issue2 = (*amm)[sfAsset2]; } return ValuesFromContextParams{ accountID, *issue1, *issue2, std::move(amm)}; }; auto r = getValuesFromContextParams(); if (!r) { RPC::inject_error(r.error(), result); return result; } auto& [accountID, issue1, issue2, amm] = *r;

seelabs

Found one minor nit; other than that LGTM. I'll look at Ed's comment's too, of course.

seelabs · 2023-08-31T13:35:04Z

src/ripple/ledger/View.h

@@ -469,8 +478,7 @@ requireAuth(ReadView const& view, Issue const& issue, AccountID const& account);
 cleanupOnAccountDelete(
    ApplyView& view,
    Keylet const& ownerDirKeylet,
-    std::function<TER(LedgerEntryType, uint256 const&, std::shared_ptr<SLE>&)>
-        deleter,
+    EntryDeleter deleter,


This shouldn't be passed by value. Pass by ref instead.

gregtatcam · 2023-08-31T14:02:40Z

Found one minor nit; other than that LGTM. I'll look at Ed's comment's too, of course.

Thanks, will fix. I'll wait with pushing another commit until you review Ed's comments.

ximinez

Other than the ref issue pointed out by @seelabs, and the parameter name question, I think this is good.

ximinez · 2023-08-31T19:23:05Z

src/ripple/app/misc/impl/AMMUtils.cpp

            // Skip AMM
            if (nodeType == LedgerEntryType::ltAMM)
-                return tecOBJECT_NOT_FOUND;
+                return {tesSUCCESS, SkipEntry::Yes};


This is so much better.

src/ripple/rpc/handlers/AMMInfo.cpp

khancode · 2023-09-01T00:37:11Z

@gregtatcam "Update amm_info to fetch AMM object by amm account id instead of the assets pair."

If I'm understanding this correctly, is amm account id replacing asset pair params in amm_info request?

So will this be no longer valid?

{
    "command": "amm_info",
    "asset": {
      "currency": "XRP"
    },
    "asset2": {
      "currency": "TST",
      "issuer": "rP9jPyP5kyvFRb6ZiRghAGw5u8SGAmU4bd"
    }
}

gregtatcam · 2023-09-01T06:51:45Z

@gregtatcam "Update amm_info to fetch AMM object by amm account id instead of the assets pair."

If I'm understanding this correctly, is amm account id replacing asset pair params in amm_info request?

So will this be no longer valid?
{
    "command": "amm_info",
    "asset": {
      "currency": "XRP"
    },
    "asset2": {
      "currency": "TST",
      "issuer": "rP9jPyP5kyvFRb6ZiRghAGw5u8SGAmU4bd"
    }
}

It doesn't replace the asset pair. Either one but not both can be provided. You can either fetch amm info by amm_account or by asset and asset2.

…ombination

seelabs · 2023-09-01T12:51:08Z

src/ripple/rpc/handlers/AMMInfo.cpp

@@ -119,7 +122,8 @@ doAMMInfo(RPC::JsonContext& context)
            ammID = sle->getFieldH256(sfAMMID);
        }

-        if (issue1.has_value() == ammID.has_value())
+        if ((issue1.has_value() ^ issue2.has_value()) ||
+            (issue1.has_value() == ammID.has_value()))


Since this isn't a bitwise operation, I'd prefer != instead of exclusive or (yes, I understand they act the same here). Also, if issue1 has a value and issue2 does not, that's a logic error, not a malformed error. I don't object to the check, but I'd assert and return rpcINTERNAL on that error.

Now that I look, we should probably detect when params.isMember(jss::asset) != params.isMember(jss::asset2) and return malformed on that too. Right now if someone specifies jss::asset, jss::amm_account, and doesn't specify jss::asset2 there's no error.

Isn't it invalid parameters if one issue is set but not the other? It means that either asset or asset2 is included but not both. params.isMember(jss::asset) != params.isMember(jss::asset2) is handled by issue1.has_value() != issue2.has_value().

Since you split up the parsing of jss::asset and jss::asset2, checking here works. However, it would save some effort to check the various params.isMember combinations before trying to parse things and reading from the ledger. So at the top of the lambda:

if ((params.isMember(jss::asset) != params.isMember(jss::asset2)) || (params.isMember(jss::asset) == params.isMember(jss::amm_account))) return Unexpected(rpcINVALID_PARAMS);

Then this check could be replaced with an assert:

assert( (issue1.has_value() == issue2.has_value()) && (issue1.has_value() != ammID.has_value()));

Also, you should add a few unit test cases to check out the various malformed combinations.

seelabs · 2023-09-01T12:52:22Z

src/ripple/ledger/View.h

@@ -478,7 +478,7 @@ using EntryDeleter = std::function<std::pair<TER, SkipEntry>(
 cleanupOnAccountDelete(
    ApplyView& view,
    Keylet const& ownerDirKeylet,
-    EntryDeleter deleter,
+    EntryDeleter const& deleter,


I probably wouldn't have made this const. If the callback wants to change itself (increment some counter say), the interface shouldn't stop that.

It would not compile otherwise.

Ah, because we're binding temporaries. We have to make it a template and use a forwarding ref. Fine as-is for now.

seelabs

👍

ximinez · 2023-09-01T17:04:01Z

src/ripple/rpc/handlers/AMMInfo.cpp

@@ -119,7 +122,8 @@ doAMMInfo(RPC::JsonContext& context)
            ammID = sle->getFieldH256(sfAMMID);
        }

-        if (issue1.has_value() == ammID.has_value())
+        if ((issue1.has_value() ^ issue2.has_value()) ||
+            (issue1.has_value() == ammID.has_value()))


Since you split up the parsing of jss::asset and jss::asset2, checking here works. However, it would save some effort to check the various params.isMember combinations before trying to parse things and reading from the ledger. So at the top of the lambda:

if ((params.isMember(jss::asset) != params.isMember(jss::asset2)) || (params.isMember(jss::asset) == params.isMember(jss::amm_account))) return Unexpected(rpcINVALID_PARAMS);

Then this check could be replaced with an assert:

assert( (issue1.has_value() == issue2.has_value()) && (issue1.has_value() != ammID.has_value()));

Also, you should add a few unit test cases to check out the various malformed combinations.

ximinez · 2023-09-01T17:11:08Z

src/ripple/protocol/jss.h

@@ -160,6 +160,7 @@ JSS(alternatives);           // out: PathRequest, RipplePathFind
 JSS(amendment_blocked);      // out: NetworkOPs
 JSS(amendments);             // in: AccountObjects, out: NetworkOPs
 JSS(amm);                    // out: amm_info
+JSS(amm_account);            // in: amm_info


EDIT: Unless I'm misunderstanding something—What's the difference between account and amm_account in this request?

The way I understand it,

amm_account is the r... address of the account that is part of the AMM itself. It's an alternative to asset and asset2 as a way to look up the AMM.

account is the r... address of (possibly) a liquidity provider for the AMM.

…update-amm_info

- Update amm_info to fetch AMM by amm account id. - This is an additional way to retrieve an AMM object. - Alternatively, AMM can still be fetched by the asset pair as well. - Add owner directory entry for AMM object. Context: - Add back the AMM object directory entry, which was deleted by XRPLF#4626. - This fixes `account_objects` for `amm` type.

Update amm_info to search by amm account and add owner directory for …

fba3388

…AMM object.

gregtatcam requested review from ximinez and seelabs August 30, 2023 20:03

gregtatcam assigned ximinez and seelabs Aug 30, 2023

ximinez requested changes Aug 30, 2023

View reviewed changes

seelabs reviewed Aug 31, 2023

View reviewed changes

Address reviewer's feedback

19ff6ea

seelabs reviewed Aug 31, 2023

View reviewed changes

ximinez reviewed Aug 31, 2023

View reviewed changes

Pass account cleanup deleter by ref and fix amm_info invalid params c…

c293837

…ombination

seelabs reviewed Sep 1, 2023

View reviewed changes

Replace "^" with "!=" in amm_info params validation

529c045

seelabs approved these changes Sep 1, 2023

View reviewed changes

intelliot requested a review from ximinez September 1, 2023 16:01

Merge branch 'develop' into update-amm_info

51de137

intelliot mentioned this pull request Sep 1, 2023

Fix account_objects command for AMM account root #4678

Closed

intelliot unassigned seelabs Sep 1, 2023

ximinez requested changes Sep 1, 2023

View reviewed changes

gregtatcam added 2 commits September 1, 2023 21:46

Refactor amm_info params validation and update amm_info unit-tests.

18a8f62

Merge branch 'update-amm_info' of github.com:gregtatcam/rippled into …

bb4ea59

…update-amm_info

ximinez approved these changes Sep 1, 2023

View reviewed changes

Merge branch 'develop' into update-amm_info

5024941

intelliot merged commit b014b79 into XRPLF:develop Sep 1, 2023

intelliot mentioned this pull request Sep 1, 2023

Proposed 1.12.0-rc4 #4685

Merged

1 task

intelliot added API Change Bug labels Sep 3, 2023

intelliot added Amendment Will Need Documentation labels Sep 3, 2023

intelliot added this to the 1.12 milestone Sep 3, 2023

intelliot added the Clio Reviewed label Sep 8, 2023

intelliot mentioned this pull request Sep 8, 2023

implement amm_info XRPLF/clio#283

Closed

ximinez mentioned this pull request Oct 2, 2023

Invariant: prevent a deleted account from leaving (most) artifacts on the ledger. #4663

Merged

godexsoft mentioned this pull request Oct 9, 2023

Missing amm type filter in account_objects XRPLF/clio#902

Closed

intelliot added the XLS-30 label Oct 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update amm_info to fetch AMM by amm account and add owner directory for AMM object #4682

Update amm_info to fetch AMM by amm account and add owner directory for AMM object #4682

gregtatcam commented Aug 30, 2023 •

edited

Loading

ximinez left a comment

ximinez Aug 30, 2023

gregtatcam Aug 30, 2023

mDuo13 Aug 31, 2023 •

edited

Loading

ximinez Sep 1, 2023

gregtatcam Sep 1, 2023

intelliot Sep 1, 2023

seelabs left a comment

seelabs Aug 31, 2023

seelabs Aug 31, 2023

seelabs Aug 31, 2023

seelabs Aug 31, 2023

seelabs Aug 31, 2023 •

edited

Loading

seelabs left a comment

seelabs Aug 31, 2023

gregtatcam commented Aug 31, 2023

ximinez left a comment

ximinez Aug 31, 2023

khancode commented Sep 1, 2023

gregtatcam commented Sep 1, 2023

seelabs Sep 1, 2023

seelabs Sep 1, 2023 •

edited

Loading

gregtatcam Sep 1, 2023

ximinez Sep 1, 2023

seelabs Sep 1, 2023

gregtatcam Sep 1, 2023

seelabs Sep 1, 2023

seelabs left a comment

ximinez Sep 1, 2023

ximinez Sep 1, 2023


		if (!params.isMember(jss::asset) \|\| !params.isMember(jss::asset2))
		if ((isAssets && isAMMAccount) \|\| (!isAssets && !isAMMAccount))

Update amm_info to fetch AMM by amm account and add owner directory for AMM object #4682

Update amm_info to fetch AMM by amm account and add owner directory for AMM object #4682

Conversation

gregtatcam commented Aug 30, 2023 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

Before / After

Test Plan

ximinez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mDuo13 Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregtatcam commented Aug 31, 2023

ximinez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khancode commented Sep 1, 2023

gregtatcam commented Sep 1, 2023

Choose a reason for hiding this comment

seelabs Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregtatcam commented Aug 30, 2023 •

edited

Loading

mDuo13 Aug 31, 2023 •

edited

Loading

seelabs Aug 31, 2023 •

edited

Loading

seelabs Sep 1, 2023 •

edited

Loading