feat: transfer cl position to new owner #6623

czarcas7ic · 2023-10-03T04:03:32Z

Closes: #6541

What is the purpose of the change

Allows a user to provide a list of position IDs that they own, collect all rewards, and transfer the ownership of that position to a new address.

This is achieved slightly different than how the issue described. Instead of withdrawing and creating a new position, we change the necessary state entries. The method described in the issue was discarded when I realized the amount of the liquidity the position has before and after withdrawing would never be 1:1, while the method done here achieves an exact duplicate of the position, with the only change being the owner.

Some drive by changes:

No longer call the SendCoins or FundCommunityPool on empty coins for incentive retrieval
No longer call SendCoins on empty coins for spread rewards retrieval
Adds a underlyingPositionsValue method to determine the coins value for a list of positionIds

Testing and Verifying

Rigorous testing done on the method itself as well as the events that are emitted, and any method that was created in this PR.

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

czarcas7ic · 2023-10-03T04:03:41Z

@devbot-wizard help

devbot-wizard · 2023-10-03T04:03:43Z

Hi! I'm DevBot, a bot that helps with common tasks in the development process. Commands:

devbot fix: Fix basic errors in the PR. (e.g. imports, changelog conflicts, go mod conflicts)
devbot merge base: Merge the base branch into the PR branch.
devbot add changelog [misc/feat/bug/api/sec] [message]: Add a changelog entry to the PR. (e.g. devbot add changelog feat Added a new feature)
- If message is blank, defaults to PR title.
devbot re-pr: Re-PR the PR, useful for external contributors where we have no edit perms.

github-actions · 2023-10-03T04:03:51Z

Important Notice

This PR modifies an in-repo Go module. It is one of:

osmomath
osmoutils
x/ibc-hooks
x/epochs

The dependent Go modules, especially the root one, will have to be
updated to reflect the changes. Failing to do so might cause e2e to fail.

Please follow the instructions below:

Open https://github.com/osmosis-labs/osmosis/actions/workflows/go-mod-auto-bump.yml
Provide the current branch name
On success, confirm if an automated commit corretly updated the go.mod and go.sum files

Please let us know if you need any help.

czarcas7ic · 2023-10-03T04:03:56Z

devbot add changelog feat

…fied dependencies locally

czarcas7ic · 2023-10-03T18:52:12Z

x/concentrated-liquidity/spread_rewards.go

+	// Early return, emit no events if there is no spread rewards to claim.
+	if spreadRewardsClaimed.IsZero() {
+		return sdk.Coins{}, nil
+	}
+


Slightly unrelated to this PR, but I think the change makes sense. I realized we would emit a bank send event coming from collecting spread rewards on every call, even when there are no spread rewards to claim. This logic added is similar to the incentives claiming logic that ends early if there are no incentives to claim.

czarcas7ic · 2023-10-03T22:07:47Z

x/concentrated-liquidity/incentives.go

+	if !collectedIncentivesForPosition.IsZero() {
+		if err := k.bankKeeper.SendCoins(ctx, pool.GetIncentivesAddress(), sender, collectedIncentivesForPosition); err != nil {
+			return sdk.Coins{}, sdk.Coins{}, err
+		}
 	}

 	// Send the forfeited incentives to the community pool from the pool's address.
