Fix account_objects command for AMM account root

[gregtatcam]

High Level Overview of Change

Fix account_objects for AMM account root to retrieve related AMM object and the trustlines.

Context of Change

This is a bug, which was introduced in #4626.

Type of Change

• [ x] Bug fix (non-breaking change which fixes an issue)

Before / After

Prior to #4626, AMM root account and AMM object were linked via the owner directory entry, which enabled transparent integration with account_object command. #4626 removed the owner directory entry and added AMMID to the account root. This fix fetches AMM object via AMMID, which is AMM object key. Since there could be the truslines associated with AMM root account, the limit is adjusted for AMM object and the marker set to AMM if the limit is 1.

Test Plan

AccountObjects unit-test is extended to verify the above logic.

[mDuo13]

I'm mildly against adding special cases here. Either the AMM should be in the owner directory, or account_objects should not return it. I think it's a weird quirk to artificially add an AMM to the account_objects response.

I would be more OK with adding error cases to the account_objects API method to return invalidParams when filtering by a type that can't be owned by an account.

Maybe this is hypocritical of me, but I'd be more OK with adding a special case error message directing people toward the right way to look up an AMM than I would with adding a special case to actually return the AMM even though it's not "owned" in the typical sense.

I'm willing to be convinced that this special case is warranted because AMM accounts are weird in a bunch of ways anyway, but, yeah.

[gregtatcam]

I'm mildly against adding special cases here. Either the AMM should be in the owner directory, or account_objects should not return it. I think it's a weird quirk to artificially add an AMM to the account_objects response.

I would be more OK with adding error cases to the account_objects API method to return invalidParams when filtering by a type that can't be owned by an account.

Maybe this is hypocritical of me, but I'd be more OK with adding a special case error message directing people toward the right way to look up an AMM than I would with adding a special case to actually return the AMM even though it's not "owned" in the typical sense.

I'm willing to be convinced that this special case is warranted because AMM accounts are weird in a bunch of ways anyway, but, yeah.

Isn't NFT handled as a special case?

Is the ownership criteria purely based on the directory entry? The AMM directory entry is only used for the purpose of supporting account_objects. AMM object is intrinsic part of AMM account root whether there is a directory entry for it or not. I'm not a big fan of special cases either, but AMM is a special case and is handled as such in many places.

[MaestroLegato]

I don't have strong opinion regarding treating/not treating it as special case in rippled, as I'm not well versed in rippled codebase.
However, from the end-user perspective it has benefit.
Suppose you have a list of LP tokens and you want to figure out based on their issuers (AMM pools) which token pairs they are related to. Real world example - user with 100 unknown LP tokens in their balance connects to your application.

- If AMM object can be accessed via account_objects request

only 1 request is needed to get AMM data.

{
	"method": "account_objects",
	"params": [
		{
			"account": "rfgqMq9M2MXc7PjQqjFLomrFSg4AwQ35z2",
			"ledger_index": "current",
			"type": "amm"
		}
	]
}

---

- If account_objects will not be able to retrieve AMM object

2 requests are required

1. account_info to get AMMID
2. ledger_entry with index of AMMID

---

Considering clients might do such requests over an big array of LP tokens - in terms of efficiency, rippled will have to handle less requests with this change.
So this is the case of providing better client flexibility over increasing complexity in rippled

[seelabs]

