restrict increase, decrease, and burn and v4 initialize pool call #416

dianakocsis · 2024-10-11T17:43:29Z

No description provided.

contracts/base/Dispatcher.sol

hensha256 · 2024-10-16T15:59:52Z

contracts/base/Dispatcher.sol

+                        uint256 action = uint8(actions[actionIndex]);
+
+                        if (
+                            action == Actions.INCREASE_LIQUIDITY || action == Actions.DECREASE_LIQUIDITY


i think this should all be in a helper function thats called like checkV4PositionManagerCall inside V3ToV4Migrator, and then this contract can stay way simpler. And all this logic can just go into there. So here its just

} else if (command == Commands.V4_POSITION_CALL) { checkV4PositionManagerCall() (success, output) = address(V4_POSITION_MANAGER).call{value: address(this).balance}(inputs); }

and i'd argue we shouldve done the same for v3 but that can be separate PR to refactor

hensha256 · 2024-10-16T16:22:09Z

contracts/base/Dispatcher.sol

+                    }
+
+                    bytes calldata slice = inputs[4:];
+                    (bytes calldata actions,) = slice.toBytes(0).decodeActionsRouterParams();


oh you never use the parameters its decoding... i feel like it would be possible instead (and more efficient) to try this...
bytes calldata actions = slice.toBytes(0).toBytes(0)

hensha256 · 2024-10-16T20:42:02Z

contracts/base/Dispatcher.sol

@@ -24,8 +26,6 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, V4SwapRout

    error InvalidCommandType(uint256 commandType);
    error BalanceTooLow();
-    error InvalidAction(bytes4 action);
-    error NotAuthorizedForToken(uint256 tokenId);


just moved to the v3 migrator module

contracts/modules/V3ToV4Migrator.sol

snreynolds

I think seems good to me, but I noticed in the tests that we dont actually use initialize before the mint, we just have a gas test for it. Unless I'm missing a test file? Could we add a full e2e test encoding the initialize then the mint, then check the success?

kmbarry1 · 2024-10-20T18:10:14Z

contracts/modules/V3ToV4Migrator.sol

