From b98e720925d1ac7cbe2b6774f4e2abd4f70f266b Mon Sep 17 00:00:00 2001 From: Tanmay Date: Fri, 9 Aug 2024 11:30:50 -0400 Subject: [PATCH] add some comments --- changelog.md | 4 ++ x/observer/keeper/msg_server_add_observer.go | 2 + .../keeper/msg_server_add_observer_test.go | 27 ++++++++- .../keeper/msg_server_update_observer_test.go | 56 +++++++++++++++++++ x/observer/keeper/observer_set.go | 3 + 5 files changed, 91 insertions(+), 1 deletion(-) diff --git a/changelog.md b/changelog.md index 73cf2331c8..28dd19e9e1 100644 --- a/changelog.md +++ b/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 diff --git a/x/observer/keeper/msg_server_add_observer.go b/x/observer/keeper/msg_server_add_observer.go index 411a461197..fe4e85aedc 100644 --- a/x/observer/keeper/msg_server_add_observer.go +++ b/x/observer/keeper/msg_server_add_observer.go @@ -52,6 +52,7 @@ 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 @@ -59,6 +60,7 @@ func (k msgServer) AddObserver( observerSet, _ := k.GetObserverSet(ctx) k.SetLastObserverCount(ctx, &types.LastObserverCount{Count: observerSet.LenUint()}) + EmitEventAddObserver( ctx, observerSet.LenUint(), diff --git a/x/observer/keeper/msg_server_add_observer_test.go b/x/observer/keeper/msg_server_add_observer_test.go index b26a2fe5d0..a135d0c324 100644 --- a/x/observer/keeper/msg_server_add_observer_test.go +++ b/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, }) diff --git a/x/observer/keeper/msg_server_update_observer_test.go b/x/observer/keeper/msg_server_update_observer_test.go index 2a4308590f..5ac3654746 100644 --- a/x/observer/keeper/msg_server_update_observer_test.go +++ b/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) diff --git a/x/observer/keeper/observer_set.go b/x/observer/keeper/observer_set.go index 2180759c9e..01045a4151 100644 --- a/x/observer/keeper/observer_set.go +++ b/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 {