-
Notifications
You must be signed in to change notification settings - Fork 37
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
<WIP> feat: set reserveOf transient #33
Conversation
So now we use sync, the excess tokens be burned ? |
do you mean the case when someone else randomly send erc20 token to the vault? If so, then yes, those tokens are burned now if this PR is merged. |
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.
LGTM
assembly { | ||
mstore(0x0, slot) | ||
mstore(0x20, currency) | ||
key := keccak256(0x0, 0x40) | ||
} |
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.
just curious, why not keccak256(abi.encodePacked(slot, currency.unwrap()))
?
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.
mostly around gas savings, can see CLPosition
and BinPosition
using similar technique.
now the latest test snapshot would be more accurate. though some test are failing and have comment out for now. likely this PR can serve as reference for now |
This PR is a POC on setting
reserveOf
as transient.Context
user flow to v4-core would change from
erc20.transfer(vault, amt) -> vault.settle()
tovault.sync() -> erc20.transfer(vault, amt) -> vault.settle()
as
reservesOf
are already tracked inreserveOfPoolManager
,reserveOf
is now transient. This should still be secured in the sense that CLPoolManager cannot take BinPoolManager assetThis also remove the need for
settleAndMintRefund()
due to async(currency)
which set the current erc20 balancethis should save gas however the snapshot might not show this case (see below)
Why gas snapshot doesn't show this case ?
forge test
where we should useforge test --isolate
to run each statement as a new transaction. In this case we can see the clear benefit of transient storageDiscussion:
forge test --isolate
andsnapLastCall()
and merge then update this branch and we can see clear gas benefitreserveOf
check or move toerc20.balanceOf(vault)
lockWithSync(bytes calldata data, Currency[] currency)
which gets a lock() and sync() those currency