add some comments

changelog.md

@@ -13,6 +13,10 @@
 
 * [2615](https://github.com/zeta-chain/node/pull/2615) - Refactor cleanup of outbound trackers
 
+### Fixes
+
+* [2672](https://github.com/zeta-chain/node/pull/2672) - check observer set for duplicates when adding a new observer or updating an existing one
+
 ## v19.0.0
 
 ### Breaking Changes

x/observer/keeper/msg_server_add_observer.go

@@ -52,13 +52,15 @@ func (k msgServer) AddObserver(
 		return &types.MsgAddObserverResponse{}, nil
 	}
 
+	// Add observer to the observer set
 	err = k.AddObserverToSet(ctx, msg.ObserverAddress)
 	if err != nil {
 		return &types.MsgAddObserverResponse{}, err
 	}
 	observerSet, _ := k.GetObserverSet(ctx)
 
 	k.SetLastObserverCount(ctx, &types.LastObserverCount{Count: observerSet.LenUint()})
+
 	EmitEventAddObserver(
 		ctx,
 		observerSet.LenUint(),

x/observer/keeper/msg_server_add_observer_test.go

@@ -52,7 +52,32 @@ func TestMsgServer_AddObserver(t *testing.T) {
 		require.Equal(t, &types.MsgAddObserverResponse{}, res)
 	})
 
-	t.Run("should add if add node account only false", func(t *testing.T) {
+	t.Run("unable to add observer if observer already exists", func(t *testing.T) {
+		k, ctx, _, _ := keepertest.ObserverKeeperWithMocks(t, keepertest.ObserverMockOptions{
+			UseAuthorityMock: true,
+		})
+		authorityMock := keepertest.GetObserverAuthorityMock(t, k)
+		admin := sample.AccAddress()
+		observerAddress := sample.AccAddress()
+		wctx := sdk.WrapSDKContext(ctx)
+
+		_, found := k.GetLastObserverCount(ctx)
+		require.False(t, found)
+		srv := keeper.NewMsgServerImpl(*k)
+		k.SetObserverSet(ctx, types.ObserverSet{ObserverList: []string{observerAddress}})
+
+		msg := types.MsgAddObserver{
+			Creator:                 admin,
+			ZetaclientGranteePubkey: sample.PubKeyString(),
+			AddNodeAccountOnly:      false,
+			ObserverAddress:         observerAddress,
+		}
+		keepertest.MockCheckAuthorization(&authorityMock.Mock, &msg, nil)
+		_, err := srv.AddObserver(wctx, &msg)
+		require.ErrorIs(t, err, types.ErrDuplicateObserver)
+	})
+
+	t.Run("should add observer if add node account only false", func(t *testing.T) {
 		k, ctx, _, _ := keepertest.ObserverKeeperWithMocks(t, keepertest.ObserverMockOptions{
 			UseAuthorityMock: true,
 		})

x/observer/keeper/msg_server_update_observer_test.go

@@ -73,6 +73,62 @@ func TestMsgServer_UpdateObserver(t *testing.T) {
 		require.Equal(t, newOperatorAddress.String(), acc.Operator)
 	})
 
+	t.Run(
+		"unable to update a tombstoned observer if the new address already exists in the observer set",
+		func(t *testing.T) {
+			k, ctx, _, _ := keepertest.ObserverKeeper(t)
+			srv := keeper.NewMsgServerImpl(*k)
+			// #nosec G404 test purpose - weak randomness is not an issue here
+			r := rand.New(rand.NewSource(9))
+
+			// Set validator in the store
+			validator := sample.Validator(t, r)
+			validatorNew := sample.Validator(t, r)
+			validatorNew.Status = stakingtypes.Bonded
+			k.GetStakingKeeper().SetValidator(ctx, validatorNew)
+			k.GetStakingKeeper().SetValidator(ctx, validator)
+
+			consAddress, err := validator.GetConsAddr()
+			require.NoError(t, err)
+			k.GetSlashingKeeper().SetValidatorSigningInfo(ctx, consAddress, slashingtypes.ValidatorSigningInfo{
+				Address:             consAddress.String(),
+				StartHeight:         0,
+				JailedUntil:         ctx.BlockHeader().Time.Add(1000000 * time.Second),
+				Tombstoned:          true,
+				MissedBlocksCounter: 1,
+			})
+
+			accAddressOfValidator, err := types.GetAccAddressFromOperatorAddress(validator.OperatorAddress)
+			require.NoError(t, err)
+
+			newOperatorAddress, err := types.GetAccAddressFromOperatorAddress(validatorNew.OperatorAddress)
+			require.NoError(t, err)
+
+			count := uint64(0)
+
+			k.SetObserverSet(ctx, types.ObserverSet{
+				ObserverList: []string{accAddressOfValidator.String(), newOperatorAddress.String()},
+			})
+			count = 1
+
+			k.SetNodeAccount(ctx, types.NodeAccount{
+				Operator: accAddressOfValidator.String(),
+			})
+
+			k.SetLastObserverCount(ctx, &types.LastObserverCount{
+				Count: count,
+			})
+
+			_, err = srv.UpdateObserver(sdk.WrapSDKContext(ctx), &types.MsgUpdateObserver{
+				Creator:            accAddressOfValidator.String(),
+				OldObserverAddress: accAddressOfValidator.String(),
+				NewObserverAddress: newOperatorAddress.String(),
+				UpdateReason:       types.ObserverUpdateReason_Tombstoned,
+			})
+			require.ErrorContains(t, err, types.ErrDuplicateObserver.Error())
+		},
+	)
+
 	t.Run("unable to update to a non validator address", func(t *testing.T) {
 		k, ctx, _, _ := keepertest.ObserverKeeper(t)
 		srv := keeper.NewMsgServerImpl(*k)

x/observer/keeper/observer_set.go

@@ -37,6 +37,7 @@ func (k Keeper) IsAddressPartOfObserverSet(ctx sdk.Context, address string) bool
 	return false
 }
 
+// AddObserverToSet adds an observer to the observer set.It makes sure the updated observer set is valid.
 func (k Keeper) AddObserverToSet(ctx sdk.Context, address string) error {
 	observerSet, found := k.GetObserverSet(ctx)
 	switch {
@@ -55,6 +56,7 @@ func (k Keeper) AddObserverToSet(ctx sdk.Context, address string) error {
 	return nil
 }
 
+// RemoveObserverFromSet removes an observer from the observer set.
 func (k Keeper) RemoveObserverFromSet(ctx sdk.Context, address string) {
 	observerSet, found := k.GetObserverSet(ctx)
 	if !found {
@@ -69,6 +71,7 @@ func (k Keeper) RemoveObserverFromSet(ctx sdk.Context, address string) {
 	}
 }
 
+// UpdateObserverAddress updates an observer address in the observer set.It makes sure the updated observer set is valid.
 func (k Keeper) UpdateObserverAddress(ctx sdk.Context, oldObserverAddress, newObserverAddress string) error {
 	observerSet, found := k.GetObserverSet(ctx)
 	if !found {