From 415d354bcf7305b9cc646d89e72172633e66fbf9 Mon Sep 17 00:00:00 2001 From: Tanmay Date: Wed, 7 Aug 2024 16:23:12 -0400 Subject: [PATCH 01/12] add duplicate check to validate observer set --- x/observer/keeper/msg_server_add_observer.go | 5 ++- x/observer/keeper/observer_set.go | 31 ++++++++++----- x/observer/keeper/observer_set_test.go | 20 +++++++++- x/observer/types/errors.go | 2 + x/observer/types/observer_set.go | 9 +++++ x/observer/types/observer_set_test.go | 42 +++++++++++++++----- 6 files changed, 86 insertions(+), 23 deletions(-) diff --git a/x/observer/keeper/msg_server_add_observer.go b/x/observer/keeper/msg_server_add_observer.go index 11f7701dde..411a461197 100644 --- a/x/observer/keeper/msg_server_add_observer.go +++ b/x/observer/keeper/msg_server_add_observer.go @@ -52,7 +52,10 @@ func (k msgServer) AddObserver( return &types.MsgAddObserverResponse{}, nil } - k.AddObserverToSet(ctx, msg.ObserverAddress) + 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()}) diff --git a/x/observer/keeper/observer_set.go b/x/observer/keeper/observer_set.go index c8a22e0e0f..e0b28142b9 100644 --- a/x/observer/keeper/observer_set.go +++ b/x/observer/keeper/observer_set.go @@ -1,6 +1,7 @@ package keeper import ( + "cosmossdk.io/errors" "github.com/cosmos/cosmos-sdk/store/prefix" sdk "github.com/cosmos/cosmos-sdk/types" @@ -36,21 +37,21 @@ func (k Keeper) IsAddressPartOfObserverSet(ctx sdk.Context, address string) bool return false } -func (k Keeper) AddObserverToSet(ctx sdk.Context, address string) { +func (k Keeper) AddObserverToSet(ctx sdk.Context, address string) error { observerSet, found := k.GetObserverSet(ctx) if !found { k.SetObserverSet(ctx, types.ObserverSet{ ObserverList: []string{address}, }) - return - } - for _, addr := range observerSet.ObserverList { - if addr == address { - return - } + return nil } observerSet.ObserverList = append(observerSet.ObserverList, address) + err := observerSet.Validate() + if err != nil { + return err + } k.SetObserverSet(ctx, observerSet) + return nil } func (k Keeper) RemoveObserverFromSet(ctx sdk.Context, address string) { @@ -72,12 +73,22 @@ func (k Keeper) UpdateObserverAddress(ctx sdk.Context, oldObserverAddress, newOb if !found { return types.ErrObserverSetNotFound } + found = false for i, addr := range observerSet.ObserverList { if addr == oldObserverAddress { observerSet.ObserverList[i] = newObserverAddress - k.SetObserverSet(ctx, observerSet) - return nil + found = true + break } } - return types.ErrUpdateObserver + if !found { + return types.ErrObserverNotFound + } + + err := observerSet.Validate() + if err != nil { + return errors.Wrap(types.ErrUpdateObserver, err.Error()) + } + k.SetObserverSet(ctx, observerSet) + return nil } diff --git a/x/observer/keeper/observer_set_test.go b/x/observer/keeper/observer_set_test.go index 3ae4a99cf2..0718aea2b4 100644 --- a/x/observer/keeper/observer_set_test.go +++ b/x/observer/keeper/observer_set_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/require" + "github.com/zeta-chain/zetacore/x/observer/types" keepertest "github.com/zeta-chain/zetacore/testutil/keeper" "github.com/zeta-chain/zetacore/testutil/sample" @@ -39,7 +40,8 @@ func TestKeeper_AddObserverToSet(t *testing.T) { os := sample.ObserverSet(10) k.SetObserverSet(ctx, os) newObserver := sample.AccAddress() - k.AddObserverToSet(ctx, newObserver) + err := k.AddObserverToSet(ctx, newObserver) + require.NoError(t, err) require.True(t, k.IsAddressPartOfObserverSet(ctx, newObserver)) require.False(t, k.IsAddressPartOfObserverSet(ctx, sample.AccAddress())) osNew, found := k.GetObserverSet(ctx) @@ -95,6 +97,20 @@ func TestKeeper_UpdateObserverAddress(t *testing.T) { require.True(t, found) require.Equal(t, newObserverAddress, observerSet.ObserverList[len(observerSet.ObserverList)-1]) }) + t.Run("unable to update observer list if the new list is not valid", func(t *testing.T) { + k, ctx, _, _ := keepertest.ObserverKeeper(t) + oldObserverAddress := sample.AccAddress() + newObserverAddress := sample.AccAddress() + observerSet := sample.ObserverSet(10) + observerSet.ObserverList = append(observerSet.ObserverList, []string{oldObserverAddress, newObserverAddress}...) + + err := k.UpdateObserverAddress(ctx, oldObserverAddress, newObserverAddress) + require.ErrorIs(t, err, types.ErrObserverSetNotFound) + k.SetObserverSet(ctx, observerSet) + + err = k.UpdateObserverAddress(ctx, oldObserverAddress, newObserverAddress) + require.ErrorContains(t, err, types.ErrDuplicateObserver.Error()) + }) t.Run("should error if observer address not found", func(t *testing.T) { k, ctx, _, _ := keepertest.ObserverKeeper(t) oldObserverAddress := sample.AccAddress() @@ -103,7 +119,7 @@ func TestKeeper_UpdateObserverAddress(t *testing.T) { observerSet.ObserverList = append(observerSet.ObserverList, oldObserverAddress) k.SetObserverSet(ctx, observerSet) err := k.UpdateObserverAddress(ctx, sample.AccAddress(), newObserverAddress) - require.Error(t, err) + require.ErrorIs(t, err, types.ErrObserverNotFound) }) t.Run("update observer address long observerList", func(t *testing.T) { k, ctx, _, _ := keepertest.ObserverKeeper(t) diff --git a/x/observer/types/errors.go b/x/observer/types/errors.go index 6485e613ed..a997267657 100644 --- a/x/observer/types/errors.go +++ b/x/observer/types/errors.go @@ -49,4 +49,6 @@ var ( ErrInboundDisabled = errorsmod.Register(ModuleName, 1132, "inbound tx processing is disabled") ErrInvalidZetaCoinTypes = errorsmod.Register(ModuleName, 1133, "invalid zeta coin types") ErrNotObserver = errorsmod.Register(ModuleName, 1134, "sender is not an observer") + ErrDuplicateObserver = errorsmod.Register(ModuleName, 1135, "observer already exists") + ErrObserverNotFound = errorsmod.Register(ModuleName, 1136, "observer not found") ) diff --git a/x/observer/types/observer_set.go b/x/observer/types/observer_set.go index ffa2d1c05a..0e68cb5ec0 100644 --- a/x/observer/types/observer_set.go +++ b/x/observer/types/observer_set.go @@ -1,6 +1,7 @@ package types import ( + "cosmossdk.io/errors" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/zeta-chain/zetacore/pkg/chains" @@ -22,6 +23,14 @@ func (m *ObserverSet) Validate() error { return err } } + // Check for duplicates + observers := make(map[string]bool) + for _, observerAddress := range m.ObserverList { + if _, ok := observers[observerAddress]; ok { + return errors.Wrapf(ErrDuplicateObserver, "observer %s", observerAddress) + } + observers[observerAddress] = true + } return nil } diff --git a/x/observer/types/observer_set_test.go b/x/observer/types/observer_set_test.go index 69a9a19f96..8344757b18 100644 --- a/x/observer/types/observer_set_test.go +++ b/x/observer/types/observer_set_test.go @@ -10,17 +10,39 @@ import ( "github.com/zeta-chain/zetacore/x/observer/types" ) -func TestObserverSet(t *testing.T) { - observerSet := sample.ObserverSet(4) +func TestObserverSet_Validate(t *testing.T) { + observer1Address := sample.AccAddress() + tt := []struct { + name string + observer types.ObserverSet + wantErr require.ErrorAssertionFunc + }{ + { + name: "observer set with duplicate observer", + observer: types.ObserverSet{ObserverList: []string{observer1Address, observer1Address}}, + wantErr: func(t require.TestingT, err error, i ...interface{}) { + require.ErrorIs(t, err, types.ErrDuplicateObserver) + }, + }, + { + name: "observer set with invalid observer", + observer: types.ObserverSet{ObserverList: []string{"invalid"}}, + wantErr: func(t require.TestingT, err error, i ...interface{}) { + require.ErrorContains(t, err, "decoding bech32 failed") + }, + }, + { + name: "observer set with valid observer", + observer: types.ObserverSet{ObserverList: []string{observer1Address}}, + wantErr: require.NoError, + }, + } + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + tc.wantErr(t, tc.observer.Validate()) + }) - require.Equal(t, int(4), observerSet.Len()) - require.Equal(t, uint64(4), observerSet.LenUint()) - err := observerSet.Validate() - require.NoError(t, err) - - observerSet.ObserverList[0] = "invalid" - err = observerSet.Validate() - require.Error(t, err) + } } func TestCheckReceiveStatus(t *testing.T) { From 9208295a6e9ddd5a195bcec90c56be8099b11015 Mon Sep 17 00:00:00 2001 From: Tanmay Date: Fri, 9 Aug 2024 10:46:56 -0400 Subject: [PATCH 02/12] add validation before setting observer_set during add and update --- x/observer/keeper/observer_set.go | 11 ++++++----- x/observer/keeper/observer_set_test.go | 12 +++++------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/x/observer/keeper/observer_set.go b/x/observer/keeper/observer_set.go index e0b28142b9..2180759c9e 100644 --- a/x/observer/keeper/observer_set.go +++ b/x/observer/keeper/observer_set.go @@ -39,13 +39,14 @@ func (k Keeper) IsAddressPartOfObserverSet(ctx sdk.Context, address string) bool func (k Keeper) AddObserverToSet(ctx sdk.Context, address string) error { observerSet, found := k.GetObserverSet(ctx) - if !found { - k.SetObserverSet(ctx, types.ObserverSet{ + switch { + case !found: + observerSet = types.ObserverSet{ ObserverList: []string{address}, - }) - return nil + } + default: + observerSet.ObserverList = append(observerSet.ObserverList, address) } - observerSet.ObserverList = append(observerSet.ObserverList, address) err := observerSet.Validate() if err != nil { return err diff --git a/x/observer/keeper/observer_set_test.go b/x/observer/keeper/observer_set_test.go index 0718aea2b4..4e568b1fe3 100644 --- a/x/observer/keeper/observer_set_test.go +++ b/x/observer/keeper/observer_set_test.go @@ -52,18 +52,16 @@ func TestKeeper_AddObserverToSet(t *testing.T) { t.Run("add observer to set if set doesn't exist", func(t *testing.T) { k, ctx, _, _ := keepertest.ObserverKeeper(t) newObserver := sample.AccAddress() - k.AddObserverToSet(ctx, newObserver) + err := k.AddObserverToSet(ctx, newObserver) + require.NoError(t, err) require.True(t, k.IsAddressPartOfObserverSet(ctx, newObserver)) osNew, found := k.GetObserverSet(ctx) require.True(t, found) require.Len(t, osNew.ObserverList, 1) - // add same address again, len doesn't change - k.AddObserverToSet(ctx, newObserver) - require.True(t, k.IsAddressPartOfObserverSet(ctx, newObserver)) - osNew, found = k.GetObserverSet(ctx) - require.True(t, found) - require.Len(t, osNew.ObserverList, 1) + // cannot add same address again + err = k.AddObserverToSet(ctx, newObserver) + require.ErrorIs(t, err, types.ErrDuplicateObserver) }) } From b98e720925d1ac7cbe2b6774f4e2abd4f70f266b Mon Sep 17 00:00:00 2001 From: Tanmay Date: Fri, 9 Aug 2024 11:30:50 -0400 Subject: [PATCH 03/12] 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 { From d1ef60d8911362083eba18d4bdcd6d37b3e30659 Mon Sep 17 00:00:00 2001 From: Tanmay Date: Fri, 9 Aug 2024 13:49:19 -0400 Subject: [PATCH 04/12] add comments for tests --- x/observer/keeper/msg_server_add_observer_test.go | 5 +++++ .../keeper/msg_server_update_observer_test.go | 15 +++++++-------- x/observer/keeper/observer_set_test.go | 9 ++++++++- 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/x/observer/keeper/msg_server_add_observer_test.go b/x/observer/keeper/msg_server_add_observer_test.go index a135d0c324..183a97c8e8 100644 --- a/x/observer/keeper/msg_server_add_observer_test.go +++ b/x/observer/keeper/msg_server_add_observer_test.go @@ -53,6 +53,7 @@ func TestMsgServer_AddObserver(t *testing.T) { }) t.Run("unable to add observer if observer already exists", func(t *testing.T) { + //ARRANGE k, ctx, _, _ := keepertest.ObserverKeeperWithMocks(t, keepertest.ObserverMockOptions{ UseAuthorityMock: true, }) @@ -73,7 +74,11 @@ func TestMsgServer_AddObserver(t *testing.T) { ObserverAddress: observerAddress, } keepertest.MockCheckAuthorization(&authorityMock.Mock, &msg, nil) + + // ACT _, err := srv.AddObserver(wctx, &msg) + + // ASSERT require.ErrorIs(t, err, types.ErrDuplicateObserver) }) diff --git a/x/observer/keeper/msg_server_update_observer_test.go b/x/observer/keeper/msg_server_update_observer_test.go index 5ac3654746..66e646eb53 100644 --- a/x/observer/keeper/msg_server_update_observer_test.go +++ b/x/observer/keeper/msg_server_update_observer_test.go @@ -76,11 +76,11 @@ func TestMsgServer_UpdateObserver(t *testing.T) { t.Run( "unable to update a tombstoned observer if the new address already exists in the observer set", func(t *testing.T) { + //ARRANGE 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) @@ -104,27 +104,26 @@ func TestMsgServer_UpdateObserver(t *testing.T) { newOperatorAddress, err := types.GetAccAddressFromOperatorAddress(validatorNew.OperatorAddress) require.NoError(t, err) - count := uint64(0) - + observerList := []string{accAddressOfValidator.String(), newOperatorAddress.String()} k.SetObserverSet(ctx, types.ObserverSet{ - ObserverList: []string{accAddressOfValidator.String(), newOperatorAddress.String()}, + ObserverList: observerList, }) - count = 1 - k.SetNodeAccount(ctx, types.NodeAccount{ Operator: accAddressOfValidator.String(), }) - k.SetLastObserverCount(ctx, &types.LastObserverCount{ - Count: count, + Count: uint64(len(observerList)), }) + //ACT _, err = srv.UpdateObserver(sdk.WrapSDKContext(ctx), &types.MsgUpdateObserver{ Creator: accAddressOfValidator.String(), OldObserverAddress: accAddressOfValidator.String(), NewObserverAddress: newOperatorAddress.String(), UpdateReason: types.ObserverUpdateReason_Tombstoned, }) + + // ASSERT require.ErrorContains(t, err, types.ErrDuplicateObserver.Error()) }, ) diff --git a/x/observer/keeper/observer_set_test.go b/x/observer/keeper/observer_set_test.go index 4e568b1fe3..dd13fd72f1 100644 --- a/x/observer/keeper/observer_set_test.go +++ b/x/observer/keeper/observer_set_test.go @@ -96,17 +96,24 @@ func TestKeeper_UpdateObserverAddress(t *testing.T) { require.Equal(t, newObserverAddress, observerSet.ObserverList[len(observerSet.ObserverList)-1]) }) t.Run("unable to update observer list if the new list is not valid", func(t *testing.T) { + // ARRANGE k, ctx, _, _ := keepertest.ObserverKeeper(t) oldObserverAddress := sample.AccAddress() newObserverAddress := sample.AccAddress() observerSet := sample.ObserverSet(10) observerSet.ObserverList = append(observerSet.ObserverList, []string{oldObserverAddress, newObserverAddress}...) + // ACT 1 err := k.UpdateObserverAddress(ctx, oldObserverAddress, newObserverAddress) + + // ASSERT 1 require.ErrorIs(t, err, types.ErrObserverSetNotFound) - k.SetObserverSet(ctx, observerSet) + // ACT 2 + k.SetObserverSet(ctx, observerSet) err = k.UpdateObserverAddress(ctx, oldObserverAddress, newObserverAddress) + + // ASSERT 2 require.ErrorContains(t, err, types.ErrDuplicateObserver.Error()) }) t.Run("should error if observer address not found", func(t *testing.T) { From cbf3e1fade99d631fccc1055cc9f856cad5ae9c5 Mon Sep 17 00:00:00 2001 From: Tanmay Date: Fri, 9 Aug 2024 13:49:19 -0400 Subject: [PATCH 05/12] add comments for tests --- .../keeper/msg_server_add_observer_test.go | 5 +++ .../keeper/msg_server_update_observer_test.go | 15 +++---- x/observer/keeper/observer_set_test.go | 44 +++++++++++++++++-- 3 files changed, 52 insertions(+), 12 deletions(-) diff --git a/x/observer/keeper/msg_server_add_observer_test.go b/x/observer/keeper/msg_server_add_observer_test.go index a135d0c324..183a97c8e8 100644 --- a/x/observer/keeper/msg_server_add_observer_test.go +++ b/x/observer/keeper/msg_server_add_observer_test.go @@ -53,6 +53,7 @@ func TestMsgServer_AddObserver(t *testing.T) { }) t.Run("unable to add observer if observer already exists", func(t *testing.T) { + //ARRANGE k, ctx, _, _ := keepertest.ObserverKeeperWithMocks(t, keepertest.ObserverMockOptions{ UseAuthorityMock: true, }) @@ -73,7 +74,11 @@ func TestMsgServer_AddObserver(t *testing.T) { ObserverAddress: observerAddress, } keepertest.MockCheckAuthorization(&authorityMock.Mock, &msg, nil) + + // ACT _, err := srv.AddObserver(wctx, &msg) + + // ASSERT require.ErrorIs(t, err, types.ErrDuplicateObserver) }) diff --git a/x/observer/keeper/msg_server_update_observer_test.go b/x/observer/keeper/msg_server_update_observer_test.go index 5ac3654746..66e646eb53 100644 --- a/x/observer/keeper/msg_server_update_observer_test.go +++ b/x/observer/keeper/msg_server_update_observer_test.go @@ -76,11 +76,11 @@ func TestMsgServer_UpdateObserver(t *testing.T) { t.Run( "unable to update a tombstoned observer if the new address already exists in the observer set", func(t *testing.T) { + //ARRANGE 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) @@ -104,27 +104,26 @@ func TestMsgServer_UpdateObserver(t *testing.T) { newOperatorAddress, err := types.GetAccAddressFromOperatorAddress(validatorNew.OperatorAddress) require.NoError(t, err) - count := uint64(0) - + observerList := []string{accAddressOfValidator.String(), newOperatorAddress.String()} k.SetObserverSet(ctx, types.ObserverSet{ - ObserverList: []string{accAddressOfValidator.String(), newOperatorAddress.String()}, + ObserverList: observerList, }) - count = 1 - k.SetNodeAccount(ctx, types.NodeAccount{ Operator: accAddressOfValidator.String(), }) - k.SetLastObserverCount(ctx, &types.LastObserverCount{ - Count: count, + Count: uint64(len(observerList)), }) + //ACT _, err = srv.UpdateObserver(sdk.WrapSDKContext(ctx), &types.MsgUpdateObserver{ Creator: accAddressOfValidator.String(), OldObserverAddress: accAddressOfValidator.String(), NewObserverAddress: newOperatorAddress.String(), UpdateReason: types.ObserverUpdateReason_Tombstoned, }) + + // ASSERT require.ErrorContains(t, err, types.ErrDuplicateObserver.Error()) }, ) diff --git a/x/observer/keeper/observer_set_test.go b/x/observer/keeper/observer_set_test.go index 4e568b1fe3..4fd64cbd08 100644 --- a/x/observer/keeper/observer_set_test.go +++ b/x/observer/keeper/observer_set_test.go @@ -36,11 +36,16 @@ func TestKeeper_IsAddressPartOfObserverSet(t *testing.T) { func TestKeeper_AddObserverToSet(t *testing.T) { t.Run("add observer to set", func(t *testing.T) { + // ARRANGE k, ctx, _, _ := keepertest.ObserverKeeper(t) os := sample.ObserverSet(10) k.SetObserverSet(ctx, os) newObserver := sample.AccAddress() + + // ACT err := k.AddObserverToSet(ctx, newObserver) + + // ASSERT require.NoError(t, err) require.True(t, k.IsAddressPartOfObserverSet(ctx, newObserver)) require.False(t, k.IsAddressPartOfObserverSet(ctx, sample.AccAddress())) @@ -50,6 +55,22 @@ func TestKeeper_AddObserverToSet(t *testing.T) { }) t.Run("add observer to set if set doesn't exist", func(t *testing.T) { + // ARRANGE + k, ctx, _, _ := keepertest.ObserverKeeper(t) + newObserver := sample.AccAddress() + + // ACT + err := k.AddObserverToSet(ctx, newObserver) + // ASSERT + require.NoError(t, err) + require.True(t, k.IsAddressPartOfObserverSet(ctx, newObserver)) + osNew, found := k.GetObserverSet(ctx) + require.True(t, found) + require.Len(t, osNew.ObserverList, 1) + }) + + t.Run("cannot add observer to set the address is already part of the set", func(t *testing.T) { + // ARRANGE k, ctx, _, _ := keepertest.ObserverKeeper(t) newObserver := sample.AccAddress() err := k.AddObserverToSet(ctx, newObserver) @@ -59,8 +80,10 @@ func TestKeeper_AddObserverToSet(t *testing.T) { require.True(t, found) require.Len(t, osNew.ObserverList, 1) - // cannot add same address again + // ACT err = k.AddObserverToSet(ctx, newObserver) + + // ASSERT require.ErrorIs(t, err, types.ErrDuplicateObserver) }) } @@ -95,18 +118,31 @@ func TestKeeper_UpdateObserverAddress(t *testing.T) { require.True(t, found) require.Equal(t, newObserverAddress, observerSet.ObserverList[len(observerSet.ObserverList)-1]) }) + t.Run("unable to update observer list observe set not found", func(t *testing.T) { + // ARRANGE + k, ctx, _, _ := keepertest.ObserverKeeper(t) + oldObserverAddress := sample.AccAddress() + newObserverAddress := sample.AccAddress() + + // ACT + err := k.UpdateObserverAddress(ctx, oldObserverAddress, newObserverAddress) + + // ASSERT + require.ErrorIs(t, err, types.ErrObserverSetNotFound) + }) t.Run("unable to update observer list if the new list is not valid", func(t *testing.T) { + // ARRANGE k, ctx, _, _ := keepertest.ObserverKeeper(t) oldObserverAddress := sample.AccAddress() newObserverAddress := sample.AccAddress() observerSet := sample.ObserverSet(10) observerSet.ObserverList = append(observerSet.ObserverList, []string{oldObserverAddress, newObserverAddress}...) + k.SetObserverSet(ctx, observerSet) + // ACT err := k.UpdateObserverAddress(ctx, oldObserverAddress, newObserverAddress) - require.ErrorIs(t, err, types.ErrObserverSetNotFound) - k.SetObserverSet(ctx, observerSet) - err = k.UpdateObserverAddress(ctx, oldObserverAddress, newObserverAddress) + // ASSERT 2 require.ErrorContains(t, err, types.ErrDuplicateObserver.Error()) }) t.Run("should error if observer address not found", func(t *testing.T) { From 8f4068d0c14db514c33cad0caa47a25744b1d12a Mon Sep 17 00:00:00 2001 From: Tanmay Date: Fri, 9 Aug 2024 14:26:33 -0400 Subject: [PATCH 06/12] add comments for validate observer set --- x/observer/types/observer_set.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/x/observer/types/observer_set.go b/x/observer/types/observer_set.go index 0e68cb5ec0..45117351f4 100644 --- a/x/observer/types/observer_set.go +++ b/x/observer/types/observer_set.go @@ -15,8 +15,11 @@ func (m *ObserverSet) LenUint() uint64 { return uint64(len(m.ObserverList)) } -// Validate observer mapper contains an existing chain +// Validate observer set verifies that the observer set is valid +// - All observer addresses are valid +// - No duplicate observer addresses func (m *ObserverSet) Validate() error { + // Check for valid observer addresses for _, observerAddress := range m.ObserverList { _, err := sdk.AccAddressFromBech32(observerAddress) if err != nil { From c4b4eac818e4072bf15cb39cbbeaa2ab389ea933 Mon Sep 17 00:00:00 2001 From: Tanmay Date: Fri, 9 Aug 2024 15:13:13 -0400 Subject: [PATCH 07/12] Update x/observer/types/observer_set.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- x/observer/types/observer_set.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/observer/types/observer_set.go b/x/observer/types/observer_set.go index 45117351f4..462378e600 100644 --- a/x/observer/types/observer_set.go +++ b/x/observer/types/observer_set.go @@ -23,7 +23,7 @@ func (m *ObserverSet) Validate() error { for _, observerAddress := range m.ObserverList { _, err := sdk.AccAddressFromBech32(observerAddress) if err != nil { - return err + return errors.Wrapf(err, "invalid observer address: %s", observerAddress) } } // Check for duplicates From 29246cbced31b4d5905391d7faee7b71baa9487b Mon Sep 17 00:00:00 2001 From: Tanmay Date: Fri, 9 Aug 2024 16:28:39 -0400 Subject: [PATCH 08/12] add error messages --- x/observer/keeper/observer_set.go | 2 +- x/observer/types/observer_set.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/x/observer/keeper/observer_set.go b/x/observer/keeper/observer_set.go index 01045a4151..adb9173ccd 100644 --- a/x/observer/keeper/observer_set.go +++ b/x/observer/keeper/observer_set.go @@ -86,7 +86,7 @@ func (k Keeper) UpdateObserverAddress(ctx sdk.Context, oldObserverAddress, newOb } } if !found { - return types.ErrObserverNotFound + return errors.Wrapf(types.ErrObserverNotFound, "observer %s", oldObserverAddress) } err := observerSet.Validate() diff --git a/x/observer/types/observer_set.go b/x/observer/types/observer_set.go index 45117351f4..67a74acc07 100644 --- a/x/observer/types/observer_set.go +++ b/x/observer/types/observer_set.go @@ -27,12 +27,12 @@ func (m *ObserverSet) Validate() error { } } // Check for duplicates - observers := make(map[string]bool) + observers := make(map[string]struct{}) for _, observerAddress := range m.ObserverList { if _, ok := observers[observerAddress]; ok { return errors.Wrapf(ErrDuplicateObserver, "observer %s", observerAddress) } - observers[observerAddress] = true + observers[observerAddress] = struct{}{} } return nil } From 8bb2893368e3e3320cb3c478dc4927a601b0b728 Mon Sep 17 00:00:00 2001 From: Tanmay Date: Tue, 13 Aug 2024 11:04:37 -0400 Subject: [PATCH 09/12] refactor AddObserverToSet --- x/observer/keeper/observer_set.go | 15 ++++++++------- x/observer/keeper/observer_set_test.go | 2 +- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/x/observer/keeper/observer_set.go b/x/observer/keeper/observer_set.go index adb9173ccd..c70cc0de34 100644 --- a/x/observer/keeper/observer_set.go +++ b/x/observer/keeper/observer_set.go @@ -40,19 +40,20 @@ func (k Keeper) IsAddressPartOfObserverSet(ctx sdk.Context, address string) bool // 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 { - case !found: + if !found { observerSet = types.ObserverSet{ - ObserverList: []string{address}, + ObserverList: []string{}, } - default: - observerSet.ObserverList = append(observerSet.ObserverList, address) } - err := observerSet.Validate() - if err != nil { + + observerSet.ObserverList = append(observerSet.ObserverList, address) + + if err := observerSet.Validate(); err != nil { return err } + k.SetObserverSet(ctx, observerSet) + return nil } diff --git a/x/observer/keeper/observer_set_test.go b/x/observer/keeper/observer_set_test.go index 4fd64cbd08..0df737d92b 100644 --- a/x/observer/keeper/observer_set_test.go +++ b/x/observer/keeper/observer_set_test.go @@ -142,7 +142,7 @@ func TestKeeper_UpdateObserverAddress(t *testing.T) { // ACT err := k.UpdateObserverAddress(ctx, oldObserverAddress, newObserverAddress) - // ASSERT 2 + // ASSERT require.ErrorContains(t, err, types.ErrDuplicateObserver.Error()) }) t.Run("should error if observer address not found", func(t *testing.T) { From 00b5f59039b8489e1b1b95d1a453d56266785edc Mon Sep 17 00:00:00 2001 From: Tanmay Date: Wed, 14 Aug 2024 10:21:40 -0400 Subject: [PATCH 10/12] set observer count in AddObserverToSet function --- x/observer/keeper/msg_server_add_observer.go | 2 -- x/observer/keeper/observer_set.go | 2 +- x/observer/keeper/observer_set_test.go | 7 +++++++ 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/x/observer/keeper/msg_server_add_observer.go b/x/observer/keeper/msg_server_add_observer.go index fe4e85aedc..b4991ad160 100644 --- a/x/observer/keeper/msg_server_add_observer.go +++ b/x/observer/keeper/msg_server_add_observer.go @@ -59,8 +59,6 @@ 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/observer_set.go b/x/observer/keeper/observer_set.go index c70cc0de34..fcca45fe0a 100644 --- a/x/observer/keeper/observer_set.go +++ b/x/observer/keeper/observer_set.go @@ -47,12 +47,12 @@ func (k Keeper) AddObserverToSet(ctx sdk.Context, address string) error { } observerSet.ObserverList = append(observerSet.ObserverList, address) - if err := observerSet.Validate(); err != nil { return err } k.SetObserverSet(ctx, observerSet) + k.SetLastObserverCount(ctx, &types.LastObserverCount{Count: observerSet.LenUint()}) return nil } diff --git a/x/observer/keeper/observer_set_test.go b/x/observer/keeper/observer_set_test.go index 0df737d92b..05534d3b11 100644 --- a/x/observer/keeper/observer_set_test.go +++ b/x/observer/keeper/observer_set_test.go @@ -52,6 +52,9 @@ func TestKeeper_AddObserverToSet(t *testing.T) { osNew, found := k.GetObserverSet(ctx) require.True(t, found) require.Len(t, osNew.ObserverList, len(os.ObserverList)+1) + count, found := k.GetLastObserverCount(ctx) + require.True(t, found) + require.Equal(t, osNew.LenUint(), count.Count) }) t.Run("add observer to set if set doesn't exist", func(t *testing.T) { @@ -61,12 +64,16 @@ func TestKeeper_AddObserverToSet(t *testing.T) { // ACT err := k.AddObserverToSet(ctx, newObserver) + // ASSERT require.NoError(t, err) require.True(t, k.IsAddressPartOfObserverSet(ctx, newObserver)) osNew, found := k.GetObserverSet(ctx) require.True(t, found) require.Len(t, osNew.ObserverList, 1) + count, found := k.GetLastObserverCount(ctx) + require.True(t, found) + require.Equal(t, osNew.LenUint(), count.Count) }) t.Run("cannot add observer to set the address is already part of the set", func(t *testing.T) { From 83aed121f5b8beaaf55676da29d8b67f15f70c2f Mon Sep 17 00:00:00 2001 From: Tanmay Date: Wed, 14 Aug 2024 10:29:39 -0400 Subject: [PATCH 11/12] set observer count in AddObserverToSet function --- x/observer/keeper/msg_server_add_observer.go | 7 +++---- x/observer/keeper/observer_set.go | 10 ++++++---- x/observer/keeper/observer_set_test.go | 10 ++++++---- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/x/observer/keeper/msg_server_add_observer.go b/x/observer/keeper/msg_server_add_observer.go index b4991ad160..2c7368eade 100644 --- a/x/observer/keeper/msg_server_add_observer.go +++ b/x/observer/keeper/msg_server_add_observer.go @@ -52,16 +52,15 @@ func (k msgServer) AddObserver( return &types.MsgAddObserverResponse{}, nil } - // Add observer to the observer set - err = k.AddObserverToSet(ctx, msg.ObserverAddress) + // Add observer to the observer set and update the observer count + count, err := k.AddObserverToSet(ctx, msg.ObserverAddress) if err != nil { return &types.MsgAddObserverResponse{}, err } - observerSet, _ := k.GetObserverSet(ctx) EmitEventAddObserver( ctx, - observerSet.LenUint(), + count, msg.ObserverAddress, granteeAddress.String(), msg.ZetaclientGranteePubkey, diff --git a/x/observer/keeper/observer_set.go b/x/observer/keeper/observer_set.go index fcca45fe0a..4a6a7044f1 100644 --- a/x/observer/keeper/observer_set.go +++ b/x/observer/keeper/observer_set.go @@ -38,7 +38,8 @@ func (k Keeper) IsAddressPartOfObserverSet(ctx sdk.Context, address string) bool } // 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 { +// It also sets the observer count and returns the updated length of the observer set. +func (k Keeper) AddObserverToSet(ctx sdk.Context, address string) (uint64, error) { observerSet, found := k.GetObserverSet(ctx) if !found { observerSet = types.ObserverSet{ @@ -48,13 +49,14 @@ func (k Keeper) AddObserverToSet(ctx sdk.Context, address string) error { observerSet.ObserverList = append(observerSet.ObserverList, address) if err := observerSet.Validate(); err != nil { - return err + return 0, err } k.SetObserverSet(ctx, observerSet) - k.SetLastObserverCount(ctx, &types.LastObserverCount{Count: observerSet.LenUint()}) + newCount := observerSet.LenUint() + k.SetLastObserverCount(ctx, &types.LastObserverCount{Count: newCount}) - return nil + return newCount, nil } // RemoveObserverFromSet removes an observer from the observer set. diff --git a/x/observer/keeper/observer_set_test.go b/x/observer/keeper/observer_set_test.go index 05534d3b11..41366943f4 100644 --- a/x/observer/keeper/observer_set_test.go +++ b/x/observer/keeper/observer_set_test.go @@ -43,7 +43,7 @@ func TestKeeper_AddObserverToSet(t *testing.T) { newObserver := sample.AccAddress() // ACT - err := k.AddObserverToSet(ctx, newObserver) + countReturned, err := k.AddObserverToSet(ctx, newObserver) // ASSERT require.NoError(t, err) @@ -55,6 +55,7 @@ func TestKeeper_AddObserverToSet(t *testing.T) { count, found := k.GetLastObserverCount(ctx) require.True(t, found) require.Equal(t, osNew.LenUint(), count.Count) + require.Equal(t, osNew.LenUint(), countReturned) }) t.Run("add observer to set if set doesn't exist", func(t *testing.T) { @@ -63,7 +64,7 @@ func TestKeeper_AddObserverToSet(t *testing.T) { newObserver := sample.AccAddress() // ACT - err := k.AddObserverToSet(ctx, newObserver) + countReturned, err := k.AddObserverToSet(ctx, newObserver) // ASSERT require.NoError(t, err) @@ -74,13 +75,14 @@ func TestKeeper_AddObserverToSet(t *testing.T) { count, found := k.GetLastObserverCount(ctx) require.True(t, found) require.Equal(t, osNew.LenUint(), count.Count) + require.Equal(t, osNew.LenUint(), countReturned) }) t.Run("cannot add observer to set the address is already part of the set", func(t *testing.T) { // ARRANGE k, ctx, _, _ := keepertest.ObserverKeeper(t) newObserver := sample.AccAddress() - err := k.AddObserverToSet(ctx, newObserver) + _, err := k.AddObserverToSet(ctx, newObserver) require.NoError(t, err) require.True(t, k.IsAddressPartOfObserverSet(ctx, newObserver)) osNew, found := k.GetObserverSet(ctx) @@ -88,7 +90,7 @@ func TestKeeper_AddObserverToSet(t *testing.T) { require.Len(t, osNew.ObserverList, 1) // ACT - err = k.AddObserverToSet(ctx, newObserver) + _, err = k.AddObserverToSet(ctx, newObserver) // ASSERT require.ErrorIs(t, err, types.ErrDuplicateObserver) From 8a3a25c8c19da1164ee565a52406c505ed1fc1eb Mon Sep 17 00:00:00 2001 From: Tanmay Date: Wed, 14 Aug 2024 10:48:37 -0400 Subject: [PATCH 12/12] refactor Validate function for observer set --- x/observer/types/errors.go | 11 ++++++----- x/observer/types/observer_set.go | 11 +++++------ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/x/observer/types/errors.go b/x/observer/types/errors.go index a997267657..218521f242 100644 --- a/x/observer/types/errors.go +++ b/x/observer/types/errors.go @@ -46,9 +46,10 @@ var ( ErrObserverSetNotFound = errorsmod.Register(ModuleName, 1130, "observer set not found") ErrTssNotFound = errorsmod.Register(ModuleName, 1131, "tss not found") - ErrInboundDisabled = errorsmod.Register(ModuleName, 1132, "inbound tx processing is disabled") - ErrInvalidZetaCoinTypes = errorsmod.Register(ModuleName, 1133, "invalid zeta coin types") - ErrNotObserver = errorsmod.Register(ModuleName, 1134, "sender is not an observer") - ErrDuplicateObserver = errorsmod.Register(ModuleName, 1135, "observer already exists") - ErrObserverNotFound = errorsmod.Register(ModuleName, 1136, "observer not found") + ErrInboundDisabled = errorsmod.Register(ModuleName, 1132, "inbound tx processing is disabled") + ErrInvalidZetaCoinTypes = errorsmod.Register(ModuleName, 1133, "invalid zeta coin types") + ErrNotObserver = errorsmod.Register(ModuleName, 1134, "sender is not an observer") + ErrDuplicateObserver = errorsmod.Register(ModuleName, 1135, "observer already exists") + ErrObserverNotFound = errorsmod.Register(ModuleName, 1136, "observer not found") + ErrInvalidObserverAddress = errorsmod.Register(ModuleName, 1137, "invalid observer address") ) diff --git a/x/observer/types/observer_set.go b/x/observer/types/observer_set.go index f17cb7a3c3..db9473f187 100644 --- a/x/observer/types/observer_set.go +++ b/x/observer/types/observer_set.go @@ -19,19 +19,18 @@ func (m *ObserverSet) LenUint() uint64 { // - All observer addresses are valid // - No duplicate observer addresses func (m *ObserverSet) Validate() error { - // Check for valid observer addresses + observers := make(map[string]struct{}) for _, observerAddress := range m.ObserverList { + // Check for valid observer addresses _, err := sdk.AccAddressFromBech32(observerAddress) if err != nil { - return errors.Wrapf(err, "invalid observer address: %s", observerAddress) + return errors.Wrapf(ErrInvalidObserverAddress, "observer %s err %s", observerAddress, err.Error()) } - } - // Check for duplicates - observers := make(map[string]struct{}) - for _, observerAddress := range m.ObserverList { + // Check for duplicates if _, ok := observers[observerAddress]; ok { return errors.Wrapf(ErrDuplicateObserver, "observer %s", observerAddress) } + observers[observerAddress] = struct{}{} } return nil