I like declaring variables in if statements for simple cases where it saves polluting the parent scope with extra variables. Here I think the code is clearer if the variables are declared in the parent scope (which is already small). I'd also remove the == using256{} construct. Something like:

    if (!dirIndex.signum() && (!typeFilter.has_value() || filterAMM))
    {
        auto const sle = ledger.read(keylet::account(account));
        if (!sle)
            return false;
        bool const isAMM = sle->isFieldPresent(sfAMMID);
        if (filterAMM && !isAMM)
            return false;
        if (isAMM)
        {
            auto const sleAMM =
                ledger.read(keylet::amm(sle->getFieldH256(sfAMMID)));
            if (!sleAMM)
                return false;

            jvObjects.append(sleAMM->getJson(JsonOptions::none));

            if (filterAMM)
                return true;
            if (limit == 1)
            {
                jvResult[jss::limit] = limit;
                jvResult[jss::marker] = "AMM";
                return true;
            }
            if (limit > 1)
                --mlimit;
        }

[mDuo13]

@gregtatcam

Isn't NFT handled as a special case?

I don't think NFTs are handled as a special case in account_objects. There is a separate account_nfts method, which is implemented in the same AccountObjects.cpp source file instead of on its own for some reason, but it's not a special case in account_objects.

Is the ownership criteria purely based on the directory entry? The AMM directory entry is only used for the purpose of supporting account_objects. AMM object is intrinsic part of AMM account root whether there is a directory entry for it or not. I'm not a big fan of special cases either, but AMM is a special case and is handled as such in many places.

It's true that the AMM stuff is a special case regardless. But I do think it's more confusing to have this complicated logic in account_objects to support this idea of, "Well, the AMM account sort of owns the AMM entry, and it isn't tracked like how other owned objects are, but we return it in the API as if it were." This is purely my own opinion, but I'd argue that the AMM AccountRoot does not own the AMM entry because (a) it's not paying a reserve for it, and (b) they're inseparable; you can't delete one and have the other still be valid. As you say, one is an intrinsic part of the other. Now, not everything in an account's owner directory is owned by that account—it might be owned by another account and tied to it—so it would still be valid to put the AMM entry in there if we were going to do that. But in terms of how the API should behave, I'd rather leave account_objects "dumber" and make changes elsewhere.

I suggest an alternative approach for the situation @MaestroLegato describes: extend the amm_info API method such that you can provide a (new) lp_token request parameter or (the existing) asset+asset2 parameters. (Any other combination would be an invalidParams error.) If you provide lp_token, then the response gives you the AMM info for the issuer of the LP token. (If the specified token isn't an LP token, the method would throw a different API method, maybe invalidLPToken.) This solves the issue of looking up an AMM in one call from the client side, without iterating the owner directory and without having to wonder why the API returns an object that isn't in the raw DirectoryNode ledger entry.

[gregtatcam]

I suggest an alternative approach for the situation @MaestroLegato describes: extend the amm_info API method such that you can provide a (new) lp_token request parameter or (the existing) asset+asset2 parameters. (Any other combination would be an invalidParams error.) If you provide lp_token, then the response gives you the AMM info for the issuer of the LP token. (If the specified token isn't an LP token, the method would throw a different API method, maybe invalidLPToken.) This solves the issue of looking up an AMM in one call from the client side, without iterating the owner directory and without having to wonder why the API returns an object that isn't in the raw DirectoryNode ledger entry.

This sounds reasonable to me. Why use lp_token though and not simply account?

[gregtatcam]

I suggest an alternative approach for the situation @MaestroLegato describes: extend the amm_info API method such that you can provide a (new) lp_token request parameter or (the existing) asset+asset2 parameters. (Any other combination would be an invalidParams error.) If you provide lp_token, then the response gives you the AMM info for the issuer of the LP token. (If the specified token isn't an LP token, the method would throw a different API method, maybe invalidLPToken.) This solves the issue of looking up an AMM in one call from the client side, without iterating the owner directory and without having to wonder why the API returns an object that isn't in the raw DirectoryNode ledger entry.

This sounds reasonable to me. Why use lp_token though and not simply account?

We still have to handle amm in account_objects as a special case because we need to keep amm in chooseLedgerEntryType() to support ledger_entry API. Have to fail account_objects if amm filter is set.

[MaestroLegato]

I suggest an alternative approach for the situation @MaestroLegato describes: extend the amm_info API method such that you can provide a (new) lp_token request parameter or (the existing) asset+asset2 parameters. (Any other combination would be an invalidParams error.) If you provide lp_token, then the response gives you the AMM info for the issuer of the LP token. (If the specified token isn't an LP token, the method would throw a different API method, maybe invalidLPToken.) This solves the issue of looking up an AMM in one call from the client side, without iterating the owner directory and without having to wonder why the API returns an object that isn't in the raw DirectoryNode ledger entry.

This sounds reasonable to me. Why use lp_token though and not simply account?

yeah I agree account should be enough.
Since 1 AMM account can only issue 1 LP token currency - the currency of LP token is not really relevant.
You could accept lp_token as input and just ignore it's currency, but account is cleaner in my opinion.

[gregtatcam]

@mDuo13 Maybe we should just add back the owner directory for AMM? Could still add enhanced amm_info.

[gregtatcam]

Closing in favor of another PR, which is going to add the owner directory back in and add amm account to amm_info.

[intelliot]

Closed in favor of #4682