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 weth wrapping hook #436

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Add weth wrapping hook #436

wants to merge 9 commits into from

Conversation

marktoda
Copy link
Contributor

@marktoda marktoda commented Feb 2, 2025

token wrapper adapter

Overview

This PR introduces a WETH (Wrapped Ether) hook for Uniswap v4, enabling native ETH to WETH conversions directly through v4 pools. The implementation provides a convenient way to wrap and unwrap ETH within a v4 lock / route.

@marktoda marktoda requested a review from hensha256 February 2, 2025 01:57
@marktoda
Copy link
Contributor Author

marktoda commented Feb 2, 2025

Note one constraint of this approach is that the input currency must already exist in poolmanager before the swap call. This could either be from existing standing liquidity or else the user must transfer input assets first

src/base/hooks/BaseTokenWrapperHook.sol Outdated Show resolved Hide resolved
/// @dev This contract provides the base functionality for wrapping/unwrapping tokens through V4 pools
/// @dev All liquidity operations are blocked as liquidity is managed through the underlying token wrapper
/// @dev Implementing contracts must provide deposit() and withdraw() functions
abstract contract BaseTokenWrapperHook is BaseHook {
Copy link
Contributor

Choose a reason for hiding this comment

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

imo we should be removing the inheritance of SafeCallback that’s happening into BaseHook. I don’t think that should be in there - not all hooks are going to take out locks on the PM (eg this one) so it’s an unnecessary addition.
Could be a separate PR just putting this thought here.

src/base/hooks/BaseTokenWrapperHook.sol Show resolved Hide resolved
src/base/hooks/BaseTokenWrapperHook.sol Outdated Show resolved Hide resolved

if (isWrapping) {
uint256 inputAmount =
isExactInput ? uint256(-params.amountSpecified) : _getWrapInputRequired(uint256(params.amountSpecified));
Copy link
Contributor

Choose a reason for hiding this comment

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

technically its not safe to invert a negative number without safecast/checking whether the value is the minimum integer. Although I cant think of an attack vector. We do have a large SafeCast.sol in core you can use for most of these things

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not totally sure what you mean here - what is the problem if it's the minimum integer? and how does safecast help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also theres no int256 => uint256 function in that library

uint256 inputAmount =
isExactInput ? uint256(-params.amountSpecified) : _getWrapInputRequired(uint256(params.amountSpecified));
poolManager.take(underlyingCurrency, address(this), inputAmount);
uint256 wrappedAmount = deposit(inputAmount);
Copy link
Contributor

Choose a reason for hiding this comment

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

is it worth deposit just handling a uint128 or smaller, given you have to cast up to uint256 before this, and then back down later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm ok did this but not sure its really any cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just pushes the weird casting down into the inheriting contracts

@marktoda marktoda requested a review from hensha256 February 3, 2025 16:18
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