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.

Sign up for GitHub

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

Jump to bottom

add (un)/wrapping for steth #317

Merged
merged 11 commits into from
Sep 18, 2023
Merged

add (un)/wrapping for steth #317

merged 11 commits into from
Sep 18, 2023

Conversation

ewilz
Copy link
Member

@ewilz ewilz commented Sep 13, 2023 •
edited
Loading

No description provided.

zhongeric
zhongeric reviewed Sep 13, 2023
View reviewed changes
contracts/modules/Payments.sol Show resolved Hide resolved
contracts/modules/Payments.sol Outdated Show resolved Hide resolved
zhongeric
zhongeric reviewed Sep 15, 2023
View reviewed changes
contracts/base/RouterImmutables.sol Outdated
@@ -34,6 +37,10 @@ contract RouterImmutables {
/// @dev WETH9 address
IWETH9 internal immutable WETH9;

ERC20 internal immutable STETH;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add @ dev comments

contracts/libraries/Commands.sol Outdated
@@ -70,5 +70,8 @@ library Commands {
uint256 constant SEAPORT_V1_4 = 0x20;
uint256 constant EXECUTE_SUB_PLAN = 0x21;
uint256 constant APPROVE_ERC20 = 0x22;
uint256 constant WRAP_STETH = 0x23;
uint256 constant UNWRAP_STETH = 0x24;

// COMMAND_PLACEHOLDER for 0x23 to 0x3f (all unused)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: change comment for new command values

contracts/modules/Payments.sol Outdated
@@ -137,4 +137,45 @@ abstract contract Payments is RouterImmutables {
}
}
}

/// @notice Wrap an amount of stETH into wstETH
/// @param recipient The recipient of the stETH
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: wstETH

contracts/modules/Payments.sol Outdated

/// @notice Unwraps all of the contract's wstETH into stETH
/// @param recipient The recipient of the stETH
/// @param amountMinimum The minimum amount of ETH desired
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: amount of stETH desired

test/integration-tests/Uniswap.test.ts
const { wethBalanceBefore, wethBalanceAfter } = await executeRouter(planner)
const stethBalanceAfter = await stethContract.balanceOf(bob.address)

expect(stethBalanceBefore.sub(stethBalanceAfter)).to.eq(amountInSTETH.sub(STETH_DUST))
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm a little confused on what this balance check is checking and what STETH_DUST represents, so when we wrap into wSETH, there's some dust that is lost that we have to account for here?

Copy link
Member Author

Choose a reason for hiding this comment

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

yah, wheneverrrrr STETH is tranferred there's always 1 wei dust 😓

Copy link
Contributor

Choose a reason for hiding this comment

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

whatt that's crazy

marktoda
marktoda approved these changes Sep 18, 2023
View reviewed changes
contracts/base/Dispatcher.sol
} else {
// placeholder area for commands 0x22-0x3f
// placeholder area for commands 0x25-0x3f
Copy link
Contributor

Choose a reason for hiding this comment

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

added 2 commands but this number increased by 3? Seems it switched from inclusive to exclusive

Copy link
Member Author

@ewilz ewilz Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like this was just forgotten in a past PR

@ewilz ewilz changed the title add wrapping for steth add (un)/wrapping for steth Sep 18, 2023
@ewilz ewilz merged commit bdb2942 into main Sep 18, 2023
3 checks passed
@ewilz ewilz deleted the steth branch September 18, 2023 19:24
marktoda added a commit that referenced this pull request Nov 9, 2023
@marktoda
Revert "add (un)/wrapping for steth (#317)"
f85e056 
This reverts commit bdb2942.
@marktoda marktoda mentioned this pull request Nov 9, 2023
Merged
hensha256 pushed a commit that referenced this pull request Jan 4, 2024
@marktoda @aadams
revert steth (#331)
5060892 
* Revert "add (un)/wrapping for steth (#317)"

This reverts commit bdb2942.

* Revert "deploy UniversalRouter v1.3 to mainnet and goerli (#320)"

This reverts commit 0250714.

* fix: fixup steth leftovers

* feat: update snaps

* fix: eric comments

* fix: fixed readme

---------

Co-authored-by: Austin Adams <[email protected]>
aadams added a commit that referenced this pull request Jan 4, 2024
@aadams @marktoda
fix: update v2 factory addresses (#334)
6a9846b 
* Revert "add (un)/wrapping for steth (#317)"

This reverts commit bdb2942.

* Revert "deploy UniversalRouter v1.3 to mainnet and goerli (#320)"

This reverts commit 0250714.

* fix: fixup steth leftovers

* feat: update snaps

* fix: eric comments

* fix: fixed readme

* fix: updated v2 factories

sepolia does not need upgrading

* fixed base's checksum

---------

Co-authored-by: Mark Toda <[email protected]>
hensha256 pushed a commit that referenced this pull request Jan 11, 2024
@aadams @marktoda @hensha256
fix: update v2 factory addresses (#334)
06316e9 
* Revert "add (un)/wrapping for steth (#317)"

This reverts commit bdb2942.

* Revert "deploy UniversalRouter v1.3 to mainnet and goerli (#320)"

This reverts commit 0250714.

* fix: fixup steth leftovers

* feat: update snaps

* fix: eric comments

* fix: fixed readme

* fix: updated v2 factories

sepolia does not need upgrading

* fixed base's checksum

---------

Co-authored-by: Mark Toda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhongeric zhongeric zhongeric left review comments

@marktoda marktoda marktoda approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ewilz @zhongeric @marktoda