handle multiple upgrades for the same height in chain provider

internal/pkg/provider/chain/chain.go

@@ -2,23 +2,28 @@ package chain
 
 import (
 	"context"
-	"slices"
+	"sort"
 
-	"blazar/internal/pkg/cosmos"
 	"blazar/internal/pkg/errors"
 	urproto "blazar/internal/pkg/proto/upgrades_registry"
 	"blazar/internal/pkg/provider"
 
 	v1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1"
+	"github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1"
 )
 
+type CosmosProposalsProvider interface {
+	GetProposalsV1(ctx context.Context) (v1.Proposals, error)
+	GetProposalsV1beta1(ctx context.Context) (v1beta1.Proposals, error)
+}
+
 type Provider struct {
-	cosmosClient *cosmos.Client
+	cosmosClient CosmosProposalsProvider
 	chain        string
 	priority     int32
 }
 
-func NewProvider(cosmosClient *cosmos.Client, chain string, priority int32) *Provider {
+func NewProvider(cosmosClient CosmosProposalsProvider, chain string, priority int32) *Provider {
 	return &Provider{
 		cosmosClient: cosmosClient,
 		chain:        chain,
@@ -32,23 +37,60 @@ func (p *Provider) GetUpgrades(ctx context.Context) ([]*urproto.Upgrade, error)
 		return []*urproto.Upgrade{}, err
 	}
 
-	// cosmos-sdk allows changing parameters of a previously passed upgrade
-	// by creating a new upgrade proposal with the same name in upgrade plan
-	// https://github.com/cosmos/cosmos-sdk/blob/41f92723399ef0affa90c6b3d8e7b47b82361280/x/upgrade/keeper/keeper.go#L185
-	// since, upgrades is sorted by proposal ID and we'll only keep last instance for a name
-	// if a passed upgrade already exists for that name
-	passedNames := make(map[string]struct{}, len(upgrades))
-	filtered := make([]chainUpgrade, 0, len(upgrades))
-	slices.Reverse(upgrades)
+	// Blazar expects one upgrade per height, but the governance allows to create multiple proposals for the same height
+	// In the end only one upgrade will be expecuted at given height, no matter how many software upgrades proposals are registered onchain
+	// The most common case for having more than one proposal is when someone create a new proposal and asks everyone to vote-no on the previous one
+	// due to invalid data etc.
+	//
+	// To handle this case we pick the last proposal for each height with some conditions:
+	// 1. if there is a proposal in PASSED state, we pick it
+	// 2. if there are two equal proposals say in VOTING_PERIOD state, we pick the one with the highest proposal id
+
+	// sort upgrades in descending order by proposal id
+	sort.Slice(upgrades, func(i, j int) bool {
+		return upgrades[i].ProposalID > upgrades[j].ProposalID
+	})
+
+	upgradesByHeight := make(map[int64][]chainUpgrade)
 	for _, upgrade := range upgrades {
-		if _, ok := passedNames[upgrade.Name]; !ok {
+		if _, ok := upgradesByHeight[upgrade.Height]; !ok {
+			upgradesByHeight[upgrade.Height] = make([]chainUpgrade, 0)
+		}
+		upgradesByHeight[upgrade.Height] = append(upgradesByHeight[upgrade.Height], upgrade)
+	}
+
+	filtered := make([]chainUpgrade, 0, len(upgrades))
+	for _, upgradesForHeight := range upgradesByHeight {
+		// if there is only one upgrade for the height, we don't need to do anything
+		if len(upgradesForHeight) == 1 {
+			filtered = append(filtered, upgradesForHeight[0])
+			continue
+		}
+
+		// if there is a passed upgrade, we pick it
+		foundPassed := false
+		for _, upgrade := range upgradesForHeight {
+			// the upgrades are sorted by proposal id in descending order
+			// so the first upgrade in the list is the one with the highest
+			// proposal id (in case there are two PASSED proposals for the same height)
 			if upgrade.Status == PASSED {
-				passedNames[upgrade.Name] = struct{}{}
+				foundPassed = true
+				filtered = append(filtered, upgrade)
+				break
 			}
-			filtered = append(filtered, upgrade)
+		}
+
+		// if there is no passed upgrade, we pick the one with the highest proposal id
+		if !foundPassed {
+			filtered = append(filtered, upgradesForHeight[0])
 		}
 	}
 
+	// sort upgrades in descending order by proposal id because iterating over map doesn't guarantee order
+	sort.Slice(filtered, func(i, j int) bool {
+		return filtered[i].ProposalID > filtered[j].ProposalID
+	})
+
 	return toProto(filtered, p.priority), nil
 }
 

internal/pkg/provider/chain/chain_test.go

@@ -0,0 +1,186 @@
+package chain
+
+import (
+	"context"
+	"fmt"
+	"testing"
+	"time"
+
+	urproto "blazar/internal/pkg/proto/upgrades_registry"
+
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	v1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1"
+	"github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1"
+	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+type mockCosmosClient struct {
+	v1Proposals      v1.Proposals
+	v1beta1Proposals v1beta1.Proposals
+}
+
+func (m *mockCosmosClient) GetProposalsV1(_ context.Context) (v1.Proposals, error) {
+	return m.v1Proposals, nil
+}
+
+func (m *mockCosmosClient) GetProposalsV1beta1(_ context.Context) (v1beta1.Proposals, error) {
+	return m.v1beta1Proposals, nil
+}
+
+func TestGetUpgrades(t *testing.T) {
+	tests := []struct {
+		name      string
+		proposals v1.Proposals
+		expected  []*urproto.Upgrade
+	}{
+		{
+			name:      "EmptyProposals",
+			proposals: v1.Proposals{},
+			expected:  []*urproto.Upgrade{},
+		},
+		{
+			name: "Simple",
+			proposals: v1.Proposals{
+				newProposal(t, 1, 100, v1.StatusPassed),
+				newProposal(t, 2, 200, v1.StatusVotingPeriod),
+			},
+			expected: []*urproto.Upgrade{
+				{
+					Height: 200,
+					Type:   urproto.UpgradeType_GOVERNANCE,
+					Status: urproto.UpgradeStatus_SCHEDULED,
+					Source: urproto.ProviderType_CHAIN,
+				},
+				{
+					Height: 100,
+					Type:   urproto.UpgradeType_GOVERNANCE,
+					Status: urproto.UpgradeStatus_ACTIVE,
+					Source: urproto.ProviderType_CHAIN,
+				},
+			},
+		},
+		{
+			name: "DuplicateProposalsWithPassedStatus",
+			proposals: v1.Proposals{
+				newProposal(t, 1, 100, v1.StatusPassed),
+				newProposal(t, 2, 100, v1.StatusPassed),
+				newProposal(t, 3, 200, v1.StatusPassed),
+			},
+			expected: []*urproto.Upgrade{
+				{
+					Height: 200,
+					Type:   urproto.UpgradeType_GOVERNANCE,
+					Status: urproto.UpgradeStatus_ACTIVE,
+					Source: urproto.ProviderType_CHAIN,
+				},
+				{
+					Height: 100,
+					Type:   urproto.UpgradeType_GOVERNANCE,
+					Status: urproto.UpgradeStatus_ACTIVE,
+					Source: urproto.ProviderType_CHAIN,
+					// the latest proposal in passed state should be returned
+					ProposalId: int64ptr(2),
+				},
+			},
+		},
+		{
+			name: "DuplicateProposalsInVotingPeriod",
+			proposals: v1.Proposals{
+				newProposal(t, 1, 100, v1.StatusVotingPeriod),
+				newProposal(t, 2, 100, v1.StatusVotingPeriod),
+				newProposal(t, 3, 200, v1.StatusDepositPeriod),
+			},
+			expected: []*urproto.Upgrade{
+				{
+					Height: 200,
+					Type:   urproto.UpgradeType_GOVERNANCE,
+					Status: urproto.UpgradeStatus_SCHEDULED,
+					Source: urproto.ProviderType_CHAIN,
+				},
+				{
+					Height: 100,
+					Type:   urproto.UpgradeType_GOVERNANCE,
+					Status: urproto.UpgradeStatus_SCHEDULED,
+					Source: urproto.ProviderType_CHAIN,
+					// in case of two equal proposals in non-active state we expect the one with the highest proposal id
+					ProposalId: int64ptr(2),
+				},
+			},
+		},
+		{
+			name: "DuplicateProposalsInActiveAndVotingPeriod",
+			proposals: v1.Proposals{
+				newProposal(t, 1, 100, v1.StatusPassed),
+				newProposal(t, 2, 100, v1.StatusVotingPeriod),
+				newProposal(t, 3, 200, v1.StatusDepositPeriod),
+			},
+			expected: []*urproto.Upgrade{
+				{
+					Height: 200,
+					Type:   urproto.UpgradeType_GOVERNANCE,
+					Status: urproto.UpgradeStatus_SCHEDULED,
+					Source: urproto.ProviderType_CHAIN,
+				},
+				{
+					Height: 100,
+					Type:   urproto.UpgradeType_GOVERNANCE,
+					Status: urproto.UpgradeStatus_ACTIVE,
+					Source: urproto.ProviderType_CHAIN,
+					// in case of two proposals where one is in active state and the other in non-active state
+					// we expect the one in active state
+					ProposalId: int64ptr(1),
+				},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cosmosClient := &mockCosmosClient{
+				v1Proposals: tt.proposals,
+			}
+			provider := NewProvider(cosmosClient, "test-chain", 1)
+
+			upgrades, err := provider.GetUpgrades(context.Background())
+			require.NoError(t, err)
+
+			assert.Equal(t, len(upgrades), len(tt.expected))
+
+			for i, upgrade := range upgrades {
+				assert.Equal(t, tt.expected[i].Height, upgrade.Height)
+				assert.Equal(t, tt.expected[i].Type, upgrade.Type)
+				assert.Equal(t, tt.expected[i].Status, upgrade.Status)
+				assert.Equal(t, tt.expected[i].Source, upgrade.Source)
+
+				if tt.expected[i].ProposalId != nil {
+					assert.Equal(t, *tt.expected[i].ProposalId, *upgrade.ProposalId)
+				}
+			}
+		})
+	}
+}
+
+func newProposal(t *testing.T, id uint64, height int64, status v1.ProposalStatus) *v1.Proposal {
+	sup := &upgradetypes.MsgSoftwareUpgrade{
+		Authority: "x/gov",
+		Plan: upgradetypes.Plan{
+			Name:   fmt.Sprintf("test upgrade: %d", height),
+			Time:   time.Now().Add(30 * time.Minute),
+			Info:   "test upgrade info",
+			Height: height,
+		},
+	}
+
+	proposal, err := v1.NewProposal([]sdk.Msg{sup}, id, time.Now(), time.Now(), "", "title", "summary", sdk.AccAddress{})
+	require.NoError(t, err)
+
+	proposal.Status = status
+
+	return &proposal
+}
+
+func int64ptr(i int64) *int64 {
+	return &i
+}