Adams review

x/lockup/keeper/lock.go

@@ -834,7 +834,7 @@ func (k Keeper) SplitLock(ctx sdk.Context, lock types.PeriodLock, coins sdk.Coin
 	splitLockID := k.GetLastLockID(ctx) + 1
 	k.SetLastLockID(ctx, splitLockID)
 
-	splitLock := types.NewPeriodLock(splitLockID, lock.OwnerAddress(), lock.OwnerAddress().String(), lock.Duration, lock.EndTime, coins)
+	splitLock := types.NewPeriodLock(splitLockID, lock.OwnerAddress(), lock.RewardReceiverAddress, lock.Duration, lock.EndTime, coins)
 
 	err = k.setLock(ctx, splitLock)
 	return splitLock, err

x/lockup/keeper/lock_test.go

@@ -888,6 +888,109 @@ func (s *KeeperTestSuite) TestLock() {
 	balance = s.App.BankKeeper.GetBalance(s.Ctx, acc.GetAddress(), "stake")
 	s.Require().Equal(sdk.NewInt(0).String(), balance.Amount.String())
 }
+func (s *KeeperTestSuite) TestSplitLock() {
+	defaultAmount := sdk.NewCoins(sdk.NewCoin("foo", sdk.NewInt(100)), sdk.NewCoin("bar", sdk.NewInt(200)))
+	defaultHalfAmount := sdk.NewCoins(sdk.NewCoin("foo", sdk.NewInt(40)), sdk.NewCoin("bar", sdk.NewInt(110)))
+	testCases := []struct {
+		name                      string
+		amountToSplit             sdk.Coins
+		isUnlocking               bool
+		isForceUnlock             bool
+		useDifferentRewardAddress bool
+		expectedErr               bool
+	}{
+		{
+			"happy path: split half amount",
+			defaultHalfAmount,
+			false,
+			false,
+			false,
+			false,
+		},
+		{
+			"happy path: split full amount",
+			defaultAmount,
+			false,
+			false,
+			false,
+			false,
+		},
+		{
+			"happy path: try using reward address with different reward receiver",
+			defaultAmount,
+			false,
+			false,
+			true,
+			false,
+		},
+		{
+			"error: unlocking lock",
+			defaultAmount,
+			true,
+			false,
+			false,
+			true,
+		},
+		{
+			"error: force unlock",
+			defaultAmount,
+			true,
+			false,
+			false,
+			true,
+		},
+	}
+	for _, tc := range testCases {
+		s.SetupTest()
+		defaultDuration := time.Minute
+		defaultEndTime := time.Time{}
+		lock := types.NewPeriodLock(
+			1,
+			s.TestAccs[0],
+			s.TestAccs[0].String(),
+			defaultDuration,
+			defaultEndTime,
+			defaultAmount,
+		)
+		if tc.isUnlocking {
+			lock.EndTime = s.Ctx.BlockTime()
+		}
+		if tc.useDifferentRewardAddress {
+			lock.RewardReceiverAddress = s.TestAccs[1].String()
+		}
+
+		// manually set last lock id to 1
+		s.App.LockupKeeper.SetLastLockID(s.Ctx, 1)
+		// System under test
+		newLock, err := s.App.LockupKeeper.SplitLock(s.Ctx, lock, tc.amountToSplit, tc.isForceUnlock)
+		if tc.expectedErr {
+			s.Require().Error(err)
+			return
+		}
+		s.Require().NoError(err)
+
+		// check if the new lock has correct states
+		s.Require().Equal(newLock.ID, lock.ID+1)
+		s.Require().Equal(newLock.Owner, lock.Owner)
+		s.Require().Equal(newLock.Duration, lock.Duration)
+		s.Require().Equal(newLock.EndTime, lock.EndTime)
+		s.Require().Equal(newLock.RewardReceiverAddress, lock.RewardReceiverAddress)
+		s.Require().True(newLock.Coins.IsEqual(tc.amountToSplit))
+
+		// now check if the old lock has correctly updated state
+		updatedOriginalLock, err := s.App.LockupKeeper.GetLockByID(s.Ctx, lock.ID)
+		s.Require().Equal(updatedOriginalLock.ID, lock.ID)
+		s.Require().Equal(updatedOriginalLock.Owner, lock.Owner)
+		s.Require().Equal(updatedOriginalLock.Duration, lock.Duration)
+		s.Require().Equal(updatedOriginalLock.EndTime, lock.EndTime)
+		s.Require().Equal(updatedOriginalLock.RewardReceiverAddress, lock.RewardReceiverAddress)
+		s.Require().True(updatedOriginalLock.Coins.IsEqual(lock.Coins.Sub(tc.amountToSplit)))
+
+		// check that last lock id has incremented properly
+		lastLockId := s.App.LockupKeeper.GetLastLockID(s.Ctx)
+		s.Require().Equal(lastLockId, newLock.ID)
+	}
+}
 
 func (s *KeeperTestSuite) AddTokensToLockForSynth() {
 	s.SetupTest()

x/lockup/keeper/migration.go

@@ -37,6 +37,8 @@ func normalizeDuration(baselineDurations []time.Duration, allowedDiff time.Durat
 // If a lockup is far from any base duration, we don't change anything about it.
 // We define a lockup length as a "Similar duration to base duration D", if:
 // D <= lockup length <= D + durationDiff.
+// Any locks with different reward receiver and lock owner would not be merged.
+// NOTE: This method has been outdated, please make new migration code if there is a need to merge locks with similar duration.
 func MergeLockupsForSimilarDurations(
 	ctx sdk.Context,
 	k Keeper,
@@ -65,6 +67,13 @@ func MergeLockupsForSimilarDurations(
 				continue
 			}
 			coin := lock.Coins[0]
+
+			// check if the reward receiver is the lock owner.
+			// if not, we do not normalize the lock.
+			if lock.Owner != lock.RewardReceiverAddress {
+				continue
+			}
+
 			// In this pass, add lock to normals if exact match
 			key := addr.String() + "/" + coin.Denom + "/" + strconv.FormatInt(int64(normalizedDuration), 10)
 			_, normalExists := normals[key]

x/lockup/keeper/msg_server.go

@@ -235,7 +235,7 @@ func (server msgServer) SetRewardReceiverAddress(goCtx context.Context, msg *typ
 		return nil, err
 	}
 
-	newRewardRecepient, err := sdk.AccAddressFromBech32(msg.Owner)
+	newRewardRecepient, err := sdk.AccAddressFromBech32(msg.RewardReceiver)
 	if err != nil {
 		return nil, err
 	}

x/lockup/keeper/msg_server_test.go

@@ -455,3 +455,97 @@ func (s *KeeperTestSuite) TestMsgForceUnlock() {
 		}
 	}
 }
+
+func (s *KeeperTestSuite) TestSetRewardReceiverAddress() {
+	type param struct {
+		isOwner                      bool
+		isRewardReceiverAddressOwner bool
+		isLockOwner                  bool
+	}
+
+	tests := []struct {
+		name       string
+		param      param
+		expectPass bool
+	}{
+		{
+			name: "happy path: change reward receiver address to another address",
+			param: param{
+				isOwner:                      true,
+				isRewardReceiverAddressOwner: false,
+				isLockOwner:                  true,
+			},
+			expectPass: true,
+		},
+		{
+			name: "error: attempt to try changing to same owner",
+			param: param{
+				isOwner:                      false,
+				isRewardReceiverAddressOwner: true,
+				isLockOwner:                  true,
+			},
+			expectPass: false,
+		},
+		{
+			name: "error: sender is not the owner of the lock",
+			param: param{
+				isOwner:                      false,
+				isRewardReceiverAddressOwner: false,
+				isLockOwner:                  true,
+			},
+			expectPass: false,
+		},
+	}
+
+	for _, test := range tests {
+		s.SetupTest()
+
+		defaultAmountInLock := sdk.NewCoins(sdk.NewInt64Coin("foo", 100))
+		s.FundAcc(s.TestAccs[0], defaultAmountInLock)
+
+		lock, err := s.App.LockupKeeper.CreateLock(s.Ctx, s.TestAccs[0], defaultAmountInLock, time.Minute)
+		s.Require().NoError(err)
+
+		// lock reward receiver address should initially be an empty string
+		s.Require().Equal(lock.RewardReceiverAddress, "")
+
+		msgServer := keeper.NewMsgServerImpl(s.App.LockupKeeper)
+		c := sdk.WrapSDKContext(s.Ctx)
+
+		owner := s.TestAccs[0]
+		if !test.param.isOwner {
+			owner = s.TestAccs[1]
+		}
+
+		rewardReceiver := s.TestAccs[0]
+		if !test.param.isRewardReceiverAddressOwner {
+			rewardReceiver = s.TestAccs[1]
+		}
+
+		lockId := lock.ID
+		if !test.param.isLockOwner {
+			lockId = lockId + 1
+		}
+		// System under test
+		msg := types.NewMsgSetRewardReceiverAddress(owner, rewardReceiver, lockId)
+		resp, err := msgServer.SetRewardReceiverAddress(c, msg)
+		if !test.expectPass {
+			s.Require().Error(err)
+			s.Require().Equal(resp.Success, false)
+			return
+		}
+
+		s.Require().NoError(err)
+		s.Require().Equal(resp.Success, true)
+
+		// now check if the reward receiver address has been changed
+		newLock, err := s.App.LockupKeeper.GetLockByID(s.Ctx, lock.ID)
+		s.Require().NoError(err)
+		if test.param.isRewardReceiverAddressOwner {
+			s.Require().Equal(newLock.RewardReceiverAddress, "")
+		} else {
+			s.Require().Equal(newLock.RewardReceiverAddress, s.TestAccs[1].String())
+		}
+
+	}
+}

x/lockup/types/msgs.go

@@ -207,8 +207,8 @@ func (m MsgForceUnlock) GetSigners() []sdk.AccAddress {
 	return []sdk.AccAddress{owner}
 }
 
-// NewMsgBeginUnlockingAll creates a message to begin unlocking tokens.
-func NewMsgMsgSetRewardReceiverAddress(owner, rewardReceiver sdk.AccAddress, lockId uint64) *MsgSetRewardReceiverAddress {
+// NewMsgSetRewardReceiverAddress creates a message for setting reward receiver address
+func NewMsgSetRewardReceiverAddress(owner, rewardReceiver sdk.AccAddress, lockId uint64) *MsgSetRewardReceiverAddress {
 	return &MsgSetRewardReceiverAddress{
 		Owner:          owner.String(),
 		RewardReceiver: rewardReceiver.String(),

x/superfluid/keeper/migrate_test.go

@@ -260,28 +260,28 @@ func (s *KeeperTestSuite) TestMigrateSuperfluidBondedBalancerToConcentrated() {
 		"lock that is superfluid delegated, not unlocking (full shares)": {
 			percentOfSharesToMigrate: sdk.MustNewDecFromStr("1"),
 		},
-		// "lock that is superfluid delegated, not unlocking (partial shares)": {
-		// 	percentOfSharesToMigrate: sdk.MustNewDecFromStr("0.5"),
-		// },
-		// "error: migrate more shares than lock has": {
-		// 	percentOfSharesToMigrate: sdk.MustNewDecFromStr("1.1"),
-		// 	expectedError:            types.MigrateMoreSharesThanLockHasError{SharesToMigrate: "55000000000000000000", SharesInLock: "50000000000000000000"},
-		// },
-		// "error: invalid validator address": {
-		// 	overwriteValidatorAddress: true,
-		// 	percentOfSharesToMigrate:  sdk.MustNewDecFromStr("1"),
-		// 	expectedError:             fmt.Errorf("decoding bech32 failed: invalid checksum"),
-		// },
-		// "error: non-existent lock ID": {
-		// 	overwriteLockId:          true,
-		// 	percentOfSharesToMigrate: sdk.MustNewDecFromStr("1"),
-		// 	expectedError:            lockuptypes.ErrLockupNotFound,
-		// },
-		// "error: lock that is superfluid delegated, not unlocking (full shares), token out mins is more than exit coins": {
-		// 	percentOfSharesToMigrate: sdk.MustNewDecFromStr("1"),
-		// 	tokenOutMins:             sdk.NewCoins(sdk.NewCoin("foo", sdk.NewInt(100000))),
-		// 	expectedError:            gammtypes.ErrLimitMinAmount,
-		// },
+		"lock that is superfluid delegated, not unlocking (partial shares)": {
+			percentOfSharesToMigrate: sdk.MustNewDecFromStr("0.5"),
+		},
+		"error: migrate more shares than lock has": {
+			percentOfSharesToMigrate: sdk.MustNewDecFromStr("1.1"),
+			expectedError:            types.MigrateMoreSharesThanLockHasError{SharesToMigrate: "55000000000000000000", SharesInLock: "50000000000000000000"},
+		},
+		"error: invalid validator address": {
+			overwriteValidatorAddress: true,
+			percentOfSharesToMigrate:  sdk.MustNewDecFromStr("1"),
+			expectedError:             fmt.Errorf("decoding bech32 failed: invalid checksum"),
+		},
+		"error: non-existent lock ID": {
+			overwriteLockId:          true,
+			percentOfSharesToMigrate: sdk.MustNewDecFromStr("1"),
+			expectedError:            lockuptypes.ErrLockupNotFound,
+		},
+		"error: lock that is superfluid delegated, not unlocking (full shares), token out mins is more than exit coins": {
+			percentOfSharesToMigrate: sdk.MustNewDecFromStr("1"),
+			tokenOutMins:             sdk.NewCoins(sdk.NewCoin("foo", sdk.NewInt(100000))),
+			expectedError:            gammtypes.ErrLimitMinAmount,
+		},
 	}
 
 	for name, tc := range testCases {
@@ -405,11 +405,6 @@ func (s *KeeperTestSuite) TestMigrateSuperfluidBondedBalancerToConcentrated() {
 			s.Require().Equal(concentratedLockId, clSynthLock.UnderlyingLockId)
 
 			// Run slashing logic and check if the new and old locks are slashed.
-			// lastLockId := s.App.LockupKeeper.GetLastLockID(s.Ctx)
-			// fmt.Println("last lock id in test is: ", lastLockId)
-			lock, err := s.App.LockupKeeper.GetLockByID(s.Ctx, 2)
-			s.Require().NoError(err)
-			fmt.Println(lock.String())
 			s.SlashAndValidateResult(ctx, originalGammLockId, concentratedLockId, clPoolId, tc.percentOfSharesToMigrate, valAddr, *balancerLock, true)
 		})
 	}