Skip to content

Conversation

@Brechtpd
Copy link
Contributor

https://github.com/Loopring/team/issues/163

Most important practical changes:

  • AMM weights are now used as the virtual balances. The virtual balances define the AMM curve. The circuits have been modified to remove the weight functionality. The only other change to the circuits is that AMM always needs to be enabled when the virtual balances of tokenS/tokenB are non-zero. The trade circuit now also updates the virtual balances by sbutracting/adding fillS/fillB just like the balances.
  • AMM pools now always needs ending AMM updates for joins/exits which update the virtual balances as needed.
  • It's possible to withdraw/deposit funds stored in the pool to L1 without changing the curve.

For the AMM pools the following was added:

  • Some AMM pool logic is moved to an IAmmController which can implement custom virtual balance logic and can shut down the pool if necessary (needed for hybrid AMM pools which doesn't provide pool liquidity tokens itself).
  • New AMM pool operation that allows setting the virtual balances, as defined by the IAssetManager.
  • New AMM pool operations deposit/withdraw which allows the exchange owner to move funds between L1<->L2. Funds are stored in the pool contract on L1 and can be withdrawn to the IAssetManager which can use them productively.

So I think all existing pools and all new amplified pools can use the same IAssetManager and IAmmController contracts, so only a single implementation contract needs to be deployed. After updating the pool contracts with 3.8 the pool virtual balances need to be set to the correct values using the set virtual balances operation of the pool contract.

These pool settings are not stored in the AMM pool as storage but as immutable variables baked into the contract because the settings will be shared between many pools:

  • For all non-hybrid pools there will likely be only a single IAssetManager which can handle all funds of all pools together (e.g. put all ETH of all pools into AAVE). I think it doesn't make sense to do this for each pool separately, funds of each pool are moved to this IAssetManager contract so L1 investing costs are shared.
  • All pools of a certain kind can reuse the same IAmmController, making it easy and cheap to do updates to all pools depending on the contract if necessary. Because the logic implemented in the controller contract is rarely used (not for trades/joins/exits) the small extra overhead of doing the logic in an external contract is insignificant. Doing the logic in an external contract also makes it easier for using new data in storage without having to worry about different storage layouts in the pool contract implementations.

@Brechtpd Brechtpd requested a review from dong77 May 22, 2021 21:25
@dong77 dong77 changed the title [Protocol 3.8] AMM v2 [Protocol 3.8] AMM v2 - Mirage May 24, 2021
@dong77 dong77 requested review from kongliangzhong and yueawang May 24, 2021 13:50
Copy link
Contributor

@dong77 dong77 left a comment

Choose a reason for hiding this comment

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

Brecht, could you share your simulation scripts so the team can run the program for better understanding of how the trades/joins/exists work?

@Brechtpd
Copy link
Contributor Author

Brecht, could you share your simulation scripts so the team can run the program for better understanding of how the trades/joins/exists work?

All code I have used is available here: https://github.com/Loopring/team/issues/163

@dong77

This comment has been minimized.

dong77
dong77 previously approved these changes May 31, 2021
@dong77 dong77 self-requested a review May 31, 2021 07:08
@Brechtpd
Copy link
Contributor Author

Brechtpd commented May 31, 2021

We need to handle the following scenario: Alice (as the only user) joins a pool, 50% of pool's assets are withdrawn to L1 and transferred out to the asset manager. Now Alice wants to exit the pool. My understanding is that Alice's exit will fail if the amount is > 50%, is that right?. (otherwise, if Alice can exit with all her LP, then she transferred his risk to other people on L2).

That's correct. If the operator doesn't transfer the necessary funds to L2 to do the exit, Alice will have to withdraw her LP tokens and force the exit on L1 which forced the operator to make the necessary funds available on L2 at the risk of shutting down the pool if he still refuses. If he still refuses the pool shuts down and all funds can be withdrawn to the pool on L1 without the help of the operator.

@Brechtpd
Copy link
Contributor Author

#2445

@dong77 dong77 requested a review from kongliangzhong June 1, 2021 04:31
@dong77
Copy link
Contributor

dong77 commented Jun 1, 2021

Brecht, I think we are over-designing the L1 liquidity feature and making things unnecessarily complicated with the current approach.

Assuming there is an AssetManager contract on L1 for managing some users' assets. The AssetManager can be designed to allow Loopring Exchange owner to make capital calls to withdraw some assets from L1 defi and automatically deposit these assets to L2 to join a predetermined Amm pool (Capital returns can also be done by the exchange owner on L2 to return funds to AssetManager and later trigger an L1 DeFi deposit). The assets in the same AssetManager can also be called for multiple pools. I think what we need to add on top of Loopring 3.6 is simply a way to deposit-then-join a pool from L1 for contract addresses with on-chain approvals. In other words, from an AMM pool's perspective, the AssetManager address is a regular address, there is no need to handle its deposit/withdrawals/joins/exits differently.

What makes such AssetManager interesting is its authorization to Loopring exchange for using its assets with a higher priority than the L1 DeFi apps.

@Brechtpd
Copy link
Contributor Author

Brechtpd commented Jun 1, 2021

I think that may work (though I have some uncertainties for people that join the pool directly). If this works than the benefit of this method is that it would be opt-in if or even in which defi app the user wants to store his funds (could also be a downside that it is opt-in and probably more expensive from L2). Moves some extra complexity to the AssetManager to manage user funds instead of just working on the pool level, but that seems fine. It seems like a more significant change compared to the system in this PR which works exactly the same way as it works now for all users, but a caspian style system would also do more on L1 so I guess that works out.

I'm still thinking about the details how to make it work as expected. Not sure yet about all the details, especially if users are still able to join directly as well). Let's say the 90% of the funds come from the AssetManager, 10% of the funds come from users on L2.

  • We don't want to put funds in the pool if not necessary, so if that 10% is enough for the current price we'll have to change the curve to track AF * (balanceL1 + balanceL2) to remain capital efficient.
  • Users that joined the pool directly on L2 need to be able to exit/join at the expected ratio, but because we faked the curve it looks like we'd have to query the AssetManager to be able to do so (similar to how we need to check balancesL1 now).
  • Price has moved so that one of the balances is 0. That means we now have 0 tokens of tokenA, and a lot of tokens of tokenB.
  • We have no tokens of tokenA, so we'll want to deposit more of these tokens to the pool. However, we have a lot of tokenB tokens that we'll want to withdraw some of those back to L1. I'm not sure if this can be done easily just with joins/exits, especially because the LP tokens need to keep working as expected in the process.
  • ...

I think the main idea is pretty close to the caspian design:

  • People put money in a contract on L1 saying how they want it to be used in an AMM.
  • All the bookkeeping is done in this contract. Funds are stored in defi, but also partially in the AMM pools on L2.
  • Joins/exits on the pool level are not necessary because they will be only be handled on L1 and just setting the virtual balances is enough to update the curve on L2.
  • Current method: Funds are moved between L1 and L2 using deposits/withdrawals without any issue because joins/exits from L2 are not allowed in these kinds of pools (this is where the joinsDisabled parameter comes in to support exactly this).

The difference with your proposal is that there would still be users that have joined from L2 so that probably complicates things.

Brecht, I think we are over-designing the L1 liquidity feature and making things unnecessarily complicated with the current approach.

Not sure the current method is really that complicated because it really is just withdrawing/depositing from the pool, the joins/exit logic just made it bit more complex than expected (I don't think needing to track the balances on L1 is that bad...). Depending on how we want the AssetManager to work it would shift more complexity there which I think will be comparable.

I think what we need to add on top of Loopring 3.6 is simply a way to deposit-then-join a pool from L1 for contract addresses with on-chain approvals. In other words, from an AMM pool's perspective, the AssetManager address is a regular address, there is no need to handle its deposit/withdrawals/joins/exits differently.

Joins/exits from contracts are possible using funds on L2.

EDIT: If we don't allow joins/exits on the pool directly we can implement a uniswap v3 style tick system on the AssetManager level which would be quite powerful because it would allow us to do everything Uniswap v3 does (allowing users to specify custom price ranges etc... while still only needing a single pool), but even with higher capital efficiency built-in because we can also automatically put unused AMM assets (assets not used by the current tick) in a defi app. So trading fees for assets on L2 in the current tick, defi yields for all other assets on L1. I wouldn't be surprised if some project isn't already working on something like this for Uniswap v3. Anyway, just a thought and I guess that would be even more complex to implement.

* [AMM] Fix pools with active asset manager

* Feedback

* Amm v2 - Exit mode and pool reuse (#2450)

* AMM v2 - minor improvement over brecht's code (#2458)

Co-authored-by: Daniel Wang <[email protected]>
dong77
dong77 previously approved these changes Jun 5, 2021
@dong77 dong77 self-requested a review June 7, 2021 01:52
dong77
dong77 previously approved these changes Jun 7, 2021
Copy link
Contributor

@dong77 dong77 left a comment

Choose a reason for hiding this comment

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

very minor change suggestions.

@Brechtpd Brechtpd merged commit e321b6c into master Jun 8, 2021
@Brechtpd Brechtpd deleted the amm_v2 branch June 8, 2021 15:00
@dong77 dong77 restored the amm_v2 branch June 16, 2021 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants