Fix Account Migration for Additive Vesting Periods (#993)

* update acceptance test to fail

* update periodic vesting reset to reflect additive period lengths, not
absolute lengths

* simplify end time logic

* remove log

* test inclusion of v0_15 migration tests

.circleci/config.yml

@@ -71,7 +71,7 @@ jobs:
             export VERSION="$(git describe --tags --long | sed 's/v\(.*\)/\1/')"
             export GO111MODULE=on
             mkdir -p /tmp/logs /tmp/workspace/profiles
-            for pkg in $(go list ./... | grep -v 'simulation\|migrate\|contrib' | circleci tests split); do
+            for pkg in $(go list ./... | grep -v -P 'simulation|migrate(?!\/v0_15)|contrib' | circleci tests split); do
               id=$(echo "$pkg" | sed 's|[/.]|_|g')
               go test -mod=readonly -timeout 8m -race -coverprofile=/tmp/workspace/profiles/$id.out -covermode=atomic -tags='ledger test_ledger_mock' "$pkg" | tee "/tmp/logs/$id-$RANDOM.log"
             done

migrate/v0_15/account.go

@@ -31,31 +31,25 @@ func ResetPeriodicVestingAccount(vacc *vesting.PeriodicVestingAccount, startTime
 
 	newOriginalVesting := sdk.Coins{}
 	newStartTime := startTime.Unix()
-	newEndTime := newStartTime
 	newPeriods := vesting.Periods{}
 
 	for _, period := range vacc.VestingPeriods {
-		currentPeriod := currentPeriod + period.Length
+		currentPeriod = currentPeriod + period.Length
 
-		// Periods less than or equal to the newStartTime are still vesting,
+		// Periods less than the newStartTime are still vesting,
-		// so adjust their length and add them to them to the newPeriods
+		// so adjust their length and add them to the newPeriods
-		if newStartTime <= currentPeriod {
+		if newStartTime < currentPeriod {
-			period.Length = currentPeriod - newStartTime
+			// adjust the length of the first vesting period
+			// to be relative to the new start time
+			if len(newPeriods) == 0 {
+				period.Length = currentPeriod - newStartTime
+			}
 
-			newEndTime = newEndTime + period.Length
 			newOriginalVesting = newOriginalVesting.Add(period.Amount...)
-
 			newPeriods = append(newPeriods, period)
 		}
 	}
 
-	// In order to preserve the spendable amount of the account, we must drop
-	// the vesting funds if the start and end time are equal.
-	if newStartTime == newEndTime {
-		newOriginalVesting = sdk.Coins{}
-		newPeriods = vesting.Periods{}
-	}
-
 	// If the new original vesting amount is less than the delegated vesting amount, set delegated vesting
 	// to the new original vesting amount, and add the difference to the delegated free amount
 	for _, delegatedVestingCoin := range vacc.DelegatedVesting {
@@ -68,8 +62,13 @@ func ResetPeriodicVestingAccount(vacc *vesting.PeriodicVestingAccount, startTime
 		}
 	}
 
+	// update vesting account
 	vacc.StartTime = newStartTime
-	vacc.EndTime = newEndTime
 	vacc.OriginalVesting = newOriginalVesting
 	vacc.VestingPeriods = newPeriods
+
+	// ensure end time is >= start time
+	if vacc.StartTime >= vacc.EndTime {
+		vacc.EndTime = vacc.StartTime
+	}
 }

migrate/v0_15/account_test.go

@@ -58,7 +58,7 @@ func TestMigrateAccount_PeriodicVestingAccount_Vesting(t *testing.T) {
 			Amount: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1e6))),
 		},
 		vesting.Period{
-			Length: 45 * 24 * 60 * 60, // +15 days - vesting
+			Length: 30 * 24 * 60 * 60, // +15 days - vesting
 			Amount: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(2e6))),
 		},
 	}
@@ -186,7 +186,7 @@ func TestResetPeriodVestingAccount_SingleVestingPeriod_ExactStartTime(t *testing
 }
 
 func TestResetPeriodVestingAccount_MultiplePeriods(t *testing.T) {
-	balance := sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(3e6)))
+	balance := sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(4e6)))
 	vestingStartTime := time.Now().Add(-30 * 24 * time.Hour) // 30 days in past
 
 	periods := vesting.Periods{
@@ -195,11 +195,15 @@ func TestResetPeriodVestingAccount_MultiplePeriods(t *testing.T) {
 			Amount: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1e6))),
 		},
 		vesting.Period{
-			Length: 30 * 24 * 60 * 60, // 0 days - exact on the start time
+			Length: 15 * 24 * 60 * 60, // 0 days - exact on the start time
 			Amount: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1e6))),
 		},
 		vesting.Period{
-			Length: 45 * 24 * 60 * 60, // +15 days - vesting
+			Length: 15 * 24 * 60 * 60, // +15 days - vesting
+			Amount: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1e6))),
+		},
+		vesting.Period{
+			Length: 15 * 24 * 60 * 60, // +30 days - vesting
 			Amount: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1e6))),
 		},
 	}
