[feat: CL hooks]: Add CL hooks into core CL logic and test hook-specific behavior #6859

AlpinYukseloglu · 2023-11-10T17:15:43Z

Closes: #XXX

What is the purpose of the change

This PR adds the CL hooks that were implemented in previous PRs into core CL logic.

Testing and Verifying

Testing this behavior in a way that does not circumvent important operations is actually quite tricky. While it is technically possible to cover this behavior using mock hooks, this felt much less robust than having a contract that actually implements the hooks to test against. This is what is done in pool_hooks_test.go.

As explained in the test comments, the tests lean on a CW contracts that implements all hooks with the following logic: when a hook is triggered, send 1 token with a denom corresponding to the hook to the originator of the tx (e.g. if I create a position, I would get 1 beforeCreatePosition token and 1 afterCreatePosition token). This makes testing behavior relatively straightforward (and hopefully readable).

Source code for the contract will be added in a separate PR to spare the diff for this one

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes?
Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

Specification (x/{module}/README.md)
Osmosis documentation site
Code comments?
N/A

AlpinYukseloglu · 2023-11-10T17:18:22Z

x/concentrated-liquidity/lp.go

@@ -89,15 +92,22 @@ func (k Keeper) CreatePosition(ctx sdk.Context, poolId uint64, owner sdk.AccAddr
 		return CreatePositionData{}, err
 	}

-	positionId := k.getNextPositionIdAndIncrement(ctx)
-


Note: this was moved below the hasPositions check to ensure the before hook was triggered after validation logic but before any state changes (getNextPositionIdAndIncrement mutates state)

AlpinYukseloglu · 2023-11-10T17:20:48Z

x/concentrated-liquidity/pool_hooks.go

-}
-
-// BeforeAddToPosition is a hook that is called before liquidity is added to a position.
-func (k Keeper) BeforeAddToPosition(ctx sdk.Context, poolId uint64, owner sdk.AccAddress, positionId uint64, amount0Added osmomath.Int, amount1Added osmomath.Int, amount0Min osmomath.Int, amount1Min osmomath.Int) error {


AddToPosition hooks were removed since the function just calls WithdrawPosition then CreatePosition under the hood. This makes it tricky to separate out AddToPosition hooks from the other two and imo unnecessary complexity that would increase the surface for circumventing hooks.

Opted to remove and keep hooks on the most primitive actions.

AlpinYukseloglu · 2023-11-10T17:21:47Z

x/concentrated-liquidity/pool_hooks_test.go

@@ -178,9 +178,136 @@ func (s *KeeperTestSuite) TestCallPoolActionListener() {
 	}
 }

+// Pool hook tests


Please see PR description for further justification of this testing approach

p0mvn

Nice work. Left some comments, please take a look.

Could you please also add a link to where the test contract code is written and make sure that it is easy to locate in case someone needs to find it in the future?

For example, I can see that there are some contract sources here:
https://github.com/osmosis-labs/osmosis/blob/c387e00c6338fa113ccb3abcb129f633670445f6/x/concentrated-liquidity/testcontracts/contract-sources/

Let's put the hooks source in there as well?

p0mvn · 2023-11-10T19:23:13Z

x/concentrated-liquidity/pool_hooks_test.go

+// General testing strategy:
+//  1. Build a pre-defined contract that defines the following behavior for all hooks:
+//     if triggered, transfer 1 token with denom corresponding to the action prefix
+//     e.g. if action prefix is "beforeSwap", transfer 1 token with denom "beforeSwap"
+//  2. Set this contract for all hooks defined by the test case (each case should have a list
+//     of action prefixes it wants to "activate")
+//  3. Run a series of actions that would trigger all the hooks (create, add to, withdraw from, swap against a position),
+//     and ensure that the correct denoms are in the account balance after each action/at the end.
+//
+// NOTE: we assume that set contracts have valid implementations for all hooks and that this is validated
+// at the contract setting stage at a higher level of abstraction. Thus, this class of errors is not covered
+// by these tests.


x/concentrated-liquidity/pool_hooks_test.go

p0mvn · 2023-11-10T19:29:41Z

x/concentrated-liquidity/swaps.go

+	// Trigger before hook for SwapExactAmountIn prior to mutating state.
+	// If no contract is set, this will be a no-op.
+	err = k.BeforeSwapExactAmountIn(ctx, pool.GetId(), sender, tokenIn, tokenOutDenom, tokenOutMinAmount, spreadFactor)
+	if err != nil {
+		return osmomath.Int{}, err
+	}


I don't think we have a mechanism in our tests for validating that BeforeSwapExactAmountIn is triggered before and not after the swap.

I think it would be valuable to add such an assertion so that we minimize the chances of a move-by-mistake in the future

This seems like it will require a meaningful change to the current testing approach so I created a separate issue with what I think would be a viable design in #6862

I agree that it's valuable to add this to ensure future changes/additions to hooks don't break this invariant. Would be great if we can handle this separately as I think it will take careful implementation to keep tests readable and would like to get the remaining core feature work in first

p0mvn · 2023-11-10T19:30:52Z

x/concentrated-liquidity/types/pool_hooks.go

@@ -81,26 +99,6 @@ type AfterCreatePositionMsg struct {
 	UpperTick      int64             `json:"upper_tick"`
 }