-	err = k.communityPoolKeeper.FundCommunityPool(ctx, forfeitedIncentivesForPosition, pool.GetIncentivesAddress())
-	if err != nil {
-		return sdk.Coins{}, sdk.Coins{}, err
+	if !forfeitedIncentivesForPosition.IsZero() {
+		err = k.communityPoolKeeper.FundCommunityPool(ctx, forfeitedIncentivesForPosition, pool.GetIncentivesAddress())
+		if err != nil {
+			return sdk.Coins{}, sdk.Coins{}, err
+		}


Similar to the comment above, there is no need to run this logic and emit events if the value is zero.

…fied dependencies locally

x/concentrated-liquidity/position.go

…fied dependencies locally

Co-authored-by: Matt, Park <[email protected]>

Co-authored-by: Roman <[email protected]>

…fied dependencies locally

AlpinYukseloglu · 2023-10-11T00:33:25Z

x/concentrated-liquidity/msg_server_test.go

@@ -159,7 +160,7 @@ func (s *KeeperTestSuite) TestAddToPosition_Events() {
 	}{
 		"happy path": {
 			expectedAddedToPositionEvent: 1,
-			expectedMessageEvents:        5,
+			expectedMessageEvents:        4,


Do we know what caused the drop here?

Now that we do an early return on spread rewards claim when there are no spread rewards, we emit one less event.

x/concentrated-liquidity/position.go

AlpinYukseloglu · 2023-10-11T00:37:51Z

x/concentrated-liquidity/position.go

+			return types.PositionOwnerMismatchError{PositionOwner: position.Address, Sender: sender.String()}
+		}
+
+		// If the position has an active underlying lock, we cannot transfer it.


To clarify, the implication is that superfluid positions can't be transferred right?

This is correct, it is beyond the scope of requirements for this feature

p0mvn

LGTM.

I'm wondering if we could integrate this into the fuzz test suite to ensure the system continues being operational after position transfers.

Additionally, we should make sure that we test this on edgenet where bug bash would be happening

p0mvn · 2023-10-11T15:55:01Z

x/concentrated-liquidity/position_test.go

+		position, err := s.App.ConcentratedLiquidityKeeper.GetPosition(ctx, positionId)
+		s.Require().NoError(err)
+
+		// We fund the account with roundedInt because, there occurs


incomplete sentence

comment was outdated, I meant to remove the entire thing when I no longer funded with roundedInt but only removed it partially. Fixed here 04a3a68

…fied dependencies locally

czarcas7ic · 2023-10-11T20:09:33Z

Added transfer position to fuzz test 0b8367a

* transer cl position * Auto: update go.mod after push to adam/transfer-position-cl that modified dependencies locally * changelog * remove commented out logic * Update CHANGELOG.md * add godoc and test for ParseUint64SliceToString * add godoc * add incentives and spread rewards to message server test * expand message server test * further expand test * godoc * Auto: update go.mod after push to adam/transfer-position-cl that modified dependencies locally * use helper methods * modify array diff method * Auto: update go.mod after push to adam/transfer-position-cl that modified dependencies locally * address test * last test fix * Update x/concentrated-liquidity/client/cli/tx.go Co-authored-by: Matt, Park <[email protected]> * Update osmoutils/compare.go Co-authored-by: Roman <[email protected]> * rename all uses * sort the result, add tests for both sorted and unsorted * extend tests * unrelated lint * add testing invariants comment to TestTransferPositions * add why actual amount should be less than expected * Auto: update go.mod after push to adam/transfer-position-cl that modified dependencies locally * regen docs * comment explaining trasferPosition in proto * add fixed gas consumption * fix scaling gas charge and add parser test case * Auto: update go.mod after push to adam/transfer-position-cl that modified dependencies locally * remove incorrect comment * tidy osmomath * transfer random position --------- Co-authored-by: github-actions <[email protected]> Co-authored-by: Matt, Park <[email protected]> Co-authored-by: Roman <[email protected]> Co-authored-by: alpo <[email protected]>

transer cl position

efc7fba

czarcas7ic added the V:state/breaking State machine breaking PR label Oct 3, 2023

github-actions bot added C:CLI C:x/concentrated-liquidity labels Oct 3, 2023

github-actions and others added 2 commits October 3, 2023 04:06

Auto: update go.mod after push to adam/transfer-position-cl that modi…

3c0dbd4

…fied dependencies locally

changelog

bca1533

github-actions bot added the C:x/epochs label Oct 3, 2023

czarcas7ic and others added 6 commits October 3, 2023 09:31

remove commented out logic

dff2315

Update CHANGELOG.md

231ada1

add godoc and test for ParseUint64SliceToString

5cfe3bf

add godoc

198cf74

add incentives and spread rewards to message server test

21195ee

expand message server test

39bac77

czarcas7ic commented Oct 3, 2023

View reviewed changes

czarcas7ic added 2 commits October 3, 2023 16:02

further expand test

07be722

godoc

4346b86

czarcas7ic commented Oct 3, 2023

View reviewed changes

github-actions and others added 2 commits October 3, 2023 22:09

Auto: update go.mod after push to adam/transfer-position-cl that modi…

2528e96

…fied dependencies locally

use helper methods

0dc9123

czarcas7ic commented Oct 3, 2023

View reviewed changes

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

czarcas7ic and others added 2 commits October 3, 2023 16:37

modify array diff method

febb5d4

Auto: update go.mod after push to adam/transfer-position-cl that modi…

16e9829

…fied dependencies locally

czarcas7ic closed this Oct 3, 2023

czarcas7ic reopened this Oct 3, 2023

czarcas7ic added V:state/breaking State machine breaking PR and removed V:state/breaking State machine breaking PR labels Oct 3, 2023

czarcas7ic and others added 10 commits October 8, 2023 14:02

Update x/concentrated-liquidity/client/cli/tx.go

5d6f510

Co-authored-by: Matt, Park <[email protected]>

Update osmoutils/compare.go

bb936a1

Co-authored-by: Roman <[email protected]>

rename all uses

9cd487e

sort the result, add tests for both sorted and unsorted

3ab50f1

extend tests

56eea8d

unrelated lint

0254345

Merge branch 'main' into adam/transfer-position-cl

57fe3ca

add testing invariants comment to TestTransferPositions

b17bd2c

add why actual amount should be less than expected

c96b505

Auto: update go.mod after push to adam/transfer-position-cl that modi…

c9ecd14

…fied dependencies locally

czarcas7ic closed this Oct 8, 2023

czarcas7ic reopened this Oct 8, 2023

czarcas7ic added 3 commits October 8, 2023 16:07

regen docs

bb9706c

comment explaining trasferPosition in proto

2ac0230

add fixed gas consumption

ad2b3c3

AlpinYukseloglu reviewed Oct 11, 2023

View reviewed changes

fix scaling gas charge and add parser test case

fd24b80

AlpinYukseloglu approved these changes Oct 11, 2023

View reviewed changes

p0mvn approved these changes Oct 11, 2023

View reviewed changes

Auto: update go.mod after push to adam/transfer-position-cl that modi…

e68d661

…fied dependencies locally

czarcas7ic closed this Oct 11, 2023

czarcas7ic reopened this Oct 11, 2023

czarcas7ic added 4 commits October 11, 2023 13:43

remove incorrect comment

04a3a68

Merge branch 'main' into adam/transfer-position-cl

710bd89

tidy osmomath

a6d8092

transfer random position

0b8367a

czarcas7ic merged commit eff0deb into main Oct 11, 2023

czarcas7ic deleted the adam/transfer-position-cl branch October 11, 2023 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: transfer cl position to new owner #6623

feat: transfer cl position to new owner #6623

czarcas7ic commented Oct 3, 2023 •

edited

Loading

czarcas7ic commented Oct 3, 2023

devbot-wizard commented Oct 3, 2023

github-actions bot commented Oct 3, 2023

czarcas7ic commented Oct 3, 2023

czarcas7ic Oct 3, 2023

czarcas7ic Oct 3, 2023

AlpinYukseloglu Oct 11, 2023

czarcas7ic Oct 11, 2023

AlpinYukseloglu Oct 11, 2023

czarcas7ic Oct 11, 2023

p0mvn left a comment

p0mvn Oct 11, 2023

czarcas7ic Oct 11, 2023 •

edited

Loading

czarcas7ic commented Oct 11, 2023

feat: transfer cl position to new owner #6623

feat: transfer cl position to new owner #6623

Conversation

czarcas7ic commented Oct 3, 2023 • edited Loading

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

czarcas7ic commented Oct 3, 2023

devbot-wizard commented Oct 3, 2023

github-actions bot commented Oct 3, 2023

czarcas7ic commented Oct 3, 2023

czarcas7ic Oct 3, 2023

Choose a reason for hiding this comment

czarcas7ic Oct 3, 2023

Choose a reason for hiding this comment

AlpinYukseloglu Oct 11, 2023

Choose a reason for hiding this comment

czarcas7ic Oct 11, 2023

Choose a reason for hiding this comment

AlpinYukseloglu Oct 11, 2023

Choose a reason for hiding this comment

czarcas7ic Oct 11, 2023

Choose a reason for hiding this comment

p0mvn left a comment

Choose a reason for hiding this comment

p0mvn Oct 11, 2023

Choose a reason for hiding this comment

czarcas7ic Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

czarcas7ic commented Oct 11, 2023

czarcas7ic commented Oct 3, 2023 •

edited

Loading

czarcas7ic Oct 11, 2023 •

edited

Loading