Merge pull request #365 from abdulla-ashurov/issue-344-repropose-upgrade-with-same-name

Issue 344 - Re-propose upgrade with same name

Author: ashcherbakov

docs/transactions.md

@@ -1639,6 +1639,8 @@ dcld tx dclupgrade propose-upgrade --name=<string> --upgrade-height=<int64> --fr
 dcld tx dclupgrade propose-upgrade --name=<string> --upgrade-height=<int64> --upgrade-info=<string> --from=<account>
 ```
 
+> **_Note:_**  If the current upgrade proposal is out of date(when the current network height is greater than the proposed upgrade height), we can resubmit the upgrade proposal with the same name.
+
 #### APPROVE_UPGRADE
 
 **Status: Implemented**

integration_tests/cli/upgrade-demo.sh

@@ -222,4 +222,87 @@ check_response "$approved_dclupgrade_query" "Not Found"
 
 test_divider
 
+# APPROVE UPGRADE'S PLAN HEIGHT LESS THAN BLOCK HEIGHT AND RE-PROPOSE UPGRADE PLAN HEIGHT BIGGER THAN BLOCK HEIGHT
+###########################################################################################################################################
+get_height current_height
+echo "Current height is $current_height"
+plan_height=$(expr $current_height + 2)
+echo "$plan_height"
+random_string upgrade_name
+random_string upgrade_info
+
+echo "propose upgrade's plan height bigger than block height"
+propose=$(dcld tx dclupgrade propose-upgrade --name=$upgrade_name --upgrade-height=$plan_height --upgrade-info=$upgrade_info --from jack --yes)
+echo "propose upgrade response: $propose"
+check_response "$propose" "\"code\": 0"
+
+proposed_dclupgrade_query=$(dcld query dclupgrade proposed-upgrade --name=$upgrade_name)
+echo "dclupgrade proposed upgrade query: $proposed_dclupgrade_query"
+check_response_and_report "$proposed_dclupgrade_query" "\"name\": \"$upgrade_name\""
+check_response_and_report "$proposed_dclupgrade_query" "\"height\": \"$plan_height\""
+check_response_and_report "$proposed_dclupgrade_query" "\"info\": \"$upgrade_info\""
+
+wait_for_height $(expr $plan_height + 5) 300 outage-safe
+get_height current_height
+echo "Current height is $current_height"
+
+echo "approve upgrade's plan height less than block height"
+approve=$(dcld tx dclupgrade approve-upgrade --name=$upgrade_name --from alice --yes)
+echo "approve upgrade response: $approve"
+check_response_and_report "$approve" "upgrade cannot be scheduled in the past" raw
+
+get_height current_height
+echo "Current height is $current_height"
+plan_height=$(expr $current_height + 3)
+
+echo "re-propose upgrade's plan height bigger than block height"
+propose=$(dcld tx dclupgrade propose-upgrade --name=$upgrade_name --upgrade-height=$plan_height --upgrade-info=$upgrade_info --from jack --yes)
+echo "propose upgrade response: $propose"
+check_response "$propose" "\"code\": 0"
+
+proposed_dclupgrade_query=$(dcld query dclupgrade proposed-upgrade --name=$upgrade_name)
+echo "dclupgrade proposed upgrade query: $proposed_dclupgrade_query"
+check_response_and_report "$proposed_dclupgrade_query" "\"name\": \"$upgrade_name\""
+check_response_and_report "$proposed_dclupgrade_query" "\"height\": \"$plan_height\""
+check_response_and_report "$proposed_dclupgrade_query" "\"info\": \"$upgrade_info\""
+###########################################################################################################################################
+
+test_divider
+
+# APPROVE UPGRADE'S PLAN HEIGHT LESS THAN BLOCK HEIGHT AND RE-PROPOSE UPGRADE PLAN HEIGHT LESS THAN BLOCK HEIGHT
+###########################################################################################################################################
+get_height current_height
+echo "Current height is $current_height"
+plan_height=$(expr $current_height + 3)
+random_string upgrade_name
+random_string upgrade_info
+
+echo "propose upgrade's plan height bigger than block height"
+propose=$(dcld tx dclupgrade propose-upgrade --name=$upgrade_name --upgrade-height=$plan_height --upgrade-info=$upgrade_info --from jack --yes)
+echo "propose upgrade response: $propose"
+check_response "$propose" "\"code\": 0"
+
+proposed_dclupgrade_query=$(dcld query dclupgrade proposed-upgrade --name=$upgrade_name)
+echo "dclupgrade proposed upgrade query: $proposed_dclupgrade_query"
+check_response_and_report "$proposed_dclupgrade_query" "\"name\": \"$upgrade_name\""
+check_response_and_report "$proposed_dclupgrade_query" "\"height\": \"$plan_height\""
+check_response_and_report "$proposed_dclupgrade_query" "\"info\": \"$upgrade_info\""
+
+wait_for_height $(expr $plan_height + 5) 300 outage-safe
+get_height current_height
+echo "Current height is $current_height"
+
+echo "approve upgrade's plan height less than block height"
+approve=$(dcld tx dclupgrade approve-upgrade --name=$upgrade_name --from alice --yes)
+echo "approve upgrade response: $approve"
+check_response_and_report "$approve" "upgrade cannot be scheduled in the past" raw
+
+echo "re-propose upgrade's plan height bigger than block height"
+propose=$(dcld tx dclupgrade propose-upgrade --name=$upgrade_name --upgrade-height=$plan_height --upgrade-info=$upgrade_info --from jack --yes)
+echo "propose upgrade response: $propose"
+check_response_and_report "$propose" "upgrade cannot be scheduled in the past" raw
+###########################################################################################################################################
+
+test_divider
+
 echo "PASSED"

x/dclupgrade/handler_test.go

@@ -843,6 +843,163 @@ func TestHandler_DoubleTimeRejectUpgrade(t *testing.T) {
 	require.False(t, isFound)
 }
 
+func TestHandler_DoubleTimeProposeUpgradePlanHeightBiggerThanBlockHeight(t *testing.T) {
+	setup := Setup(t)
+
+	trusteeAccAddress1 := testdata.GenerateAccAddress()
+	trusteeAccAddress2 := testdata.GenerateAccAddress()
+	trusteeAccAddress3 := testdata.GenerateAccAddress()
+
+	setup.AddAccount(trusteeAccAddress1, []dclauthtypes.AccountRole{dclauthtypes.Trustee})
+	setup.AddAccount(trusteeAccAddress2, []dclauthtypes.AccountRole{dclauthtypes.Trustee})
+	setup.AddAccount(trusteeAccAddress3, []dclauthtypes.AccountRole{dclauthtypes.Trustee})
+	setup.DclauthKeeper.On("CountAccountsWithRole", mock.Anything, dclauthtypes.Trustee).Return(3)
+
+	// propose new upgrade with plan height > block height
+	msgProposeUpgrade := NewMsgProposeUpgrade(trusteeAccAddress1)
+	msgProposeUpgrade.Plan.Height = 3
+	setup.Ctx = setup.Ctx.WithBlockHeight(1)
+
+	setup.UpgradeKeeper.On("ScheduleUpgrade", mock.Anything, msgProposeUpgrade.Plan).Return(nil).Once()
+	_, err := setup.Handler(setup.Ctx, msgProposeUpgrade)
+	require.NoError(t, err)
+
+	// second time propose upgrade same plan name with plan height > block height
+	msgProposeUpgrade = NewMsgProposeUpgrade(trusteeAccAddress2)
+	msgProposeUpgrade.Plan.Height = 5
+	setup.Ctx = setup.Ctx.WithBlockHeight(2)
+
+	setup.UpgradeKeeper.On("ScheduleUpgrade", mock.Anything, msgProposeUpgrade.Plan).Return(nil).Once()
+	_, err = setup.Handler(setup.Ctx, msgProposeUpgrade)
+	require.Error(t, err)
+	require.True(t, types.ErrProposedUpgradeAlreadyExists.Is(err))
+}
+
+func TestHandler_ApproveUpgradePlanHeightLessThanBlockHeight_And_ReProposeUpgradePlanHeightBiggerThanBlockHeight(t *testing.T) {
+	setup := Setup(t)
+
+	trusteeAccAddress1 := testdata.GenerateAccAddress()
+	trusteeAccAddress2 := testdata.GenerateAccAddress()
+	trusteeAccAddress3 := testdata.GenerateAccAddress()
+	setup.AddAccount(trusteeAccAddress1, []dclauthtypes.AccountRole{dclauthtypes.Trustee})
+	setup.AddAccount(trusteeAccAddress2, []dclauthtypes.AccountRole{dclauthtypes.Trustee})
+	setup.AddAccount(trusteeAccAddress3, []dclauthtypes.AccountRole{dclauthtypes.Trustee})
+	setup.DclauthKeeper.On("CountAccountsWithRole", mock.Anything, dclauthtypes.Trustee).Return(3)
+
+	// propose new upgrade
+	proposeUpgrade := NewMsgProposeUpgrade(trusteeAccAddress1)
+	proposeUpgrade.Plan.Height = 2
+	setup.Ctx = setup.Ctx.WithBlockHeight(1)
+
+	setup.UpgradeKeeper.On("ScheduleUpgrade", mock.Anything, proposeUpgrade.Plan).Return(nil).Once()
+	_, err := setup.Handler(setup.Ctx, proposeUpgrade)
+	require.NoError(t, err)
+
+	// check first proposed upgrade for being created
+	proposedUpgrade, isFound := setup.Keeper.GetProposedUpgrade(setup.Ctx, proposeUpgrade.Plan.Name)
+	require.True(t, isFound)
+
+	require.Equal(t, proposeUpgrade.Plan, proposedUpgrade.Plan)
+	require.Equal(t, proposeUpgrade.Creator, proposedUpgrade.Creator)
+
+	require.Equal(t, 1, len(proposedUpgrade.Approvals))
+
+	require.Equal(t, proposeUpgrade.Creator, proposedUpgrade.Approvals[0].Address)
+	require.Equal(t, proposeUpgrade.Time, proposedUpgrade.Approvals[0].Time)
+	require.Equal(t, proposeUpgrade.Info, proposedUpgrade.Approvals[0].Info)
+
+	// create approve message from trustee2
+	msgApproveUpgrade := NewMsgApproveUpgrade(trusteeAccAddress2)
+
+	// approve new upgrade with plan height < block height
+	setup.Ctx = setup.Ctx.WithBlockHeight(3)
+	setup.UpgradeKeeper.On("ScheduleUpgrade", mock.Anything, proposeUpgrade.Plan).Return(sdkerrors.ErrInvalidRequest).Once()
+	_, err = setup.Handler(setup.Ctx, msgApproveUpgrade)
+	require.Error(t, err, sdkerrors.ErrInvalidRequest)
+
+	// check upgrade for not being added to ApprovedUpgrade store
+	_, isFound = setup.Keeper.GetApprovedUpgrade(setup.Ctx, proposeUpgrade.Plan.Name)
+	require.False(t, isFound)
+
+	// second time propose upgrade with same name with propose height > current height
+	proposeUpgrade = NewMsgProposeUpgrade(trusteeAccAddress3)
+	proposeUpgrade.Plan.Height = 5
+
+	setup.UpgradeKeeper.On("ScheduleUpgrade", mock.Anything, proposeUpgrade.Plan).Return(nil).Once()
+	_, err = setup.Handler(setup.Ctx, proposeUpgrade)
+	require.NoError(t, err)
+
+	// check second proposed upgrade for being created
+	proposedUpgrade, isFound = setup.Keeper.GetProposedUpgrade(setup.Ctx, proposeUpgrade.Plan.Name)
+	require.True(t, isFound)
+
+	require.Equal(t, proposeUpgrade.Plan, proposedUpgrade.Plan)
+	require.Equal(t, proposeUpgrade.Creator, proposedUpgrade.Creator)
+
+	require.Equal(t, 1, len(proposedUpgrade.Approvals))
+
+	require.Equal(t, proposeUpgrade.Creator, proposedUpgrade.Approvals[0].Address)
+	require.Equal(t, proposeUpgrade.Time, proposedUpgrade.Approvals[0].Time)
+	require.Equal(t, proposeUpgrade.Info, proposedUpgrade.Approvals[0].Info)
+}
+
+func TestHandler_ApproveUpgradePlanHeightLessThanBlockHeight_And_ReProposeUpgradePlanHeightLessThanBlockHeight(t *testing.T) {
+	setup := Setup(t)
+
+	trusteeAccAddress1 := testdata.GenerateAccAddress()
+	trusteeAccAddress2 := testdata.GenerateAccAddress()
+	trusteeAccAddress3 := testdata.GenerateAccAddress()
+	setup.AddAccount(trusteeAccAddress1, []dclauthtypes.AccountRole{dclauthtypes.Trustee})
+	setup.AddAccount(trusteeAccAddress2, []dclauthtypes.AccountRole{dclauthtypes.Trustee})
+	setup.AddAccount(trusteeAccAddress3, []dclauthtypes.AccountRole{dclauthtypes.Trustee})
+	setup.DclauthKeeper.On("CountAccountsWithRole", mock.Anything, dclauthtypes.Trustee).Return(3)
+
+	// propose new upgrade
+	proposeUpgrade := NewMsgProposeUpgrade(trusteeAccAddress1)
+	proposeUpgrade.Plan.Height = 2
+	setup.Ctx = setup.Ctx.WithBlockHeight(1)
+
+	setup.UpgradeKeeper.On("ScheduleUpgrade", mock.Anything, proposeUpgrade.Plan).Return(nil).Once()
+	_, err := setup.Handler(setup.Ctx, proposeUpgrade)
+	require.NoError(t, err)
+
+	// check first proposed upgrade for being created
+	proposedUpgrade, isFound := setup.Keeper.GetProposedUpgrade(setup.Ctx, proposeUpgrade.Plan.Name)
+	require.True(t, isFound)
+
+	require.Equal(t, proposeUpgrade.Plan, proposedUpgrade.Plan)
+	require.Equal(t, proposeUpgrade.Creator, proposedUpgrade.Creator)
+
+	require.Equal(t, 1, len(proposedUpgrade.Approvals))
+
+	require.Equal(t, proposeUpgrade.Creator, proposedUpgrade.Approvals[0].Address)
+	require.Equal(t, proposeUpgrade.Time, proposedUpgrade.Approvals[0].Time)
+	require.Equal(t, proposeUpgrade.Info, proposedUpgrade.Approvals[0].Info)
+
+	// create approve message from trustee2
+	msgApproveUpgrade := NewMsgApproveUpgrade(trusteeAccAddress2)
+
+	// approve new upgrade with plan height < block height
+	setup.Ctx = setup.Ctx.WithBlockHeight(3)
+	setup.UpgradeKeeper.On("ScheduleUpgrade", mock.Anything, proposeUpgrade.Plan).Return(sdkerrors.ErrInvalidRequest).Once()
+	_, err = setup.Handler(setup.Ctx, msgApproveUpgrade)
+	require.Error(t, err, sdkerrors.ErrInvalidRequest)
+
+	// check upgrade for not being added to ApprovedUpgrade store
+	_, isFound = setup.Keeper.GetApprovedUpgrade(setup.Ctx, proposeUpgrade.Plan.Name)
+	require.False(t, isFound)
+
+	// second time propose upgrade with same name with propose height < current height
+	proposeUpgrade = NewMsgProposeUpgrade(trusteeAccAddress3)
+	proposeUpgrade.Plan.Height = 3
+	setup.Ctx = setup.Ctx.WithBlockHeight(5)
+
+	setup.UpgradeKeeper.On("ScheduleUpgrade", mock.Anything, proposeUpgrade.Plan).Return(types.ErrProposedUpgradeAlreadyExists).Once()
+	_, err = setup.Handler(setup.Ctx, proposeUpgrade)
+	require.Error(t, err)
+	require.True(t, types.ErrProposedUpgradeAlreadyExists.Is(err))
+}
+
 func isContextWithCachedMultiStore(ctx sdk.Context) bool {
 	_, ok := ctx.MultiStore().(storetypes.CacheMultiStore)
 

x/dclupgrade/keeper/msg_server_approve_upgrade.go

@@ -69,6 +69,14 @@ func (k msgServer) ApproveUpgrade(goCtx context.Context, msg *types.MsgApproveUp
 		approvedUpgrage := types.ApprovedUpgrade(proposedUpgrade)
 		k.SetApprovedUpgrade(ctx, approvedUpgrage)
 	} else {
+		// Execute scheduling upgrade in a new context branch (with branched store)
+		// to validate msg.Plan before the proposal proceeds through the approval process.
+		// State is not persisted.
+		cacheCtx, _ := ctx.CacheContext()
+		err = k.upgradeKeeper.ScheduleUpgrade(cacheCtx, proposedUpgrade.Plan)
+		if err != nil {
+			return nil, err
+		}
 		// update proposed upgrade
 		k.SetProposedUpgrade(ctx, proposedUpgrade)
 	}

x/dclupgrade/keeper/msg_server_propose_upgrade.go

@@ -24,18 +24,6 @@ func (k msgServer) ProposeUpgrade(goCtx context.Context, msg *types.MsgProposeUp
 		)
 	}
 
-	// check if proposed upgrade with the same name already exists
-	_, isFound := k.GetProposedUpgrade(ctx, msg.Plan.Name)
-	if isFound {
-		return nil, types.NewErrProposedUpgradeAlreadyExists(msg.Plan.Name)
-	}
-
-	// check if approved upgrade with the same name already exists
-	_, isFound = k.GetApprovedUpgrade(ctx, msg.Plan.Name)
-	if isFound {
-		return nil, types.NewErrApprovedUpgradeAlreadyExists(msg.Plan.Name)
-	}
-
 	// Execute scheduling upgrade in a new context branch (with branched store)
 	// to validate msg.Plan before the proposal proceeds through the approval process.
 	// State is not persisted.
@@ -45,6 +33,20 @@ func (k msgServer) ProposeUpgrade(goCtx context.Context, msg *types.MsgProposeUp
 		return nil, err
 	}
 
+	// check if proposed upgrade with the same name already exists and propose plan height
+	existingProposedUpgrade, isFound := k.GetProposedUpgrade(ctx, msg.Plan.Name)
+	if isFound && ctx.BlockHeight() > existingProposedUpgrade.Plan.Height {
+		k.RemoveProposedUpgrade(ctx, msg.Plan.Name)
+	} else if isFound {
+		return nil, types.NewErrProposedUpgradeAlreadyExists(msg.Plan.Name)
+	}
+
+	// check if approved upgrade with the same name already exists
+	_, isFound = k.GetApprovedUpgrade(ctx, msg.Plan.Name)
+	if isFound {
+		return nil, types.NewErrApprovedUpgradeAlreadyExists(msg.Plan.Name)
+	}
+
 	grant := types.Grant{
 		Address: creatorAddr.String(),
 		Time:    msg.Time,