-type BeforeAddToPositionMsg struct {


Why did we decide to remove these?

Please see the note here: https://github.com/osmosis-labs/osmosis/pull/6859/files#r1389677483

mattverse

LGTM, let's make sure #6862 gets in as high priority! Left some nit comments please take a look!

x/concentrated-liquidity/testcontracts/compiled-wasm/hooks.wasm

x/concentrated-liquidity/types/pool_hooks.go

mattverse · 2023-11-13T06:45:47Z

x/concentrated-liquidity/pool_hooks_test.go

+		"single hook: before create position": {
+			actionPrefixes: []string{before(types.CreatePositionPrefix)},
+		},
+		"all before hooks": {
+			actionPrefixes: allBeforeHooks,
+		},
+		"all after hooks": {
+			actionPrefixes: allAfterHooks,
+		},
+		"all hooks": {
+			actionPrefixes: allHooks,
+		},


Seems like there will be dupicated test runs for before hook & after hook, was it that we wanted to test compound integration of hooks? 👀

Not sure I follow – the "all hooks" case is intended to run every hook simultaneously whereas the others run different subsets

yep, I was just curious of the significance of running hooks simultaneously vs running subset. (But it was a nit question, can be ignored)

Ah I see – no particular reason other than general coverage (ensuring that various subsets can correctly run without triggering the others)

x/concentrated-liquidity/swaps.go

mattverse

LGTM

p0mvn

LGTM

AlpinYukseloglu added 5 commits November 8, 2023 22:23

implement hook messages and calls

e270013

add comments and clean up helpers

4b9d72d

Merge branch 'main' into alpo/cl-hooks-feat

0d4140e

osmoutils go mod

a0a4bd6

alpo/cl-hooks-wiring

accfed4

AlpinYukseloglu added the V:state/breaking State machine breaking PR label Nov 10, 2023

github-actions bot added the C:x/concentrated-liquidity label Nov 10, 2023

changelog

29f79a3

AlpinYukseloglu commented Nov 10, 2023

View reviewed changes

clean up tests

8e5471c

AlpinYukseloglu changed the title ~~[feat: CL hooks]: Wire up CL hooks and test hook-specific behavior~~ [feat: CL hooks]: Add CL hooks into core CL logic and test hook-specific behavior Nov 10, 2023

AlpinYukseloglu mentioned this pull request Nov 10, 2023

[CL Hooks]: Add hooks source and clean up source code folder #6860

Merged

AlpinYukseloglu marked this pull request as ready for review November 10, 2023 17:41

AlpinYukseloglu requested review from czarcas7ic and p0mvn as code owners November 10, 2023 17:41

p0mvn reviewed Nov 10, 2023

View reviewed changes

AlpinYukseloglu added 5 commits November 12, 2023 21:33

tighten assertions

3eca42f

Merge branch 'main' into alpo/cl-hooks-feat

9d3cce8

Merge branch 'alpo/cl-hooks-feat' into alpo/cl-hooks-wiring

a2b10d5

go mod osmoutils

0c3c7f0

Merge branch 'alpo/cl-hooks-feat' into alpo/cl-hooks-wiring

61a63a6

Base automatically changed from alpo/cl-hooks-feat to main November 13, 2023 06:20

AlpinYukseloglu requested review from mattverse and nicolaslara as code owners November 13, 2023 06:20

Merge branch 'main' into alpo/cl-hooks-wiring

71bba74

mattverse reviewed Nov 13, 2023

View reviewed changes

x/concentrated-liquidity/swaps.go Show resolved Hide resolved

AlpinYukseloglu mentioned this pull request Nov 14, 2023

[tests: CL Hooks]: Implement test contract that returns error and test it #6873

Open

mattverse approved these changes Nov 14, 2023

View reviewed changes

p0mvn approved these changes Nov 22, 2023

View reviewed changes

AlpinYukseloglu merged commit 059c5db into main Nov 22, 2023
1 check passed

AlpinYukseloglu deleted the alpo/cl-hooks-wiring branch November 22, 2023 07:57

github-actions bot mentioned this pull request Nov 1, 2024

Broken Links Detected notional-labs/osmosis#76

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat: CL hooks]: Add CL hooks into core CL logic and test hook-specific behavior #6859

[feat: CL hooks]: Add CL hooks into core CL logic and test hook-specific behavior #6859

AlpinYukseloglu commented Nov 10, 2023

AlpinYukseloglu Nov 10, 2023

AlpinYukseloglu Nov 10, 2023

AlpinYukseloglu Nov 10, 2023

p0mvn left a comment

p0mvn Nov 10, 2023

p0mvn Nov 10, 2023

AlpinYukseloglu Nov 11, 2023

p0mvn Nov 10, 2023

AlpinYukseloglu Nov 11, 2023

mattverse left a comment

mattverse Nov 13, 2023

AlpinYukseloglu Nov 14, 2023

mattverse Nov 14, 2023

AlpinYukseloglu Nov 22, 2023

mattverse left a comment

p0mvn left a comment

[feat: CL hooks]: Add CL hooks into core CL logic and test hook-specific behavior #6859

[feat: CL hooks]: Add CL hooks into core CL logic and test hook-specific behavior #6859

Conversation

AlpinYukseloglu commented Nov 10, 2023

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattverse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattverse left a comment

Choose a reason for hiding this comment

p0mvn left a comment

Choose a reason for hiding this comment