Skip to content

Conversation

Brechtpd
Copy link
Contributor

@Brechtpd Brechtpd commented May 31, 2021

#2432

Okay, I implemented this in a way that doesn't work twice now, so hopefully the third time is the charm.

So the main problem is that joins/exits need to work on the actual balances in the pool, which (if an AssetManager is set) could be both on L2 and L1.

The other problem is to correctly implement this, because L1 transactions like deposit/withdraw and L2 transactions do not automatically sync up correctly because of a number of reasons. For example, let's say we process 3 pool transactions in a block:

  1. An exit
  2. A withdrawal
  3. Another exit

What are the pool balances? For 1) it would be the balances as reported on L2. For 3) however it would be the balances on L2 + the newly withdrawn funds in 2). But the funds withdrawn in 2) could either be already in the pool contract (when the block is submitted before the callback) or not yet in the pool contract (the block is submitted after the callback). So only depending on the actual token balances on L1 is not reliable because we do not know what the balances are for each L2 transaction because it depends on when withdrawals/deposits are handled on L2. The same problem is true for deposits (and could be even worse because the deposit may keep on pending).

And so I think the only way to do this correctly is to keep track of the balances deposited/withdrawn to/from the pool in exactly the same order they are done on L2. For that we use mapping (address => uint96) balancesL1; and so we just need to add this value to the L2 balances to get the correct balance in the pool. Because these values need to be updated next to their L2 counterparts deposits are now also required to be done with the PoolTxType.DEPOSIT pool instruction.

The pool contracts will only watch the balancesL1 values. The AssetManager is allowed to update these values when required. It's not really possible to have fully dynamic L1 balances (e.g. always up to date with latest interest) because these values need to be deterministic when the operator processes joins/exits. And so by just updating the balancesL1 at certain expected intervals also makes sure the relayer can count on them being the same when needed.

To make the deposits work as other L2 transactions on L2 (do a deposit and also see the transaction the block in the pool contract) I have made it possible again to do the txReceiverCallbacks before the block is submitted again.

I think this works correctly now, but I'll have to think about it some more to make sure.

@Brechtpd Brechtpd requested a review from dong77 May 31, 2021 22:17
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.

Is the new idea keeping track of tokens that have been successfully deposited back to L2 (substracting balancesL1) and withdrawn to L1 (adding to balancesL1) and also allowing the AssetManager to refresh balancesL1 based on L1 DeFi data?

It seems the new bulk work is to processing pool deposits and withdrawals, correct?

@Brechtpd
Copy link
Contributor Author

Brechtpd commented Jun 1, 2021

Is the new idea keeping track of tokens that have been successfully deposited back to L2 (substracting balancesL1) and withdrawn to L1 (adding to balancesL1) and also allowing the AssetManager to refresh balancesL1 based on L1 DeFi data?

Yes exactly. Keeping track of those balances on L1 in exactly the same way the balances change on L2. And tI guess it should be even easier for the relayer to keep track of the funds on L1 because it only depends on balancesL1 instead of also some external dependency (which would either be ERC20 token balances in the contracts or some stats from an external dapp).

It seems the new bulk work is to processing pool deposits and withdrawals, correct?

Yes, only the implementation of those pool operations have changed, but on not that different on a high level than before.

@Brechtpd Brechtpd merged commit 57126c0 into amm_v2 Jun 5, 2021
@Brechtpd Brechtpd deleted the amm_v2_am_fix branch June 5, 2021 16:42
Brechtpd added a commit that referenced this pull request Jun 8, 2021
* [protocol 3] Initial commit amm v2

* [protocol 3.8] Amm v2 improvements

* Small improvements

* Small simplification

* Update AmmData.sol

* Amm v2 - AssetManager fix (#2445)

* [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 <daniel@loopring.org>

* Amm v2 improve2 (#2462)

* Update AmplifiedAmmController.sol

* Update AmplifiedAmmController.sol

* AMM v2: minor improvements (#2466)

Co-authored-by: Daniel Wang <daniel@loopring.org>
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.

2 participants