Automated Market Makers (CAP-38 Version)

[Jonathan Jove]

Hi everyone,

OrbitLens recently published a proposal for automated market makers (see https://groups.google.com/g/stellar-dev/c/Ofb2KXwzva0/m/LLcUKWFmBwAJ for discussion). In order to demonstrate another set of possible design choices, I drafted https://github.com/stellar/stellar-protocol/blob/master/core/cap-0038.md. The two proposals have a lot in common, but there are some substantial differences.

I would appreciate any and all feedback. Love it? Hate it? Does/doesn’t meet your requirements? Do/don’t like the approach? Let me know.

Thanks,
Jon

[Siddharth Suresh]

Re: Pool Shares are not Subject to Clawback

This would let users protect their regulated assets from clawback, which shouldn’t be allowed. An easy “fix” to this would be to not allow clawback enabled trustlines to deposit into a pool. This isn’t ideal as regulated assets wouldn’t benefit from the added liquidity from AMMs. 

To allow clawbacks from a pool, we need the trustlines for the assets to not be deleted while a balance exists in the pool so the clawback flag can be checked. We can use an extension on trustlines to keep track of how many pools this trustline has balances in. Once we have that, we can implement one of the two solutions below as a ClawbackFromPoolOp -

1. On clawback, create a claimable balance for the other asset.
   1. ClawbackFromPoolOp returns balanceID.
   2. Reserve for the claimable balance will be from the account submitting the clawback.
   3. Claimable balance should be clawback enabled if the trustline has it enabled (same logic as CreateClaimableBalanceOp).
   4. We could create a different ledger entry for this instead of using ClaimableBalanceEntry.
2. If clawback is enabled on a trustline, the other asset in the pool must be native so we can send it back to the owner.
   1. For this to work, we’d have to work around the native asset limit due to buying liabilities. It’s not possible for any native balance to exceed INT64_MAX, so working around this limit might be feasible.
   2. This isn’t a great idea since we’d have to allow native buy offers that are technically not tradable. Also, it’s probably not worth doing since this solution is already a compromise, but I think it's worth thinking about.

[Nicolas Barry]

Nice proposal Jon.

I have a few suggestions (some were inspired by OrbitLens' proposal that I just reviewed yesterday).

XDR

 

+struct ClaimLiquidityAtom
+{
+    LiquidityPoolKey pool;
+    int64 amountSold; // amount taken from the pool
+    int64 amountBought; // amount and asset sent to the pool
+};

 

How do we know which asset was sold vs taken?

 

Actually, maybe it would be cleaner/easier for SDKs to have the `AccountID` be the variable as what we're trying to pivot on is where the liquidity is coming from?

 

```

union LiquiditySource switch(LiquidityType type)

{

case LIQUIDITY_TYPE_OFFERV0:

    struct

    {

        uint256 sellerIDEd25519; // Account that owns the offer

        int64 offerID;

    } offerV0;

 

case LIQUIDITY_TYPE_OFFER:

    struct

    {

        AccountID sellerID; // Account that owns the offer

        int64 offerID;

    } offer;

case LIQUIDITY_TYPE_POOL:

    LiquidityPoolKey pool;

};

 

struct ClaimAtom

{

    // emitted to identify the offer

    LiquiditySource source;

 

    // amount and asset taken from the source

    Asset assetSold;

    int64 amountSold;

 

    // amount and asset sent to the source

    Asset assetBought;

    int64 amountBought;

};

```

 

Use of `LiquidityPoolKey`:

In the future I imagine that we'll have

• Other algo/curve choices
• More than 1 "constant product" instance (parameters)

I don't know how many parameters we'll need to identify a pool, but this may get fairly big: there will be the base fee, potentially other things that restrict amounts, time etc.

It seems to me that we should try to make it more efficient by default.

If we assume that pools tend to be "long lived", we can take advantage of it: if we had an explicit "createpool" (that is idempotent) that creates a pool with the full `LiquidityPoolKey`, we can then use a "liquidity pool identifier" everywhere else something like H(LiquidityPoolKey).

If we do that, we could add right away a fee (expressed in basis points for example) in the `LiquidityPoolKey` (which saves us from having to introduce a new complete type just for that later on) - we would only allow to set it to a hard coded constant in the first version (and open it up to more values later on).

 

LiquidityPoolDepositOp

 

Validity:

You're referencing `maxAmountB` but there is only `minAmountB`

 

Apply:

"A" or "B" can be NATIVE (no trustline)

 

You should move the UNDERFUNDED check all the way to the top, that way you'll also check for that condition when creating the pool.

 

Can computations like `amountB = reserveB * amountA / reserveA` overflow int64?

I think the answer is yes, we could error out in this situation, or maybe a better way to deal with this a bit differently: in this situation  we can reduce amounts, but this breaks the contract you were going for (where you rely on the fact that you always fix one amount, and use minimum for the other one). Maybe using what Orbitlens did (use a max error on price) allows to fix this?

 

Use of "sqrt" - I asked the same thing in CAP37 - I don't know if we can guarantee the result using standard libraries. Also if this is using "floating point" like double, is the loss of precision (56 bits mantissa) a problem?

 

LiquidityPoolWithdrawOp

 

Apply:

Same issue than for deposit wrt NATIVE support

 

Is it possible to have amountA or amountB == 0? I suspect yes, we should error out in that case?

 

ChangeTrustOp

 

Should specify limit (max int 64).

 

 

PathPaymentStrictSendOp and PathPaymentStrictReceiveOp

 

I like this interaction model as it's trivial to validate.

 

There is one small gap:

You need to define a strict ordering. Right now it's not specified which liquidity provider wins in case of a tie.

I'd say that order book is better in case of a tie?

For pools, we can leave this for later (as there is only one per asset pair), intuitively I'd say it should be the one that causes the smallest shift in price (favoring larger pools) as to minimize potential arbitrage.

 

Rationale:

 

I like this idea of having the pool ref counted using trustlines. I am fine with no additional reserve requirement in this case (as opposed to using sponsorship).

As to make invariant checking easier, should we add the count to the pool?

 

 

Clawback - we need something there, the proposal doesn't address it (but I saw that Siddharth is working on it)

 

What happens when an account gets its auth revoked?

Right now it looks like nothing happens (funds stay in the pool), but the account cannot withdraw from the pool.

From a framework point of view, this makes it behave like a "BalanceEntry". This may not be enough. In the case of offers, we cancel offers in this situation. What may make more sense is to figure out how to force a withdrawal in this situation. This can then be combined with the normal clawback flows (if available).

--
You received this message because you are subscribed to the Google Groups "Stellar Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to stellar-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/stellar-dev/CAG4FKhxzGFPPYVsQe5GdrRnMBhb6w%2B2ym4ObvKbzVHb0%3DWRNrA%40mail.gmail.com.

[Nicolas Barry]

A few corrections/additions from my response earlier

LiquidityPoolDepositOp

I think a change is needed to ensure that a and b have values consistent with "s" post rounding:

basically the property we're looking for here is that if I deposit A,B -->s and right after withdraw s, I should get A,B back (note that this probably cannot be guaranteed if there are more deposits/swaps)

I think CAP37 suffers from the same problem (maybe worse because `sqrt` is used all the time, and it only has a 56 bit precision).

LiquidityPoolWithdrawOp

amountA = 0 or amountB = 0 should not error.

Consider the following degenerate situation

LP1: deposit 1, 1 --> gets 1 share of newly created pool (1,1)

LP2: deposit 99, 99 --> gets 99 shares of the pool (100, 100)

User: swap 5  --> pool is now out of balance, something like (105, 96)

LP1 should be able to withdraw its 1 share, and get (1,0).

The proposed work around in the rationale to ensure a fair price (deposit in the pool) does not work as the pool price is actually fine.

ChangeTrustOp

Should specify limit (max int 64). ---> no need for this (we can let people specify the limit they want)

Rationale/XDR

I misread the rationale on "unused liquidity pools" (call it brainfart).

I reviewed the semantics like you intended (using pool tokens) but later on interpreted "ref counting" as "trust lines".

This mistake gave me an idea though, so not all is lost :)

If we were creating `LiquidityPoolEntry` when the trustline is created (not on first deposit), and delete it when then last trustline is deleted, this will address my other comment on use of `LiquidityPoolKey`:

Basically the only place where the full ` LiquidityPoolKey` is needed is with `ChangeTrustOp`, everywhere else can use the "liquidity pool identifier" (derived from the hash of the key).

This will "by design" ensure that any operation that uses pool identifiers will work because all those operations would fail anyways if there was no trustline.

There is therefore no assumption on pool's age.

In operation semantics like deposit, the check `if exists(lp)` gets replaced by `if lp.constantProduct().totalPoolShares != 0` (lp is guaranteed to exist if `tlPool` exists)

[Siddharth Suresh]

To Nico's point about auth revocation and forcing a withdrawal - the clawback solution I mentioned earlier using claimable balances can be extended to allow pools to work similar to offers. When authorization is pulled, a claimable balance can be created for both balances. This solution does require the issuer to put up two reserves on revocation, but it's the best way that we can think of for forcing a withdrawal at the moment.

[Jonathan Jove]

Thank you for the feedback Nicolas. These are some responses:

> How do we know which asset was sold vs taken?

Good question, this was an error.

> Actually, maybe it would be cleaner/easier for SDKs to have the `AccountID` be the variable as what we're trying to pivot on is where the liquidity is coming from?

While this would probably be easier for SDKs, it doesn’t maximize future flexibility for the protocol. What if we need to add additional information to a `ClaimAtom` in the future, such as the amount of fee paid? Putting the union at the top enables this.

> You're referencing `maxAmountB` but there is only `minAmountB`

Good catch, I changed the interface but missed fixing this. I’ll fix this section.

> "A" or "B" can be NATIVE (no trustline)

I didn’t want to handle every case in the pseudocode before we agree on what the semantics should be. Once final agreement is reach, I’ll cover all the other cases.

> You should move the UNDERFUNDED check all the way to the top, that way you'll also check for that condition when creating the pool.

I missed the case in the other branch, I’ll fix that. But moving it to the top doesn’t make sense to me, because we don’t know the required amounts in advance. Checking it later handles this more gracefully.

> Can computations like `amountB = reserveB * amountA / reserveA` overflow int64?
> I think the answer is yes, we could error out in this situation, or maybe a better way to deal with this a bit differently: in this situation  we can reduce amounts, but this breaks the contract you were going for (where you rely on the fact that you always fix one amount, and use minimum for the other one). Maybe using what Orbitlens did (use a max error on price) allows to fix this?

Yes, this can overflow int64. This int64 issue permeates this entire proposal, it is not specific to this calculation. For example, the reserves can exceed int64 due to fees collected from exchange. This problem should be handled uniformly in the general case (eg. perhaps use int128 values), not in this specific case.

Orbitlens’ approach is unsafe because the reference price is not provided externally (see https://uniswap.org/docs/v2/smart-contract-integration/trading-from-a-smart-contract/#safety-considerations).

> Use of "sqrt" - I asked the same thing in CAP37 - I don't know if we can guarantee the result using standard libraries. Also if this is using "floating point" like double, is the loss of precision (56 bits mantissa) a problem?

I’m not committed to using sqrt, so if there is an alternative proposal we can discuss it. Regarding exactness of the result (https://en.cppreference.com/w/cpp/numeric/math/sqrt):

"std::sqrt is required by the IEEE standard to be exact. The only other operations required to be exact are the arithmetic operators and the function std::fma. After rounding to the return type (using default rounding mode), the result of std::sqrt is indistinguishable from the infinitely precise result. In other words, the error is less than 0.5 ulp. Other functions, including std::pow, are not so constrained."

This, however, assumes that we only support IEEE floating point numbers.

Regarding precision: same thing as my earlier remark. This issue is prevalent throughout the proposal and should be handled in the general case.

> Same issue than for deposit wrt NATIVE support

Same thing about handling every case once the semantics are finalized.

> Is it possible to have amountA or amountB == 0? I suspect yes, we should error out in that case?

This is possible. I haven’t done a full analysis to determine the consequences when it does happen. I see you provided an interesting case below.

> Should specify limit (max int 64).

No reason to do this (I see you rescinded this in a later comment).

> I'd say that order book is better in case of a tie?

Yes, this is a good approach. There are many things we could do in the multiple pool case. Smallest shift in price isn’t sufficient, because there could be a tie even after that. Probably the right approach is smallest shift in price, and then use pool-type (or identifier) as the final tie breaker.

> As to make invariant checking easier, should we add the count to the pool?

I support this if there is a good reason to do it (eg. if an operation depends on this).

> What happens when an account gets its auth revoked?

Creating two claimable balances is fine by me, it’s what I had discussed with Siddharth.

> basically the property we're looking for here is that if I deposit A,B -->s and right after withdraw s, I should get A,B back

I think this is a good property, but I’m not sure if it’s easy to achieve.

> If we were creating `LiquidityPoolEntry` when the trustline is created (not on first deposit), and delete it when then last trustline is deleted, this will address my other comment on use of `LiquidityPoolKey`:
> Basically the only place where the full ` LiquidityPoolKey` is needed is with `ChangeTrustOp`, everywhere else can use the "liquidity pool identifier" (derived from the hash of the key). This will "by design" ensure that any operation that uses pool identifiers will work because all those operations would fail anyways if there was no trustline.
> There is therefore no assumption on pool's age.

This is a nice proposal. I like this much better than having a separate "create pool" operation.

To view this discussion on the web visit https://groups.google.com/d/msgid/stellar-dev/96c4933f-c947-4ed3-a6ee-b004c35dce2bn%40googlegroups.com.

[Siddharth Suresh]

 

Use of `LiquidityPoolKey`:
In the future I imagine that we'll have

• Other algo/curve choices

• More than 1 "constant product" instance (parameters)

I don't know how many parameters we'll need to identify a pool, but this may get fairly big: there will be the base fee, potentially other things that restrict amounts, time etc.
It seems to me that we should try to make it more efficient by default.
If we assume that pools tend to be "long lived", we can take advantage of it: if we had an explicit "createpool" (that is idempotent) that creates a pool with the full `LiquidityPoolKey`, we can then use a "liquidity pool identifier" everywhere else something like H(LiquidityPoolKey).

 

 

If we do that, we could add right away a fee (expressed in basis points for example) in the `LiquidityPoolKey` (which saves us from having to introduce a new complete type just for that later on) - we would only allow to set it to a hard coded constant in the first version (and open it up to more values later on).

Interesting idea Nico. One of the nice things about this solution is that we can come up with flexible ways to allow other fees. For example, you could say the fee must be between 5 and 105 basis points, in increments of 10 basis points.

Something to think about though is that if we include the fee in the LiquidityPoolKey, we won't be able to update the fee for existing pools. I'm not sure if this is a significant issue or not yet.

Another option is to include the parameters in the LedgerHeader, and have the parameter pulled based on the LiquidityPoolType. This is an extension on what OrbitLens proposed. For example, the new fields in LedgerHeader could be the following (we only need one parameter initially, but I'm adding two more here to demonstrate what they can look like) -

1. constantProductFeeHigh
2. constantProductFeeMid
   
3. constantProductFeeLow
   

Where constantProductFeeHigh > constantProductFeeMid > constantProductFeeLow. We'll also need corresponding LiquidityPoolTypes (The xdr unions that use LiquidityPoolType will have multiple cases resolve to the same object) -

1. LIQUIDITY_POOL_CONSTANT_PRODUCT_HIGH
   
2. LIQUIDITY_POOL_CONSTANT_PRODUCT_MID
   
3. LIQUIDITY_POOL_CONSTANT_PRODUCT_LOW
   

This solution has a few nice properties - 

1. Fees for existing pools are upgradeable without a protocol change.
2. Less pools per asset pair since existing pools don't keep their old fees on fee upgrades. This should lead to less liquidity fragmentation (this point might be up for debate though).
   
3. Additional parameters can be added to LedgerHeader when new curves are added.

There are a few issues with this though -

1. LedgerHeader can get significantly larger depending on what we do in the future.
2. Less user control over fees.

Ultimately, I think the solution we use depends on if we care about updating fees for existing pools, and how many possible pools we want to allow per asset pair. I would imagine that the more pools we add per pair, the more liquidity fragmentation we'll see, so both of these points are relevant.

[Leigh McCulloch]

Hi Jon,

Something missing from CAP-38 is discussion about how to address or identify pool shares in the ecosystem. I think there needs to be some discussion about how upstream services will address and identify pool shares.

I see in the proposal that the address/identity of a pool is:

union TrustLineAsset switch (AssetType type)

{

...

case ASSET_TYPE_POOL_SHARE:

    LiquidityPoolKey poolShare;

};

union LiquidityPoolKey switch (LiquidityPoolType type)
{
case LIQUIDITY_POOL_CONSTANT_PRODUCT:
    struct
    {
        Asset assetA;
        Asset assetB;
    } constantProduct;
};

The way that existing assets are addressed and identified is well established and rather obvious – asset code and issuer, <asset-code>:<asset-issuer> (txrep) or <asset-code>-<asset-issuer> (common in ecosystem as well).

For liquidity pools it seems less obvious to me, since attributes of the pools like their pricing function appear to be part of their identity.

I understand that you intend to add more types of liquidity pools and identifying each by their attributes may become difficult for applications. I think we need to outline how the ecosystem will be impacted and how they will address these pools long term if a pools address/identity is a list of all the pools attributes.

Could the CAP discuss this issue? Could it include a definition of a canonical string representation that could remain consistent even with other future changes to the liquidity pools that are planned after CAP-38? Even if the CAP isn't the right place to define a canonical string representation it would be helpful to plan that out so that we understand how ecosystem applications will adapt to this new asset type.

Thanks,

Leigh

[Jonathan Jove]

Hi Leigh,

This is a good question. Since you posted this question, I opened https://github.com/stellar/stellar-protocol/pull/918. That pull request updates the proposal to take on Nicolas' suggestion that the canonical representation of a liquidity pool and the corresponding pool share should be a hash.

I'm not an expert on ecosystem proposals, so I don't know if this is a solution or a source of new problems. It is a solution in the sense that it uniquely and unambiguously identifies the liquidity pool with something that can be printed as hex. It could be a source of new problems in the sense that it no longer makes it remotely clear what the underlying asset actually is (eg. what's the fee?). For example, knowing the hash isn't enough to create a trust line in the updated proposal (this is because creating the trust line might create the actual liquidity pool, so the parameters need to be fully specified). But of course it would be possible to query Horizon for the underlying LiquidityPoolEntry by hash, and this would then contain all the required information. Does this create new problems? What do you think?

Thanks,

Jon

--

You received this message because you are subscribed to the Google Groups "Stellar Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to stellar-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/stellar-dev/99b87948-cb52-4728-960f-ba2c664bd08dn%40googlegroups.com.

[Leigh McCulloch]

I think hashes were a decent choice for claimable balances because a claimable balance has little meaning or identity beyond the participants of the claimable balance. Liquidity pools don't quite have that same property, but I think a hash still works. If there is any intention to support multiple liquidity pools that have the same attributes it will be necessary to have an opaque identifier, as long as there is still someway to discover liquidity pools and their attributes, which Horizon is best suited for. There could be any number of attributes on future liquidity pools, so I doubt we could conveniently build them into a text value that sufficiently uniquely addresses them. Even type, assets, and fee is difficult.

Will it be possible to evolve the identifier in the future if we want to add multiple liquidity pools that share identical attributes?

[Jonathan Jove]

What do you see as the use case for multiple liquidity pools with identical attributes? I think that this would create liquidity fragmentation with no benefit, but perhaps there is something I don't see.

Regardless, fix n >= 1 and imagine that you have a tuple of attributes A=(A[1], ..., A[n]) which you want to use to characterize pools and you identify them by SHA256(A). If you want to support pools with duplicate (A[1], ..., A[n]), then you can add a dummy attribute A[n+1] forming A'=(A[1], ..., A[n], A[n+1]) and you identify them by SHA256(A'). A[n+1] could be basically anything such as a counter or hash. Using A[n+1]=SHA256(OperationID) would be convenient but would require some thinking about how pool creation works, potentially requiring a new operation.

Does this approach satisfy your concerns?

To view this discussion on the web visit https://groups.google.com/d/msgid/stellar-dev/10da88c3-9501-4b3c-a305-fd80b5684cc8n%40googlegroups.com.

[Siddharth Suresh]

Clawback and Auth Revocation - After some discussion, we think it makes sense to take care of the auth revocation case first, which will then take care of any clawback issues. When auth is revoked, the pool shares will be redeemed and the assets will either be sent back to the owner, or if that isn’t possible, stored in a claimable balance for the owner to claim. Existing clawback operations can be used by the issuer in either scenario.

There are two separate approaches we can take on auth revocation, and each have their pros and cons.

1. Revoke auth on the non-pool trustline and redeem from all corresponding pools -
   1. When a non-pool trustline has its auth revoked, look up all pool trustlines for the account/asset on the trustline that is now deauthorized. LiquidityPoolKeys have multiple assets, so we need a column for each asset in the trustlines table, and an index on each for efficient querying.
   2. Using the LiquidityPoolKeys from the pool trustlines above, load the corresponding LiquidityPoolEntries.
   3. Redeem pool shares for all of the loaded pool trustlines.
   4. This approach will require a custom query and additional columns/indexes in the trustline table. If we add AMM strategies with more than two assets, we will need to add even more columns and indexes. The nice thing about this approach is that it works just like auth revocation does with offers.
2. Revoke auth per pool trustline (redeeming from a single pool) -
   1. Revoking auth on a non-pool trustline will only keep that trustline from depositing/redeeming from any pool. This action alone will not auto-redeem pool shares.
   2. Any issuer of the assets in a pool trustline can revoke authorization for that pool trustline, which will redeem the pool shares. This means that the issuer will need to submit an operation per pool trustline that it wants to redeem.
   3. This approach does not require any additional asset columns in the trustline table or a custom query. It would be much simpler to implement than the first approach. The downside is that it requires the issuer to look up all of the relevant pool trustlines individually, and also be aware of an additional step when forcing a redeem. The individual lookups aren't a big deal since auth revocation should be rare, but the additional step could be an issue if issuers aren't aware of this change.

Siddharth

[Nicolas Barry]

Thanks Siddharth.

I like the simplicity of the second approach, but it might be too much of a breaking change for issuers: right now revoking trust on a trustline is the only thing that people need to do, so in order to stay compliant issuers would have to perform additional logic.

I thought of a third possibility (maybe more controversial as it requires more work upfront).

This topic may deserve its own dedicated thread if people want to explore it more.

Something we discussed in the past was to

 1) get rid of trustline limits, and

 2) not allow issuers to issue more than INT64_MAX

Together they would allow to simplify a bunch of quirks in the protocol over time.

If we had that, revoking auth on a trustline would just trigger withdrawal from all pools and would not need balance entries (as trustlines would always be able to hold balances).

As to implement those new semantics for trustlines, at a high level:

* we would need a new ledger entry (very similar to the pool entry) that tracks the total amount issued for a given asset, this ledger entry would be created when a trustline gets created (and deleted when the last trust line gets deleted)

* we would need to decide what to do with assets with more than INT64_MAX outstanding tokens today, we could flag them as such in the balance entry and not allow them to be traded in pools (or in general) - I think it might be useful to have a flag anyways for assets to disallow trading in pools (or in the DEX) so this would piggyback on that behavior.

Nicolas

To view this discussion on the web visit https://groups.google.com/d/msgid/stellar-dev/d012a47e-5465-4625-ae23-98bf0848a00cn%40googlegroups.com.

[Jonathan Jove]

I'm not very enthusiastic about removing trustline limits because I think they can play an important role in smart contracting applications.

I think simplifying the protocol by restricting features to assets with less than INT64_MAX supply would be helpful in general. Tracking total supply would also help us implement valuable invariants. If we want to discuss this further, we probably should move it into a different thread because there are some immediate questions that come up. For example, how do liabilities work in this setting? Right now an account can have liabilities that exceed the total supply of an asset (eg. I can have INT64_MAX buying liabilities even if the total supply is 1).

Regardless of how we handle liabilities, I don't think restricting supply actually mitigates any issues with Siddharth's proposal 1. You claim, "If we had that, revoking auth on a trustline would just trigger withdrawal from all pools and would not need balance entries (as trustlines would always be able to hold balances)." But this isn't true.

- If we don't change how liabilities work, then I can always configure my trust line to have balance + buyingLiabilities = INT64_MAX even if the supply is less than INT64_MAX.

- If we do change how liabilities work such that liabilities are limited by the available supply, then if available supply > INT64_MAX/2 it is still possible to have scenarios where balance + buyingLiabilities + amount from pool > INT64_MAX.

I don't see an obvious way to circumvent this reality, although I haven't thought too deeply about it. But these scenarios indicate that restricting supply alone is not sufficient to avoid the use of claimable balances.

To view this discussion on the web visit https://groups.google.com/d/msgid/stellar-dev/CABf4b3y1VXifW6GbJ6ccA69Q5uz9ZyZcVHvKBxQbXoR6Yo-mww%40mail.gmail.com.

[Nicolas Barry]

On Tue, Apr 27, 2021 at 12:21 PM Jonathan Jove <j...@stellar.org> wrote:

Regardless of how we handle liabilities, I don't think restricting supply actually mitigates any issues with Siddharth's proposal 1. You claim, "If we had that, revoking auth on a trustline would just trigger withdrawal from all pools and would not need balance entries (as trustlines would always be able to hold balances)." But this isn't true.

- If we don't change how liabilities work, then I can always configure my trust line to have balance + buyingLiabilities = INT64_MAX even if the supply is less than INT64_MAX.

- If we do change how liabilities work such that liabilities are limited by the available supply, then if available supply > INT64_MAX/2 it is still possible to have scenarios where balance + buyingLiabilities + amount from pool > INT64_MAX.

I don't see an obvious way to circumvent this reality, although I haven't thought too deeply about it. But these scenarios indicate that restricting supply alone is not sufficient to avoid the use of claimable balances.

A point of clarification: I wrote this in the context of auth revocation: when it happens, liabilities go to 0 as offers get deleted (unless I am forgetting something about liabilities). So in terms of flow, we would transfer from pools *after* deleting offers.

[Jonathan Jove]

Ah of course. In that context, I think this would work if limits were also removed.

[Jonathan Jove]

Another interesting possibility stems from Nicolas' remark, "I think it might be useful to have a flag anyways for assets to disallow trading in pools (or in the DEX) so this would piggyback on that behavior."

Imagine that we added a flag so issuers can disallow trading in pools. For simplicity, this can be an authorization flag on the issuing account (default false) meaning issuers have to explicitly enable trading in liquidity pools. Then the issue with Siddharth's proposal 2 that "right now revoking trust on a trustline is the only thing that people need to do, so in order to stay compliant issuers would have to perform additional logic" is avoided: enabling trading in liquidity pools means that the issuer understands and accepts the new authorization requirements. Adding this flag also has the nice advantage that it increases backwards compatibility for issuers, and allows them to avoid liquidity pools if they have compliance (or other) concerns.

What do you think about this approach?

[Leigh McCulloch]

I don't have a use case for multiple liquidity pools with identical attributes. I think that was a blind assumption I had. After further thought, I don't see a benefit in having multiple pools with the same attributes. I think the fact that pool creation and destruction are implicit in deposit and withdraw has a few really nice properties, namely there being no race conditions and inconsistent side-effects. Multiple liquidity pools with identical attributes would possibly mess with that, so I'm good with this as is. Thanks!

[Nicolas Barry]

On Wed, Apr 28, 2021 at 7:36 AM Jonathan Jove <j...@stellar.org> wrote:

Another interesting possibility stems from Nicolas' remark, "I think it might be useful to have a flag anyways for assets to disallow trading in pools (or in the DEX) so this would piggyback on that behavior."

Imagine that we added a flag so issuers can disallow trading in pools. For simplicity, this can be an authorization flag on the issuing account (default false) meaning issuers have to explicitly enable trading in liquidity pools. Then the issue with Siddharth's proposal 2 that "right now revoking trust on a trustline is the only thing that people need to do, so in order to stay compliant issuers would have to perform additional logic" is avoided: enabling trading in liquidity pools means that the issuer understands and accepts the new authorization requirements. Adding this flag also has the nice advantage that it increases backwards compatibility for issuers, and allows them to avoid liquidity pools if they have compliance (or other) concerns.

What do you think about this approach?

I like it. This might just work!

(side note for the readers: this is a good example of combining "dumb ideas" into something that after all makes sense, so don't be shy to contribute your own)

There would be no backward compatibility concerns at all if it's opt-in.

Maybe one thing to think about is should we make it opt-in for all assets or just the ones with `AUTH_REQUIRED` set? I like to think as XLM as a degenerate case for a token without `AUTH_REQUIRED`, so this would imply that pools should "just work" for all open assets.

Nicolas

[Markus Paulson-Luna]

Hey all,

Sharing a few thoughts on some of the points I've seen brought up regarding this version of the CAP.

1. I think pool shares should be transferrable.

• It doesn't seem like it would be a significant change to make them transferrable, so I don't really see the point of leaving it for later. Pool shares are incredibly valuable for their utility as a relatively stable collateral asset that still generates yield for the borrower. I know lending isn't big on Stellar right now, but that will hopefully change in the near future, especially with the progress being made on TSS.

• Suggested asset structure:
  struct Pool_Share
  {
  
  Hash liquidityPoolID
  
  };
  
  • The pool hash should work well as an asset identifier since this asset ID can be relatively opaque (it will mostly be handled by lending protocols). Additionally, once mutable pool fees and invariants are added in the future, the hash should still be sufficient to communicate the relevant information. 

2. I agree with the viewpoint that pools shouldn't be required to include XLM as one of their assets.

• XLM is still very volatile. There will definitely be users that would rather serve as a liquidity provider for two stablecoins.
• As mentioned in the CAP, forcing XLM to be a central asset would limit the invariants that these pools can support.
  

3. I like how this proposal allows users to specify the amount of pool shares they want to withdraw. As I mentioned in the other group, it seems strange to force a user to submit two transactions to adjust their pool share.

4. I didn't see the Authorized_to_Maintain_Liabilities flag mentioned in the CAP. A user who is authorized to maintain liabilities should be allowed to redeem pool shares including the controlled asset but not allowed to deposit the controlled asset into a pool. This avoids the vulnerability where a user who is authorized to maintain liabilities deposits the controlled assets in a pool then sends the pool shares to another party for them to redeem, thereby circumventing the flag (this assumes that pool shares will be transferrable at some point).

5. I agree that there isn't a need for deposit timelocks. The main purpose of those is to prevent front-running attacks, but if I'm not mistaken, the SCP randomizes the order in which transactions are applied to the ledger. Since the order is randomized, frontrunning mempool transactions would be unreliable.

6. I actually prefer Siddharth's first solution for dealing with revoked authorization of pool assets.

• While the second solution is cleaner, it doesn't translate well to solving for clawbacks. The first solution could be easily modified to work for clawbacks by redeeming enough pool shares to fulfill the clawback amount and adding a final step that claws back the issued asset after redeeming the pool shares.

7. I'm not convinced that the CAP should avoid interleaved execution.

• Interleaved execution is better for users.
  • Regular users now have to decide whether to buy assets using a manageBuyOffer or a pathPayment. This is probably confusing, even though pathPayments will almost always be the better choice.
  • Market makers now have to incorporate pathPayments into their strategies as they cannot expect their price floor to purchase assets from AMMs that begin trading below the price floor.
• Interleaved execution is important for maximizing AMM capital efficiency. Preventing AMM's from filling orderbook offers reduces the liquidity they add to the ecosystem. Additionally, based on my back-testing, most AMM yield is driven by the AMM filling a large offer as it handles slippage more efficiently than the orderbook does. While under the proposed system, most large offers may take place through pathPayments, it still concerns me that we're consciously limiting AMM's ability to generate yield and improve liquidity.
• I don't think I see the issue with the exchange trading with liquidity pools. Wouldn't this just result in a more efficient market? If the exchange and the AMM are trading with each other, one of them has an inaccurate price. 
• I'm not sure I totally follow the point that interleaved execution should not be allowed due to the Stellar Protocol preventing orders from crossing. The protocol only prevents orders from crossing for a single account, correct? Are you concerned that a user will place an order that crosses the current price of a pool they own shares of? Shouldn't that be a pretty easy error condition to add?
• Given that offers can already be fulfilled by multiple counter offers, would it be possible to handle interleaved execution by
  

1. Calculating the maxCounterAssetWithdrawn for the AMM given the input amount
   
2. Check if the best orderbook offer will fulfill the trade at a better rate
3. Subtract the amount needed to fill the best orderbook offer from the input amount
4. If the best orderbook offer does not provide a better rate, fill the input offer with the AMM.
   
5. Repeat until the input offer has been filled as much as is possible given the current market and liquidity pool conditions.
   
6. Apply input offer.
   

Fair warning, I'm not an expert on Stellar Core Development, so some of my comments may be off-base. Regardless, I hope they help further the discussion.

[Nicolas Barry]

I just reviewed the latest changes from CAP38.

Here are my thoughts.

 

```

+    Hash liquidityPoolID;

```

 

Nit: should typedef to avoid using `Hash` (that way we can avoid comments everywhere `SHA256(…)` to remind people of the definition).

 

```

+union LiquidityPoolEntry switch (LiquidityPoolType type)
+{
+case LIQUIDITY_POOL_CONSTANT_PRODUCT:
+    struct
+    {
+        Hash liquidityPoolID;  // SHA256(LiquidityPoolKey)

```

 

I think this is fine to include this hash here even though it's not technically required.

Technically speaking I think that looking at the proposal, we should be defining this hash as `LiquidityPoolID` and define the key to be this ID.

 

Note that you have a bug in the definition of `LedgerKey` (where you really want to use the Hash):

```

 case OFFER:
@@ -479,6 +542,9 @@ case CLAIMABLE_BALANCE:
     {
         ClaimableBalanceID balanceID;
     } claimableBalance;
+
+case LIQUIDITY_POOL:
+    LiquidityPoolKey liquidityPool;
 };

```

 

Related: I think that the type `LiquidityPoolKey` that contains all the parameters needed to create a pool should be renamed as `LiquidityPoolParameters` to avoid any possible confusion, and moved to Stellar-Transaction.x (as it's the only place where we should use it).

 

Alternatively, something maybe a bit cleaner:

Define in `Stellar-ledger-entries.x`:

```

struct LiquidityPoolConstantProductParams {

        Asset assetA;          // assetA < assetB
        Asset assetB;
        uint32 fee;            // Fee is fee/UINT32_MAX

};

```

And actually use it:

```

union LiquidityPoolEntry switch (LiquidityPoolType type)
{
case LIQUIDITY_POOL_CONSTANT_PRODUCT:
    struct
    {
        LiquidityPoolID id;
        LiquidityPoolConstantProductParams params;

…

```

 

Then, use it again in `Stellar-transactions.x` to define `LiquidityPoolParameters`:

```

union LiquidityPoolParameters switch (LiquidityPoolType type)

{

case LIQUIDITY_POOL_CONSTANT_PRODUCT:
        LiquidityPoolConstantProductParams constantProduct;

…

```

 

 

LiquidityPoolDepositOp

There is mention of `poolEnabled` but it's not defined anywhere.

 

Related: as `AUTH_LIQUIDITY_POOL_ENABLED_FLAG` is only used in the context of "AUTH REQUIRED" assets (I mentioned that before), I imagine that poolEnabled could be defined as `!(AUTH_REQUIRED_FLAG set) ||  (AUTH_LIQUIDITY_POOL_ENABLED_FLAG set)` (which also allows to derive its value if the issuer account does not exist and allows those tokens to be in pools).

 

 

ChangeTrustOp

 

We should define a constant (in `Stellar-transactions.x`) instead of using `12884902 ` directly.

 

It would be clearer to define `LiquidyPoolID` just once instead of repeating how it's computed.

On that topic: we should probably properly domain separate this ID, so adding a new value to `EnvelopeType`.

 

Nit: rules are the same than for other types (it's worded as if rules were different). For example, can't delete a pool trustline if balance != 0.

 

SetTrustLineFlagsOp

 

>  SetTrustLineFlagsOp is the only way for an issuer to redeem it's assets from a Liquidity Pool back to the owner, or a claimable balance for the owner to claim.

 

I can't really parse that sentence and what the intent is. I think it was just trying to explain what happens when auth is revoked.

 

This operation should probably take a `LiquidityPoolID` as a parameter as this is how pool trustlines are identified after they're created? This avoids having to duplicate parameter checking code that should really only exist in ChangeTrustOp.

 

Code doesn't deal with native (I think this is a pending issue for the CAP overall that assumes trustlines everywhere).

 

SetOptions

Missing changes that explain the rules governing changing `AUTH_LIQUIDITY_POOL_ENABLED_FLAG`.

 

 

 

 

[Nicolas Barry]

Thanks Markus, see inline

On Wed, Apr 28, 2021 at 4:09 PM Markus Paulson-Luna <mar...@script3.io> wrote:

1. I think pool shares should be transferrable.

• It doesn't seem like it would be a significant change to make them transferrable, so I don't really see the point of leaving it for later. Pool shares are incredibly valuable for their utility as a relatively stable collateral asset that still generates yield for the borrower. I know lending isn't big on Stellar right now, but that will hopefully change in the near future, especially with the progress being made on TSS.

I agree, the lens I am using while I review those CAPs is:

* what is the minimum set of functionality that we need so that we can get to a first iteration?

* can this be added later?

With that being said, I think that transferrable pool shares probably fall into the "maybe later" category: from a few conversations I had on the topic, the complexity with having them transferable is that it's unclear (for now) on how much more work would be needed to have a compliance story that makes sense (and there are no tangible use cases just yet).

To me those CAPs are the first step in doing work in the DeFi space, I expect a lot more CAPs in the future to allow the network to maximize utility.

 

4. I didn't see the Authorized_to_Maintain_Liabilities flag mentioned in the CAP. A user who is authorized to maintain liabilities should be allowed to redeem pool shares including the controlled asset but not allowed to deposit the controlled asset into a pool. This avoids the vulnerability where a user who is authorized to maintain liabilities deposits the controlled assets in a pool then sends the pool shares to another party for them to redeem, thereby circumventing the flag (this assumes that pool shares will be transferrable at some point).

Agree, this is under specified right now. Siddharth just made some updates, but some more work is needed.

 

7. I'm not convinced that the CAP should avoid interleaved execution.

• Interleaved execution is better for users.
  • Regular users now have to decide whether to buy assets using a manageBuyOffer or a pathPayment. This is probably confusing, even though pathPayments will almost always be the better choice.
  • Market makers now have to incorporate pathPayments into their strategies as they cannot expect their price floor to purchase assets from AMMs that begin trading below the price floor.
• Interleaved execution is important for maximizing AMM capital efficiency. Preventing AMM's from filling orderbook offers reduces the liquidity they add to the ecosystem. Additionally, based on my back-testing, most AMM yield is driven by the AMM filling a large offer as it handles slippage more efficiently than the orderbook does. While under the proposed system, most large offers may take place through pathPayments, it still concerns me that we're consciously limiting AMM's ability to generate yield and improve liquidity.
• I don't think I see the issue with the exchange trading with liquidity pools. Wouldn't this just result in a more efficient market? If the exchange and the AMM are trading with each other, one of them has an inaccurate price. 
• I'm not sure I totally follow the point that interleaved execution should not be allowed due to the Stellar Protocol preventing orders from crossing. The protocol only prevents orders from crossing for a single account, correct? Are you concerned that a user will place an order that crosses the current price of a pool they own shares of? Shouldn't that be a pretty easy error condition to add?
• Given that offers can already be fulfilled by multiple counter offers, would it be possible to handle interleaved execution by
  

1. Calculating the maxCounterAssetWithdrawn for the AMM given the input amount
   
2. Check if the best orderbook offer will fulfill the trade at a better rate
3. Subtract the amount needed to fill the best orderbook offer from the input amount
4. If the best orderbook offer does not provide a better rate, fill the input offer with the AMM.
   
5. Repeat until the input offer has been filled as much as is possible given the current market and liquidity pool conditions.
   
6. Apply input offer.
   

Fair warning, I'm not an expert on Stellar Core Development, so some of my comments may be off-base. Regardless, I hope they help further the discussion.

I see interleaved execution as a premature optimization, and per my previous comment falls into the "can be done later" (the contract without it is the same, so it would be a change "internal" to the protocol).

The analysis needed just on the rounding front to make sure that it works is non trivial and will take a while.

As for issues with crossing: the problem is that the existing DEX cannot deal well with small amounts in certain situations (like trading XLM:BTC).

In particular: if the taker tries to cross against a standing offer, but rounding would cause the error to be too big (relative to the expected price, we don't allow more than 1% error), the order is just cancelled.

You can read https://github.com/stellar/stellar-protocol/blob/master/core/cap-0004.md#specification that talks about this.

This is fine if only trading against the DEX, but if this is something that happens in the context of interleaved execution, it means that that "small amount" has to either be dropped from the trade (but then the interleaving price may be worst than just trading against a pool) or traded against the pool (which may violate price rules expected by interleaving as this could leave the pool price "crossed" with the DEX).

Both of these things are complicated further if we introduce different types of pools (and I'm pretty sure we want to allow that) where computing the right way to interleave gets really complicated (CAP38 explains this pretty well), as it gets closer to a knapsack optimization problem.

HTH

Nicolas

To view this discussion on the web visit https://groups.google.com/d/msgid/stellar-dev/063eb5a3-cde1-4f27-b1c4-52a56662f944n%40googlegroups.com.

[Markus Paulson-Luna]

Thanks for addressing my concerns Nicolas,

I can get behind focusing on implementing the minimum set of functionalities first and adding on more features in future CAPs. The currently proposed system should still be very valuable to the ecosystem. I'm excited to see where this proposal goes!

Markus

[Siddharth Suresh]

Re: Claimable balances on auth revocation

When authorization is revoked and pool shares are redeemed in the current proposal, claimable balances will be created if the balance cannot be sent back to the owner (this can happen due to a few reasons like hitting limits or deleted trustlines). One question in regards to this mechanism that we are still considering is who puts up the reserves for the claimable balances? In the current proposal, the issuer will have to put up the reserves on auth revocation, but this isn’t ideal as it’s possible for the owner to never redeem the balance.

An idea that Jon had was to increase the number of base reserves for a pool trustline to two. On revocation, the pool shares are redeemed and the trustline is deleted. The owner now has two available base reserves to sponsor the claimable balances. Now that the issuer doesn’t have the responsibility of sponsoring many claimable balances, we can also get rid of the best effort logic where we first try to send the balance back, and then create the claimable balance if that doesn’t work, and instead always create the claimable balance. This will make the effects of auth revocation easier to understand downstream.  

There are some minor details to work out (like what happens if the owner is already sponsoring UINT32_MAX entries or if the cost of a base reserve increases), but this approach is better as the owner has an incentive to claim the balance they are sponsoring.

Siddharth

[Nicolas Barry]

Thanks Siddharth and Jon for the latest update on CAP38.

Here are the notes I took during my review today.

# Carry over from my previous review:

 

## Pool ID definition

 

There should be a typedef to avoid using `Hash` everywhere a pool ID is expected (that way we can avoid comments everywhere `SHA256(…)` to remind people of the definition and it makes it less ambiguous on where the hash is coming from).

 

So something like

```

typedef PoolID Hash; // SHA256(LiquidityPoolKey)

```

 

## Pool key/parameters

 

I think that the type `LiquidityPoolKey` that contains all the parameters needed to create a pool should be renamed as `LiquidityPoolParameters` to avoid any possible confusion, and moved to Stellar-Transaction.x (as it's the only place where we should use it).

 

Alternatively, something maybe a bit cleaner:

Define in `Stellar-ledger-entries.x`:

```

struct LiquidityPoolConstantProductParams {

        Asset assetA;          // assetA < assetB
        Asset assetB;
        uint32 fee;            // Fee is fee/UINT32_MAX

};

```

And actually use it:

```

union LiquidityPoolEntry switch (LiquidityPoolType type)
{
case LIQUIDITY_POOL_CONSTANT_PRODUCT:
    struct
    {
        LiquidityPoolID id;
        LiquidityPoolConstantProductParams params;

…

```

 

Then, use it again in `Stellar-transactions.x` to define `LiquidityPoolParameters`:

```

union LiquidityPoolParameters switch (LiquidityPoolType type)

{

case LIQUIDITY_POOL_CONSTANT_PRODUCT:
        LiquidityPoolConstantProductParams constantProduct;

…

```

 

## Use of constants

 

We should define a constant (in `Stellar-transactions.x`) instead of using `12884902 ` directly.

 

 

# new questions

 

## Fee representation

Fee defined as `uint32 fee;   // Fee is fee/UINT32_MAX`

Any reason to define it like this given that fees are defined by humans and that there is already a convention used to express fees?

We could avoid rounding error on the fee by just defining the fee as:

```

int32 feeBasisPoints;   // Fee is fee/10000

```

(CAP37 was doing something like that too before switching to dynamic fees)

 

## LiquidityPoolEntry

 

`numTrustLines` could benefit from a name that is less ambiguous (we're talking pool shares trust lines here), so maybe rename to `poolSharesTrustLineCount` (that works well with the previous field `

totalPoolShares`).

 

## ClaimableBalanceID change

There is no need to add

```

union ClaimableBalanceID switch (ClaimableBalanceIDType type)

 {

 case CLAIMABLE_BALANCE_ID_TYPE_V0:

     Hash v0;

+case CLAIMABLE_BALANCE_ID_TYPE_FROM_POOL_REVOKE:

+    Hash fromPoolRevoke;

 };

 

```

 

This extension point is there in case we want to use something other than a Hash to identify a `ClaimableBalance`.

Hash collision is already  taken care of by introducing a new `OperationID`.

 

## Trustline changes

 

`numPoolsInvolved` is not a great name (I had to lookup what was the meaning in the CAP). Maybe something more direct like `liquidityPoolUseCount`?

Related, grep for "involved" in the rest of the doc and use something more precise/neutral like "referenced" or "used".

 

## "Upgrade flags"

 

`MASK_UPGRADE_FLAGS` is not a great name as it lives in the `LedgerHeader` and does not refer to an upgrade.

As the field is called "flags" the mask and enum should be derived from "Ledger header flags".

So `MASK_LEDGERHEADER_FLAGS` and `LedgerHeaderFlags` for the enum.

Also, consider renaming `DISABLE_LIQUIDITY_POOL_TRADING` to `DISABLE_LIQUIDITY_POOL_TRADING_FLAG` (consistent with how flags are named elsewhere).

 

## Semantics

 

### General readability

 

I think sections here are well detailed - what is missing in each is a "mini intro" of sorts that explains in one sentence what the changes are and (if possible) invariants.

 

For example, `ChangeTrustOp` should start with "This is the only operation that creates `LiquidityPoolEntry`". Also as other operations need pools to exist, it's probably a good idea to start with this operation.

 

### `ChangeTrustOp`

 

Using "trust line" makes the logic a little harder to follow, it would make things simpler to read if they were specified like "pool shares trust line".

 

The rules applied during creation and deletion of the pool trust line are mixed/interleaved. It would be much better to break things up under different `if` blocks.

 

### Upgrades

 

(this section should really be called "ledger header flags"")

 

Should you also add a flag to stop deposits? If there are rounding bugs in the deposit/withdraw flow, a bad actor could perform a many deposit + withdraw in rapid succession in order to take advantage of the bug (this is true of any AMM implementation).

 

For the later, this would imply adding a new error code to "deposit" that the operation is not possible somehow.

To view this discussion on the web visit https://groups.google.com/d/msgid/stellar-dev/206ead11-fa60-45f5-9301-140b0786407fn%40googlegroups.com.

[Jonathan Jove]

Thanks for the response.

> I think that the type `LiquidityPoolKey` that contains all the parameters needed to create a pool should be renamed as `LiquidityPoolParameters` to avoid any possible confusion, and moved to Stellar-Transaction.x (as it's the only place where we should use it).

Good catch here, the current structure reflects an old model where we weren't identifying pools by hash. We'll fix this.

> We should define a constant (in `Stellar-transactions.x`) instead of using `12884902 ` directly.

Definitely. Also, the fee should be int32 instead.

> Any reason to define it like this given that fees are defined by humans and that there is already a convention used to express fees?

We can do that if you prefer. It has some advantages.

> This extension point is there in case we want to use something other than a Hash to identify a `ClaimableBalance`.

I actually proposed adding this for clarity rather than collision resistance: with this it would be obvious why a ClaimableBalance exists, allowing an easy distinction between "payments" and "returned funds".

> General readability
I'll do some work on this.

--------
I'll let Siddharth respond to some of the other things since he is the most recent author on those.

To view this discussion on the web visit https://groups.google.com/d/msgid/stellar-dev/CABf4b3wZc6Mdb8fnhDJYRMEh%2BFuzT6mpZtZ3PKzOyZEmk6h1RQ%40mail.gmail.com.

[Siddharth Suresh]

Thanks for taking a look Nico.

 

## Pool ID definition

 

There should be a typedef to avoid using `Hash` everywhere a pool ID is expected (that way we can avoid comments everywhere `SHA256(…)` to remind people of the definition and it makes it less ambiguous on where the hash is coming from).

 

So something like

```

typedef PoolID Hash; // SHA256(LiquidityPoolKey)

```

 

## LiquidityPoolEntry

 

`numTrustLines` could benefit from a name that is less ambiguous (we're talking pool shares trust lines here), so maybe rename to `poolSharesTrustLineCount` (that works well with the previous field `

totalPoolShares`).

 

 ## Trustline changes

 

`numPoolsInvolved` is not a great name (I had to lookup what was the meaning in the CAP). Maybe something more direct like `liquidityPoolUseCount`?

Related, grep for "involved" in the rest of the doc and use something more precise/neutral like "referenced" or "used".

 ## "Upgrade flags"

 

`MASK_UPGRADE_FLAGS` is not a great name as it lives in the `LedgerHeader` and does not refer to an upgrade.

As the field is called "flags" the mask and enum should be derived from "Ledger header flags".

So `MASK_LEDGERHEADER_FLAGS` and `LedgerHeaderFlags` for the enum.

Also, consider renaming `DISABLE_LIQUIDITY_POOL_TRADING` to `DISABLE_LIQUIDITY_POOL_TRADING_FLAG` (consistent with how flags are named elsewhere).

### `ChangeTrustOp`
 
Using "trust line" makes the logic a little harder to follow, it would make things simpler to read if they were specified like "pool shares trust line".
 
The rules applied during creation and deletion of the pool trust line are mixed/interleaved. It would be much better to break things up under different `if` blocks.

I agree with the points above and will make the appropriate changes.

### Upgrades
 
(this section should really be called "ledger header flags"")
 
Should you also add a flag to stop deposits? If there are rounding bugs in the deposit/withdraw flow, a bad actor could perform a many deposit + withdraw in rapid succession in order to take advantage of the bug (this is true of any AMM implementation).
 
For the later, this would imply adding a new error code to "deposit" that the operation is not possible somehow.

Yeah this sounds like a good idea and is easy to do. I'll update the CAP for this as well.

Siddharth