Skip to content
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

Add wrap, unwrap functionality #369

Merged
merged 17 commits into from
Oct 24, 2024
Merged

Add wrap, unwrap functionality #369

merged 17 commits into from
Oct 24, 2024

Conversation

snreynolds
Copy link
Member

Related Issue

Which issue does this pull request resolve?

Description of changes

@snreynolds snreynolds requested a review from hensha256 October 17, 2024 15:15
}

function wrap(uint256 _amount) external payable {
uint256 amount = _amount == ActionConstants.CONTRACT_BALANCE ? address(this).balance : _amount;
Copy link
Contributor

Choose a reason for hiding this comment

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

might be worth added 2 extra checks to each of these functions

  1. if amount > balance revert - so that theres a nice custom error?
  2. only deposit (or withdraw) if amount != 0. As someone might pass in CONTRACT_BALANCE and the current balance is 0 so then deposit/withdraw shouldnt be called

So maybe like

        if (amount == ActionConstants.CONTRACT_BALANCE) {
            amount = address(this).balance;
        } else if (amount > address(this).balance) {
            revert InsufficientETH();
        }
        if (amount > 0) WETH9.deposit{value: amount}();

Copy link
Member Author

Choose a reason for hiding this comment

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

good call out!

src/PositionManager.sol Outdated Show resolved Hide resolved
src/PositionManager.sol Outdated Show resolved Hide resolved
src/PositionManager.sol Outdated Show resolved Hide resolved
src/base/NativeWrapper.sol Outdated Show resolved Hide resolved
src/libraries/Actions.sol Show resolved Hide resolved
src/base/DeltaResolver.sol Show resolved Hide resolved
src/base/DeltaResolver.sol Outdated Show resolved Hide resolved
src/base/DeltaResolver.sol Outdated Show resolved Hide resolved
src/base/DeltaResolver.sol Outdated Show resolved Hide resolved
/// @notice Calculates the sanitized amount before wrapping/unwrapping.
/// @param inputCurrency The currency, either native or wrapped native, that the contract holds
/// @param amount The amount to wrap or unwrap. Can be CONTRACT_BALANCE or OPEN_DELTA or a specific amount
/// @param outputCurrency The currency after the wrap/unwrap that the user may owe a balance in on the poolManager
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// @param outputCurrency The currency after the wrap/unwrap that the user may owe a balance in on the poolManager
/// @param outputCurrency The currency after the wrap/unwrap

Copy link
Contributor

Choose a reason for hiding this comment

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

They also could be wrapping/unwrapping before sweeping to themselves etc. I feel like theres a few valid use-cases so shouldnt single one out

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah but outputCurrency is only used to get the OPEN_DELTA balance on the contract

Copy link
Member Author

Choose a reason for hiding this comment

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

i didn't change this but lmk i can change if you still think so after reading this comment above^

Copy link
Contributor

Choose a reason for hiding this comment

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

i see, maybe it could be clearer to say something that describes that better like

The currency after the wrap/unwrap, used when amount is OPEN_DELTA to look up the balance owed to the poolManager

or something like that?

src/libraries/CalldataDecoder.sol Show resolved Hide resolved
src/base/NativeWrapper.sol Show resolved Hide resolved
@@ -308,4 +329,114 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF
);
lpm.modifyLiquidities(calls, _deadline);
}

function test_weth_wrap_increaseLiquidity() public {
Copy link
Contributor

Choose a reason for hiding this comment

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

would love to see a test case that uses OPEN_DELTA to test that flow works too

@snreynolds snreynolds requested a review from hensha256 October 24, 2024 20:34
@snreynolds snreynolds merged commit 01ddf3a into main Oct 24, 2024
3 checks passed
@snreynolds snreynolds deleted the wrap-unwrap branch October 24, 2024 20:49
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.

6 participants