@@ -212,11 +216,11 @@ func TestResetPeriodVestingAccount_MultiplePeriods(t *testing.T) {
 	ResetPeriodicVestingAccount(vacc, newVestingStartTime)
 
 	// new period length 15 days
-	expectedEndtime := newVestingStartTime.Add(15 * 24 * time.Hour).Unix()
+	expectedEndtime := newVestingStartTime.Add(30 * 24 * time.Hour).Unix()
 	// new period length changed, amount unchanged
 	expectedPeriods := vesting.Periods{
 		vesting.Period{
-			Length: 0, // 0 days
+			Length: 15 * 24 * 60 * 60, // 15 days
 			Amount: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1e6))),
 		},
 		vesting.Period{
@@ -242,11 +246,11 @@ func TestResetPeriodVestingAccount_DelegatedVesting_GreaterThanVesting(t *testin
 			Amount: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1e6))),
 		},
 		vesting.Period{
-			Length: 30 * 24 * 60 * 60, // 0 days - exact on the start time
+			Length: 15 * 24 * 60 * 60, // 0 days - exact on the start time
 			Amount: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1e6))),
 		},
 		vesting.Period{
-			Length: 45 * 24 * 60 * 60, // +15 days - vesting
+			Length: 15 * 24 * 60 * 60, // +15 days - vesting
 			Amount: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1e6))),
 		},
 	}
@@ -257,8 +261,8 @@ func TestResetPeriodVestingAccount_DelegatedVesting_GreaterThanVesting(t *testin
 	newVestingStartTime := vestingStartTime.Add(30 * 24 * time.Hour)
 	ResetPeriodicVestingAccount(vacc, newVestingStartTime)
 
-	assert.Equal(t, sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1e6))), vacc.DelegatedFree, "expected delegated free to be updated")
+	assert.Equal(t, sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(2e6))), vacc.DelegatedFree, "expected delegated free to be updated")
-	assert.Equal(t, sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(2e6))), vacc.DelegatedVesting, "expected delegated vesting to be updated")
+	assert.Equal(t, sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1e6))), vacc.DelegatedVesting, "expected delegated vesting to be updated")
 }
 
 func TestResetPeriodVestingAccount_DelegatedVesting_LessThanVested(t *testing.T) {
@@ -271,11 +275,11 @@ func TestResetPeriodVestingAccount_DelegatedVesting_LessThanVested(t *testing.T)
 			Amount: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1e6))),
 		},
 		vesting.Period{
-			Length: 30 * 24 * 60 * 60, // 0 days - exact on the start time
+			Length: 15 * 24 * 60 * 60, // 0 days - exact on the start time
 			Amount: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1e6))),
 		},
 		vesting.Period{
-			Length: 45 * 24 * 60 * 60, // +15 days - vesting
+			Length: 15 * 24 * 60 * 60, // +15 days - vesting
 			Amount: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1e6))),
 		},
 	}

migrate/v0_15/migrate_test.go

@@ -149,17 +149,21 @@ func TestAuth_AccountConversion(t *testing.T) {
 	bz, err := ioutil.ReadFile(filepath.Join("testdata", "kava-7-test-auth-state.json"))
 	require.NoError(t, err)
 
-	var genesisState auth.GenesisState
 	cdc := app.MakeCodec()
+
+	var genesisState auth.GenesisState
 	cdc.MustUnmarshalJSON(bz, &genesisState)
 
+	var originalGenesisState auth.GenesisState
+	cdc.MustUnmarshalJSON(bz, &originalGenesisState)
+
 	migratedGenesisState := Auth(genesisState, GenesisTime)
 	require.Equal(t, len(genesisState.Accounts), len(migratedGenesisState.Accounts), "expected the number of accounts after migration to be equal")
 	err = auth.ValidateGenesis(migratedGenesisState)
 	require.NoError(t, err, "expected migrated genesis to be valid")
 
 	for i, acc := range migratedGenesisState.Accounts {
-		oldAcc := genesisState.Accounts[i]
+		oldAcc := originalGenesisState.Accounts[i]
 
 		// total owned coins does not change
 		require.Equal(t, oldAcc.GetCoins(), acc.GetCoins(), "expected base coins to not change")
@@ -178,6 +182,9 @@ func TestAuth_AccountConversion(t *testing.T) {
 		// check 365 days
 		futureDate = GenesisTime.Add(365 * 24 * time.Hour)
 		require.Equal(t, oldAcc.SpendableCoins(futureDate), acc.SpendableCoins(futureDate), "expected spendable coins to not change")
+		// check 2 years
+		futureDate = GenesisTime.Add(2 * 365 * 24 * time.Hour)
+		require.Equal(t, oldAcc.SpendableCoins(futureDate), acc.SpendableCoins(futureDate), "expected spendable coins to not change")
 
 		if vacc, ok := acc.(*vesting.PeriodicVestingAccount); ok {
 			// old account must be a periodic vesting account
@@ -200,6 +207,9 @@ func TestAuth_AccountConversion(t *testing.T) {
 
 			// new account as less than or equal
 			require.LessOrEqual(t, len(vacc.VestingPeriods), len(oldVacc.VestingPeriods), "expected vesting periods of new account to be less than or equal to old")
+
+			// end time should not change
+			require.Equal(t, oldVacc.EndTime, vacc.EndTime, "expected end time to not change")
 		}
 	}
 }