Support arbitrary bytes on test routers #343

saucepoint · 2023-08-25T04:23:05Z

Related Issue

Which issue does this pull request resolve?

The test routers (i.e. PoolSwapTest) did not support arbitrary data arguments (new in #332). This PR updates the test routers to support the flow of arbitrary bytes from the router to the hooks.

Description of changes

Updated the main function(s) in PoolSwapTest, PoolModifyPositionTest, and PoolDonateTest to support arbitrary data args
Updated MockHooks to write arbitrary data to storage. Used to verify that test routers are providing arbitrary bytes correctly (Hooks.t.sol)

…red in the hook

saucepoint · 2023-08-30T14:14:11Z

@ewilz any chance i can get reviewers on this?

snreynolds

lgtm!

ewilz · 2023-09-11T20:34:29Z

contracts/test/PoolSwapTest.sol

@@ -35,21 +36,29 @@ contract PoolSwapTest is ILockCallback {
        payable
        returns (BalanceDelta delta)
    {
-        delta =
-            abi.decode(manager.lock(abi.encode(CallbackData(msg.sender, testSettings, key, params))), (BalanceDelta));
+        return swap(key, params, testSettings, new bytes(0));


looks like gas snapshots went up by >1k for all swaps...wonder if consolidating the swap functions here makes more sense than having 2 external calls (or just keeping them separate altogether)

I prefer removing the old function signature and moving everything over to the new function signature w/ bytes memory hookData (instead of duplicating the logic). I wanted to keep the original for backwards compatibility with v4-periphery and community repos, but I suppose theres been breaking changes already so might as well keep it up 😝

Will try it for both swap + modifyPosition

ewilz · 2023-09-11T20:36:25Z

contracts/test/PoolModifyPositionTest.sol

    }

    function modifyPosition(PoolKey memory key, IPoolManager.ModifyPositionParams memory params)
        external
        payable
        returns (BalanceDelta delta)
    {
-        delta = abi.decode(manager.lock(abi.encode(CallbackData(msg.sender, key, params))), (BalanceDelta));
+        return modifyPosition(key, params, new bytes(0));


same as swap, but only 977 gas increase for modify position. I wonder if we can minimize the impact on gas
per test contracts by either consolidating the functions..

I removed the old modifyPosition() and the gas is practically the same, or worse in some cases 🤔

Here's the diff
https://github.com/saucepoint/v4-core/pull/1/files

snreynolds · 2023-09-11T20:39:49Z

Also looks like the hardhat tests are failing. Since we still have some coverage with hardhat unfortunately we should get those updated even though we are in the process of deprecating. Can reproduce with yarn test.

…t.modifyPosition()

… bytes memory

saucepoint · 2023-09-12T16:53:59Z

To recap the outstanding changes, there's two paths we could take:

Remove overloaded functions by consolidating to the function(s) with bytes memory parameters
- Gas improvements are negligible or worse
- Update hardhat tests to the new function signatures
- Update v4-periphery to the new function signatures
- Requires users to provide ZERO_BYTES when using the test routers
- More work, but cleaner codebase
Keep overloaded functions
- Hardhat tests are ugly because they need to reference the full function signature. But these are getting deprecated anyway 🙏
- backwards compatible with v4-periphery and existing repos
- cleaner interface for users that dont need arbitrary bytes

Consolidate overloaded `modifyPosition` and `swap`

snreynolds · 2023-09-19T18:34:57Z

To recap the outstanding changes, there's two paths we could take:

Remove overloaded functions by consolidating to the function(s) with bytes memory parameters

Gas improvements are negligible or worse

Update hardhat tests to the new function signatures

Update v4-periphery to the new function signatures

Requires users to provide ZERO_BYTES when using the test routers

More work, but cleaner codebase

Keep overloaded functions

Hardhat tests are ugly because they need to reference the full function signature. But these are getting deprecated anyway 🙏

backwards compatible with v4-periphery and existing repos

cleaner interface for users that dont need arbitrary bytes

I'm in favor of option #1. Don't think we need to maintain backwards compatibility in tests - we should just update them to match the new functionality. Gas seems to just be overhead of the new ZERO_BYTES param so I'm not that worried about it but curious to hear your thoughts @ewilz.

…ed hardhat gas snapshots

contracts/test/PoolSwapTest.sol

contracts/test/PoolDonateTest.sol

contracts/test/PoolModifyPositionTest.sol

…ction signature

snreynolds · 2023-09-26T18:32:51Z

@saucepoint Hm looks like hardhat tests are still failing, and I think you need to run yarn prettier but other than that happy to merge! I think for the hardhat tests you just need to update the snapshots

snreynolds · 2023-09-26T19:57:44Z

Closing in favor of #361

saucepoint added 9 commits August 24, 2023 22:27

overload PoolSwapTest.swap() with support for arbitrary bytes

968c345

forge fmt

870b588

consolidate logic to one function

8e65fdf

overload routertest.modifyPosition() with support for arbitrary bytes

bb5ae64

overload PoolDonateTest.donate() with support for arbitrary bytes

c304834

overload test helper with support for arbitrary bytes

b45318b

define storage variables to verify arbitrary calldata

bb48e13

test that arbitrary calldata provided through routers are being captu…

1d53214

…red in the hook

forge fmt

9a67aaa

saucepoint added 2 commits September 8, 2023 17:43

resolve conflicts

8c65e81

forge snapshots

1347893

snreynolds previously approved these changes Sep 11, 2023

View reviewed changes

ewilz reviewed Sep 11, 2023

View reviewed changes

saucepoint added 3 commits September 12, 2023 11:33

Merge branch 'main' into router-bytes

f3623dc

remove overloaded modifyPosition(); consolidate PoolModifyPositionTes…

e04f2c2

…t.modifyPosition()

remove overloaded swapRouter.swap() and consolidate to signature with…

9f08118

… bytes memory

This comment was marked as outdated.

Sign in to view

saucepoint requested a review from ewilz September 18, 2023 18:31

Merge pull request #1 from saucepoint/consolidate

1346029

Consolidate overloaded `modifyPosition` and `swap`

saucepoint dismissed snreynolds’s stale review via 1346029 September 19, 2023 18:29

updated hardhart tests with the new router function signatures; updat…

fdef1fa

…ed hardhat gas snapshots

snreynolds requested changes Sep 21, 2023

View reviewed changes

contracts/test/PoolSwapTest.sol Outdated Show resolved Hide resolved

contracts/test/PoolDonateTest.sol Outdated Show resolved Hide resolved

contracts/test/PoolDonateTest.sol Outdated Show resolved Hide resolved

contracts/test/PoolModifyPositionTest.sol Outdated Show resolved Hide resolved

saucepoint added 2 commits September 21, 2023 14:07

function visibility

298e7bb

remove overloaded PoolDonateTest.donate(), updated tests with new fun…

8b66c7b

…ction signature

saucepoint requested a review from snreynolds September 21, 2023 18:10

snreynolds previously approved these changes Sep 22, 2023

View reviewed changes

provide empty bytes on donate; yarn prettier

40122d5

saucepoint dismissed snreynolds’s stale review via 40122d5 September 26, 2023 18:40

recalc gas snapshots

079693d

snreynolds self-requested a review September 26, 2023 19:30

snreynolds approved these changes Sep 26, 2023

View reviewed changes

saucepoint added 2 commits September 26, 2023 15:37

misc

ba7bdd7

forge fmt

9d8ce09

saucepoint mentioned this pull request Sep 26, 2023

Support arbitrary hookData on test routers #361

Merged

snreynolds closed this Sep 26, 2023

saucepoint deleted the router-bytes branch November 8, 2023 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support arbitrary bytes on test routers #343

Support arbitrary bytes on test routers #343

saucepoint commented Aug 25, 2023 •

edited

Loading

saucepoint commented Aug 30, 2023

snreynolds left a comment

ewilz Sep 11, 2023 •

edited

Loading

saucepoint Sep 11, 2023

ewilz Sep 11, 2023 •

edited

Loading

saucepoint Sep 12, 2023

snreynolds commented Sep 11, 2023

This comment was marked as outdated.

saucepoint commented Sep 12, 2023 •

edited

Loading

snreynolds commented Sep 19, 2023

snreynolds commented Sep 26, 2023 •

edited

Loading

snreynolds commented Sep 26, 2023

Support arbitrary bytes on test routers #343

Support arbitrary bytes on test routers #343

Conversation

saucepoint commented Aug 25, 2023 • edited Loading

Related Issue

Description of changes

saucepoint commented Aug 30, 2023

snreynolds left a comment

Choose a reason for hiding this comment

ewilz Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

saucepoint Sep 11, 2023

Choose a reason for hiding this comment

ewilz Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

saucepoint Sep 12, 2023

Choose a reason for hiding this comment

snreynolds commented Sep 11, 2023

This comment was marked as outdated.

saucepoint commented Sep 12, 2023 • edited Loading

snreynolds commented Sep 19, 2023

snreynolds commented Sep 26, 2023 • edited Loading

snreynolds commented Sep 26, 2023

saucepoint commented Aug 25, 2023 •

edited

Loading

ewilz Sep 11, 2023 •

edited

Loading

ewilz Sep 11, 2023 •

edited

Loading

saucepoint commented Sep 12, 2023 •

edited

Loading

snreynolds commented Sep 26, 2023 •

edited

Loading