-
Notifications
You must be signed in to change notification settings - Fork 954
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Solve ToB-UNI4-3 without restricting sync #886
base: main
Are you sure you want to change the base?
Conversation
I quite like this as a solution, nice. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove L56 in ProtocolFees.sol but I'm in favor of this generally. I looked at the reasoning in #883 and I see how it provides more flexibility to integrators
@wjmelements Can you please update this branch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, would you mind also adding a test where the pool manager is unlocked, and a currency is synced and ProtocolFeeCurrencySynced() is thrown?
Co-authored-by: Sara Reynolds <30504811+snreynolds@users.noreply.github.com>
I have pushed a revert test, e710ad1, for the locked case that works with |
Can you add back in this modifier and include it on your test. I think thats how we solved this problem in the past https://github.com/Uniswap/v4-core/pull/856/files#r1739134580 |
|
@@ -1,3 +1,3 @@ | |||
{ | |||
"getReserves": "3931" | |||
"getReserves": "3973" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surprisingly this increase seems to be caused by noIsolate
in test_sync_multiple_unlocked
, even though this snapshot is from test_settle_nonNative_withoutSync_loseFunds
. When I revert 82dda85 it goes back to 3931
. I'm unsure why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change also results in 3931
:
function test_sync_multiple_unlocked() public /*noIsolate*/ {
manager.sync(currency1);
//assertEq(Currency.unwrap(currency1), Currency.unwrap(manager.getSyncedCurrency()));
manager.sync(currency0);
//assertEq(Currency.unwrap(currency0), Currency.unwrap(manager.getSyncedCurrency()));
}
Solves #883 and
ToB-UNI4-3
Reviewers @hensha256 @snreynolds
As described in #885 there are advantages to allowing sync to be called outside of the unlock callback.
The main reason this ability was removed was to fool-proof
collectProtocolFees
so that it cannot be called betweensync
andsettle
.This PR presents an alternative to #885 that still solves ToB-UNI4-3, not by modifying
sync
, but by modifyingcollectProtocolFees
.Assuming
sync
is called more often thancollectProtocolFees
, this fix forToB-UNI4-3
is superior to #856 even without considering situations like ERC-4337.Changes
Restore
sync
usability outside of the unlock callback.Check in
collectProtocolFees
whether the token to be transferred out is currently the synced token, handling the native Currency case.