[Bug]: Sync reverts when locked

[wjmelements]

Describe the bug

Sync reverts when locked

Expected Behavior

Sync should work when locked

To Reproduce

call sync outside unlock

Additional context

broken by #856

[linear]

PROTO-621 [Bug]: Sync reverts when locked

[wjmelements]

I should be able to

1. sync
2. transfer
3. unlock
4. settle; swap; take
5. finish unlock

This pattern was broken by #856; now sync and transfer must happen within unlock.

• There is no reason for this restriction.
• It impacts many use cases by requiring more activities happen inside the callback.
• In some cases the restriction requires additional transfers, wasting thousands of gas.

[wjmelements]

My recommendation is to revert #856.

[wjmelements]

Full writeup is in the PR description: #885

[wjmelements]

I have a second solution in case ToB-UNI4-3 is still considered important after reconsideration: #886.

[wjmelements]

I calculate that the gas impact of the newly necessary approve/transferFrom or extra transfer described in #885 for my project is between 8371 and 25292 for tokens DAI and WETH.

[wjmelements]

I calculate that the gas impact of the newly necessary approve/transferFrom or extra transfer described in #885 for my project is between 8371 and 25292 for tokens DAI and WETH.

Full writeup is in the PR description: #885

I reproduce the impact statement here, which discusses who all is impacted and how.

Impact: Additional transfers

This section describes example of the aforementioned scenario, where being able to sync before unlock saves thousands of gas. For my project the gas impact is between 8371 and 25292 gas when the settled token is DAI or WETH.

Custodian Contract (CC)

This contract custodies funds and is the ultimate counterparty performing the swap. Perhaps it is an ERC-4337 smart account. Perhaps it is a multisig. But for whatever reason it doesn't have support for the UniswapV4 callback.

Swap Callback (SC)

This contract has support for the UniswapV4 callback. Its logic might be shared between many CC or it may be specific to one CC.

The swap with versatile sync: transfer

CC: PoolManager.sync(TokenA)
CC: TokenA.transfer(PoolManager, AmountIn)
CC: SC.performSwap()
SC: PoolManager.unlock()
PoolManager: SC.unlockCallback()
SC: PoolManager.settle(TokenA)
SC: PoolManager.swap()
SC: PoolManager.take(TokenB, CC)

The swap without versatile sync: approve/transferFrom

CC: TokenA.approve(SC, AmountIn)
CC: SC.performSwap()
SC: PoolManager.unlock()
PoolManager: SC.unlockCallback()
SC: PoolManager.sync(TokenA)
SC: TokenA.transferFrom(CC, PoolManager, AmountIn)
SC: PoolManager.settle(TokenA)
SC: PoolManager.swap()
SC: PoolManager.take(TokenB, CC)

The swap without versatile sync: transfer/transfer

CC: TokenA.transfer(SC, AmountIn)
CC: SC.performSwap()
SC: PoolManager.unlock()
PoolManager: SC.unlockCallback()
SC: PoolManager.sync(TokenA)
SC: TokenA.transfer(PoolManager, AmountIn)
SC: PoolManager.settle(TokenA)
SC: PoolManager.swap()
SC: PoolManager.take(TokenB, CC)

Analysis

The additional approval/transferFrom or transfer/transfer in comparison to a single transfer amounts to thousands to tens of thousands of gas.
Any account that wants to swap but doesn't have uniswap v4 callbacks benefits from sync outside of unlock.

[wjmelements]

I have a second solution in case ToB-UNI4-3 is still considered important after reconsideration: #886.

#885 was closed, possibly in favor of #886.