Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(halo/evmstaking2) unit tests for unhappy paths #2591

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions halo/evmstaking2/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,10 @@ func (k Keeper) deliverDelegate(ctx context.Context, ev *bindings.StakingDelegat
return errors.New("validator does not exist", "validator", valAddr.String())
}

if ev.Amount == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets add a "verifyStakingDelegatefunction. Also, we can rename theev` variable, it was copy paste bug.

return errors.New("stake amount missing")
}

amountCoin, amountCoins := omniToBondCoin(ev.Amount)

k.createAccIfNone(ctx, delAddr)
Expand Down
132 changes: 120 additions & 12 deletions halo/evmstaking2/keeper/keeper_internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,18 +88,112 @@ func TestInsertAndDeleteEVMEvents(t *testing.T) {
}
}

func TestHappyPathDelivery(t *testing.T) {
func TestDeliveryWithBrokenServer(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need godocs for all tests

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't really do this for any tests in the code base. So not sure we can start a new convention simply in the middle of a PR review?

t.Parallel()

deliverInterval := int64(3)
ethStake := int64(7)
privKey := k1.GenPrivKey()

ethClientMock, err := ethclient.NewEngineMock(
ethclient.WithPortalRegister(netconf.SimnetNetwork()),
ethclient.WithMockValidatorCreation(privKey.PubKey()),
ethclient.WithMockSelfDelegation(privKey.PubKey(), ethStake),
)
require.NoError(t, err)
sServer := brokenMsgServerStub{errors.New("unconditional error")}

keeper, ctx := setupKeeper(t, deliverInterval, ethClientMock, new(authKeeperStub), new(bankKeeperStub), new(stakingKeeperStub), &sServer)

var hash common.Hash
events, err := keeper.Prepare(ctx, hash)
require.NoError(t, err)

expectDelegates := 1
expectCreates := 1
expectTotalEvents := expectDelegates + expectCreates

require.Len(t, events, expectTotalEvents)

for _, event := range events {
err := keeper.parseAndDeliver(ctx, &event)
require.True(t, strings.Contains(err.Error(), sServer.err.Error()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Incorrect syntax usage of require , update all usages.

Suggested change
require.True(t, strings.Contains(err.Error(), sServer.err.Error()))
require.Error(t, err)
require.Contains(t, err.Error(),"create validator: pubkey to cosmos")

Using require.Error and require.Contains is preferred as it provides clearer intent, better failure messages, and avoids potential panics if err is nil. It also aligns with idiomatic testing practices for improved readability and debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks!

}
}

func TestDeliveryOfInvalidEvents(t *testing.T) {
t.Parallel()

deliverInterval := int64(3)
ethStake := int64(7)
privKey := k1.GenPrivKey()

ethClientMock, err := ethclient.NewEngineMock(
ethclient.WithPortalRegister(netconf.SimnetNetwork()),
ethclient.WithMockValidatorCreation(privKey.PubKey()),
ethclient.WithMockSelfDelegation(privKey.PubKey(), ethStake),
)
require.NoError(t, err)

keeper, ctx := setupKeeper(t, deliverInterval, ethClientMock, new(authKeeperStub), new(bankKeeperStub), new(stakingKeeperStub), new(msgServerStub))

var hash common.Hash
events, err := keeper.Prepare(ctx, hash)
require.NoError(t, err)

expectDelegates := 1
expectCreates := 1
expectTotalEvents := expectDelegates + expectCreates

require.Len(t, events, expectTotalEvents)

// Break the address for both events and make sure parsing fails
for _, event := range events {
event.Address = []byte{}
err := keeper.parseAndDeliver(ctx, &event)
require.True(t, strings.Contains(err.Error(), "invalid address length"))
}

// Break the topics for both events and make sure parsing fails
for _, event := range events {
event.Topics = [][]byte{}
err := keeper.parseAndDeliver(ctx, &event)
require.True(t, strings.Contains(err.Error(), "empty topics"))
}

createValEvent := events[0]
// Break the data for the create validator event
createValEvent.Data = []byte{}
err = keeper.parseAndDeliver(ctx, &createValEvent)
require.True(t, strings.Contains(err.Error(), "create validator: pubkey to cosmos"))

// Deliver the event so that we can test delegation
err = keeper.parseAndDeliver(ctx, &events[0])
require.NoError(t, err)

// Can't add same validator twice (this relies on sKeeper stub working correctly)
err = keeper.parseAndDeliver(ctx, &events[0])
require.True(t, strings.Contains(err.Error(), "create validator: validator already exists"))

delegateEvent := events[1]
// Break the data for the delegate event
delegateEvent.Data = []byte{}
err = keeper.parseAndDeliver(ctx, &delegateEvent)
require.True(t, strings.Contains(err.Error(), "stake amount missing"))
}

func TestHappyPathDelivery(t *testing.T) {
t.Parallel()

deliverInterval := int64(3)
ethStake := int64(7)

privKey := k1.GenPrivKey()

ethClientMock, err := ethclient.NewEngineMock(
ethclient.WithPortalRegister(netconf.SimnetNetwork()),
ethclient.WithMockValidatorCreation(privKey.PubKey()),
ethclient.WithMockSelfDelegation(privKey.PubKey(), ethStake),
)
require.NoError(t, err)

Expand Down Expand Up @@ -202,19 +296,21 @@ func setupKeeper(
}

type stakingKeeperStub struct {
// calls is the number of calls to GetValidator
calls uint32
validators map[string]bool
}

// GetValidator returns no errors on the first call, because it is called on delegation
// event delivery for the first time and the validator should be available.
// Second time it is called on a validator creation event and it should return an error
// on the pubkey of the new validator.
func (m *stakingKeeperStub) GetValidator(context.Context, sdk.ValAddress) (stypes.Validator, error) {
m.calls++
if m.calls == 1 {
// GetValidator memorizes all addresses and returns an error for calls it never seen before.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// GetValidator memorizes all addresses and returns an error for calls it never seen before.
// GetValidator tracks accessed addresses, returning an error for unseen addresses.

func (m *stakingKeeperStub) GetValidator(ctx context.Context, addr sdk.ValAddress) (stypes.Validator, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func (m *stakingKeeperStub) GetValidator(ctx context.Context, addr sdk.ValAddress) (stypes.Validator, error) {
func (m *stakingKeeperStub) GetValidator(_ context.Context, addr sdk.ValAddress) (stypes.Validator, error) {

if m.validators == nil {
m.validators = make(map[string]bool)
}

hexAddr := string(addr)

if _, found := m.validators[hexAddr]; found {
return stypes.Validator{}, nil
}
m.validators[hexAddr] = true

return stypes.Validator{}, errors.New("validator does not exist")
Comment on lines +313 to 315
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit confused by this, we set the validator to true but we return an error?

}
Expand Down Expand Up @@ -246,12 +342,24 @@ type msgServerStub struct {
delegateMsgBuffer []*stypes.MsgDelegate
}

func (s *msgServerStub) CreateValidator(_ context.Context, msg *stypes.MsgCreateValidator) (*stypes.MsgCreateValidatorResponse, error) {
func (s *msgServerStub) CreateValidator(ctx context.Context, msg *stypes.MsgCreateValidator) (*stypes.MsgCreateValidatorResponse, error) {
s.createValidatorMsgBuffer = append(s.createValidatorMsgBuffer, msg)
return new(stypes.MsgCreateValidatorResponse), nil
}

func (s *msgServerStub) Delegate(_ context.Context, msg *stypes.MsgDelegate) (*stypes.MsgDelegateResponse, error) {
func (s *msgServerStub) Delegate(ctx context.Context, msg *stypes.MsgDelegate) (*stypes.MsgDelegateResponse, error) {
s.delegateMsgBuffer = append(s.delegateMsgBuffer, msg)
return new(stypes.MsgDelegateResponse), nil
}

type brokenMsgServerStub struct {
err error
}

func (s *brokenMsgServerStub) CreateValidator(ctx context.Context, msg *stypes.MsgCreateValidator) (*stypes.MsgCreateValidatorResponse, error) {
return nil, s.err
}

func (s *brokenMsgServerStub) Delegate(ctx context.Context, msg *stypes.MsgDelegate) (*stypes.MsgDelegateResponse, error) {
return nil, s.err
}
Loading