+            uint256 action = uint8(actions[actionIndex]);
+
+            if (
+                action == Actions.INCREASE_LIQUIDITY || action == Actions.DECREASE_LIQUIDITY


IMO it would be clearer (and should save some bytecode size) to express this condition as:

if (action != Actions.MINT_POSITION) {

Or is there an explicit desire to allow all the other actions, and just disallow these three? In that case I'd argue the error name is a bit misleading and should be ForbiddenAction() or something like that.

Thinking about it more, it's gotta be the second case, so ignore the original comment and consider an error naming tweak.

Yeah because we still want settling, taking, clearing as actions. We can definitely make the error better and leave a comment to make this clearer

alternative check is if (action < Actions.SETTLE && action != Actions.MINT_POSITION), not sure if it'll be cheaper

kmbarry1 · 2024-10-20T18:50:59Z

contracts/base/Dispatcher.sol

+                    uint160 sqrtPriceX96;
+                    assembly {
+                        poolKey := inputs.offset
+                        sqrtPriceX96 := calldataload(add(inputs.offset, 0xa0))


It may be nice to leave a comment explaining the 0xa0 constant, it's a somewhat fragile pattern.

* Prepare for v4 work (#346) * update hardhat * remove symlinks by using hardhat-foundry use hardhat-foundry package to use foundry remappings to compile. Symlinks are no longer necessary After upgrade HH412 error was thrown caused by the existing symlinks ref: https://hardhat.org/hardhat-runner/docs/errors#HH412 NomicFoundation/hardhat#3623 * update compiler version to ^0.8.24 supporting the cancun upgrades * fix ci * fix lock file * Update yarn.lock * regenerate gas snapshots * install foundry in ci * Use mainnet permit2 (#347) * solc upgrade to 0.8.26 * use mainnet permit2 work started * fix uniswap tests * Remove block from resetFork * Refactor to fetch fee tiers * remove NFT protocols for V4 router (#348) * first pass * fix forge builds * fix reentrancy test * Add check to receive * remove .only rip * add todo for tests that need wrtiting * update readme and planner --------- Co-authored-by: Alice <[email protected]> Co-authored-by: Alice Henshaw <[email protected]> * V3 liquidity commands (#355) * import with dif version of OZ * fix remapping issue * duplicate exports and v3 version error * remove else statement and fix format * fix duplicate IERC165 in typechain * permit, decrease, collect, burn commands * all v3 commands separately * add structs * remove mint and increaseLiquidity on v3 * test change * fix yarn.lock * pass foundry tests for now * nit - change name * spelling error * v3pm address not needed * some gas tests * check msg.sender * format * transient storage * unauthorized tests * transient storage * v3 multicall with tests * v3 call + transient storage * no decode * clean up tests * some changes * comment changes * suggestion fixes * regenerate yarn.lock file * revert yarn.lock * some changes * separate migration tests * bignumber to fix test * fix remappings for forge compile * remove command placeholder * remove transient storage * format * use v3 periphery 0.8 instead * fix gas snapshots * v3 position manager addresses * name change * remove v4 in this pr * remove unnecessary stuff * change test names * fix erc721 permit * more tests * name changes, comments, test --------- Co-authored-by: gretzke <[email protected]> * Reentrancy guard transient (#354) * Make reentrancy guard transient * locker tests * abstract the locker library away * Updated locker tests * move map, and fix CI * Update lint.yml * update comment * V4 liquidity commands (#359) * v4 periphery git submodule * deploy v4 * prettier * point v4 periphery to main * use solmate/src and deploy v4 * format * update v4-periphery and future proof deployRouter * prettier * update v4-periphery, change remapping, and update deploy test (#361) * update v4-periphery, change remapping, and update deploy test * make immutables public * little fixes * last one * make only migrator immutables public * Make msgSender public (#365) * make maximum input transient (#366) * update v4 periphery to main and update deployRouter (#367) * update v4 periphery to main and update deployRouter * point to updated v4 posm * v4 planner (#368) * v4 planner * some renaming * separate uniswap tests (#369) * separate uniswap tests * Move permit2 tests * final decode in calldata (#363) * batch permit decode in calldata * array of structs * PR comment * V4 posm call (#364) * v4 mint so far * solmate remapping * v4 mint and increase tests * prettier * update v4 periphery to main * some gas tests * remove selector check in v4 posm call * organize some tests * more tests * prettier * more tests * update gas * remove unnecessary import * remove unnecessary encodings * Add v4 routing (#360) * complete setup, everything working * v4 command plus planner * use main v4 branch * update periphery, handle changes * linting * V4 test setup * working pool setup * cleanup of imports * 2 working v4 swap tests * exact out swaps * 2 hop swap tests * update periphery * Update posm with tests (#370) * update tests to correspond with posm * match deploy universal router with alices * update oz for ierc721permit problem * switch back to OZ 4.7.0 * use OZ 5.0.2 to match v4 * remove internal _msgSender() * pr comments * add license * Update periphery in UR (#371) * update to audit commit (#372) * V4 Native Tests (#373) * ETH input v4 tests * eth output v4 tests * PR comments * Add v4 to receive (#376) * add v4 to receive function * linting * PR comments * fix and snapshots * Take portion tests (#377) * take portion test * take portion test native * correct comment * take portion on input test * remove console logs oops * Oz L-09 (#382) * OZ L-09 * Remove .vscode/settings.json from the repository * update periphery to main (#380) * update periphery to main * update periphery after it updated main * update again * update again * add different tests for increasing and add tests for forwarding eth (#379) * add different tests for increasing and add tests for forwarding eth * fix gas snapshot * add more comments * fix gas snapshots * fix gas again * add comments on unpermit tests * OZ N-06 (#383) * OZ N-16 (#388) * ABDK CVF-107 (#385) * spearbit 94 (#389) * ABDK CVF106 (#387) * ABDK CVF106 * fix comment * oz-L13: missing natspec (#390) * Spearbit 59 (#391) * Spearbit 60 & 61 (#392) * OZ N-11 (#393) * update periphery (#394) * update periphery * remove position config * lint * Audit reports (#395) * Periphery update - calldata decoder (#396) * Periphery update - calldata decoder * rename function and snapshots * actually use the function * feat: update to 1.6.1-beta.1 (#398) * Chore/push button deploy (#400) * Update .gitmodules (#337) There is no need to use an account for GitHub. * chore(infra): set up deploy --------- Co-authored-by: Jonney <[email protected]> * chore: fix yarn publish (#401) * chore: fix yarn publish * Update deploy.yml * Update deploy.yml * chore(infra): deploy with npm (#403) * Update deploy.yml * Update deploy.yml * Update package.json * compile contracts in workflow * 2.0.0-beta.1 (#407) * Release workflow needs forge install (#408) * init submodules in release workflow (#409) * gitmodules should use https (#410) * Update v4 initialize (#415) * update periphery * update initialize ABI * temporary position descriptor constructor arg * lint * updated snap * restrict increase, decrease, and burn and v4 initialize pool call (#416) * restrict increase, decrease, and burn * prettier * initialize v4 pool call * prettier * refactor logic into function, plus optimise decode * merge latest periphery * move v3 logic outside of dispatcher * initialize pool tests * sbapshot * larger refactor * pull v4 periphery and revert payments * make checkV4InitializeCall like the others * Call initialize directly * undo v3 refactor * imports * remove unused typescript type * remove unnecessary extra immutable * Bump beta version * Improve coments * Add pool intiialization to some tests --------- Co-authored-by: Alice Henshaw <[email protected]> * deploy script for Base Sepolia (#381) * Update .gitmodules (#337) There is no need to use an account for GitHub. * deploy script for Base Sepolia * updating some deploys for sepolia L2s * include vanity/public RPCs for scripts * Update script/deployParameters/DeployOPSepolia.s.sol Co-authored-by: Alice <[email protected]> * Update script/deployParameters/DeployUnichainSepolia.s.sol Co-authored-by: Alice <[email protected]> --------- Co-authored-by: Jonney <[email protected]> Co-authored-by: Alice <[email protected]> * Latest v4 periphery (#420) * Latest v4 periphery * why didnt the linter catch this * v3 refactor and pull latest periphery (#418) * v3 refactor and pull latest periphery * switch to if else * allow permit2 to silently fail to avoid dos (#417) * allow permit2 to silently fail to avoid dos * fix tests * update periphery (#424) * update periphery not working * fix ur according to periphery update * prettier * fix issue during hardhat tests fixed in version 2.22.14 * change optimizer size * change optimizer runs for manager only * update again * prettier * update periphery again * switch runs to 30000 --------- Co-authored-by: gretzke <[email protected]> * move file (#425) * Merge updates into dev (#428) * Update .gitmodules (#337) There is no need to use an account for GitHub. * chore(infra): set up deploy (#397) * chore: yarn publish (#402) * chore: yarn publish * Update deploy.yml * Update deploy.yml * Update deploy.yml * Update deploy.yml * chore: add provenance (#404) * compile contracts in workflow (#405) * feat: deploy worldchain (#411) * fix: update UR address on worldchain (#412) previous accidentally set it to the unsupported protocol * Create CODEOWNERS (#419) --------- Co-authored-by: Jonney <[email protected]> Co-authored-by: mr-uniswap <[email protected]> Co-authored-by: Emily Williams <[email protected]> Co-authored-by: marktoda <[email protected]> Co-authored-by: dianakocsis <[email protected]> --------- Co-authored-by: Daniel Gretzke <[email protected]> Co-authored-by: diana <[email protected]> Co-authored-by: saucepoint <[email protected]> Co-authored-by: marktoda <[email protected]> Co-authored-by: mr-uniswap <[email protected]> Co-authored-by: Jonney <[email protected]> Co-authored-by: Emily Williams <[email protected]>

dianakocsis added 4 commits October 11, 2024 13:24

restrict increase, decrease, and burn

c39570d

prettier

8cc587b

initialize v4 pool call

d02e36a

prettier

3ad3fef

dianakocsis changed the title ~~restrict increase, decrease, and burn~~ restrict increase, decrease, and burn and v4 initialize pool call Oct 15, 2024

hensha256 reviewed Oct 16, 2024

View reviewed changes

contracts/base/Dispatcher.sol Outdated Show resolved Hide resolved

hensha256 reviewed Oct 16, 2024

View reviewed changes

hensha256 and others added 12 commits October 16, 2024 12:47

refactor logic into function, plus optimise decode

3034164

merge latest periphery

6c5baa8

move v3 logic outside of dispatcher

91745bc

move logic out of dispatcher

6fffb82

initialize pool tests

db7c0a7

sbapshot

d80c241

larger refactor

44fd388

pull v4 periphery and revert payments

e5ad3bd

make checkV4InitializeCall like the others

696d96d

Call initialize directly

ef8bb2f

undo v3 refactor

4994b4a

imports

6fb80be

hensha256 reviewed Oct 16, 2024

View reviewed changes

hensha256 added 3 commits October 16, 2024 16:42

remove unused typescript type

ddc45c5

remove unnecessary extra immutable

99c7506

Bump beta version

5bc8881

hensha256 had a problem deploying to release October 17, 2024 14:19 — with GitHub Actions Failure

mr-uniswap deployed to release October 17, 2024 14:31 — with GitHub Actions Active

snreynolds reviewed Oct 17, 2024

View reviewed changes

contracts/modules/V3ToV4Migrator.sol Show resolved Hide resolved

snreynolds reviewed Oct 17, 2024

View reviewed changes

contracts/modules/V3ToV4Migrator.sol Show resolved Hide resolved

snreynolds reviewed Oct 17, 2024

View reviewed changes

Improve coments

b6f402a

Add pool intiialization to some tests

0a0de10

snreynolds approved these changes Oct 17, 2024

View reviewed changes

hensha256 merged commit 5db48b1 into dev Oct 17, 2024
5 checks passed

hensha256 deleted the mint-only branch October 17, 2024 23:22

kmbarry1 reviewed Oct 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

restrict increase, decrease, and burn and v4 initialize pool call #416

restrict increase, decrease, and burn and v4 initialize pool call #416

dianakocsis commented Oct 11, 2024

hensha256 Oct 16, 2024

hensha256 Oct 16, 2024 •

edited

Loading

hensha256 Oct 16, 2024

snreynolds left a comment

kmbarry1 Oct 20, 2024

kmbarry1 Oct 20, 2024 •

edited

Loading

kmbarry1 Oct 20, 2024

hensha256 Oct 20, 2024

HickupHH3 Oct 21, 2024

kmbarry1 Oct 20, 2024

restrict increase, decrease, and burn and v4 initialize pool call #416

restrict increase, decrease, and burn and v4 initialize pool call #416

Conversation

dianakocsis commented Oct 11, 2024

hensha256 Oct 16, 2024

Choose a reason for hiding this comment

hensha256 Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

hensha256 Oct 16, 2024

Choose a reason for hiding this comment

snreynolds left a comment

Choose a reason for hiding this comment

kmbarry1 Oct 20, 2024

Choose a reason for hiding this comment

kmbarry1 Oct 20, 2024 • edited Loading

Choose a reason for hiding this comment

kmbarry1 Oct 20, 2024

Choose a reason for hiding this comment

hensha256 Oct 20, 2024

Choose a reason for hiding this comment

HickupHH3 Oct 21, 2024

Choose a reason for hiding this comment

kmbarry1 Oct 20, 2024

Choose a reason for hiding this comment

hensha256 Oct 16, 2024 •

edited

Loading

kmbarry1 Oct 20, 2024 •

edited

Loading