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.

Sign up for GitHub

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

Jump to bottom

imp(core): allow huckleberry events with a prefix #5541

Merged
merged 21 commits into from
Jan 11, 2024
Merged

imp(core): allow huckleberry events with a prefix #5541

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
c53f913
feat: initial impl
srdtrk Jan 5, 2024
688a9d9
imp: moved event helpers to types and added tests
srdtrk Jan 5, 2024
70e7cad
imp(testing): added mock events to mock module
srdtrk Jan 5, 2024
bdbd9f6
test: added msg_server tests for application events
srdtrk Jan 5, 2024
88f430b
Merge branch 'main' into serdar/issue#5284-error-event-suffix
srdtrk Jan 8, 2024
f4caa14
imp: converted suffix to prefix
srdtrk Jan 8, 2024
da528e4
docs: updated inline comments
srdtrk Jan 8, 2024
b5f6ee0
Merge branch 'main' into serdar/issue#5284-error-event-suffix
srdtrk Jan 8, 2024
6a2bef0
Merge branch 'main' into serdar/issue#5284-error-event-suffix
srdtrk Jan 8, 2024
07fca7e
Merge branch 'main' into serdar/issue#5284-error-event-suffix
srdtrk Jan 8, 2024
78a1c60
Merge branch 'main' into serdar/issue#5284-error-event-suffix
srdtrk Jan 9, 2024
1c7862f
imp: review item
srdtrk Jan 9, 2024
43a03f3
Merge branch 'main' into serdar/issue#5284-error-event-suffix
srdtrk Jan 9, 2024
986fc82
Merge branch 'main' into serdar/issue#5284-error-event-suffix
srdtrk Jan 9, 2024
16a3312
Merge branch 'main' into serdar/issue#5284-error-event-suffix
srdtrk Jan 9, 2024
52b1e0e
Merge branch 'main' into serdar/issue#5284-error-event-suffix
srdtrk Jan 10, 2024
a30078b
Merge branch 'main' into serdar/issue#5284-error-event-suffix
srdtrk Jan 10, 2024
fae32e4
test: review items
srdtrk Jan 10, 2024
6106643
Merge branch 'main' into serdar/issue#5284-error-event-suffix
srdtrk Jan 10, 2024
4b58d7f
Merge branch 'main' into serdar/issue#5284-error-event-suffix
srdtrk Jan 11, 2024
4f9acf3
imp: review items
srdtrk Jan 11, 2024
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
3 changes: 3 additions & 0 deletions modules/core/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,9 @@ func (k Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPacke
if ack == nil || ack.Success() {
// write application state changes for asynchronous and successful acknowledgements
writeFn()
} else {
// Modify events in cached context to reflect unsuccessful acknowledgement
ctx.EventManager().EmitEvents(coretypes.ConvertToErrorEvents(cacheCtx.EventManager().Events()))
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}

// Set packet acknowledgement only if the acknowledgement is not nil.
Expand Down
56 changes: 53 additions & 3 deletions modules/core/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
"github.com/cosmos/ibc-go/v8/modules/core/exported"
"github.com/cosmos/ibc-go/v8/modules/core/keeper"
"github.com/cosmos/ibc-go/v8/modules/core/types"
ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint"
ibctesting "github.com/cosmos/ibc-go/v8/testing"
ibcmock "github.com/cosmos/ibc-go/v8/testing/mock"
Expand All @@ -39,6 +40,7 @@ func (suite *KeeperTestSuite) TestHandleRecvPacket() {
packet channeltypes.Packet
path *ibctesting.Path
async bool // indicate no ack written
replay bool // indicate replay (no-op)
)

testCases := []struct {
Expand Down Expand Up @@ -127,6 +129,7 @@ func (suite *KeeperTestSuite) TestHandleRecvPacket() {
// mock will panic if application callback is called twice on the same packet
path.SetChannelOrdered()
suite.coordinator.Setup(path)
replay = true

sequence, err := path.EndpointA.SendPacket(timeoutHeight, 0, ibctesting.MockPacketData)
suite.Require().NoError(err)
Expand All @@ -138,6 +141,7 @@ func (suite *KeeperTestSuite) TestHandleRecvPacket() {
{"successful no-op: UNORDERED - packet already received (replay)", func() {
// mock will panic if application callback is called twice on the same packet
suite.coordinator.Setup(path)
replay = true

sequence, err := path.EndpointA.SendPacket(timeoutHeight, 0, ibctesting.MockPacketData)
suite.Require().NoError(err)
Expand All @@ -154,6 +158,7 @@ func (suite *KeeperTestSuite) TestHandleRecvPacket() {
suite.Run(tc.name, func() {
suite.SetupTest() // reset
async = false // reset
replay = false // reset
path = ibctesting.NewPath(suite.chainA, suite.chainB)

tc.malleate()
Expand All @@ -171,7 +176,10 @@ func (suite *KeeperTestSuite) TestHandleRecvPacket() {

msg := channeltypes.NewMsgRecvPacket(packet, proof, proofHeight, suite.chainB.SenderAccount.GetAddress().String())

_, err := keeper.Keeper.RecvPacket(*suite.chainB.App.GetIBCKeeper(), suite.chainB.GetContext(), msg)
ctx := suite.chainB.GetContext()
_, err := keeper.Keeper.RecvPacket(*suite.chainB.App.GetIBCKeeper(), ctx, msg)

events := ctx.EventManager().Events()

if tc.expPass {
suite.Require().NoError(err)
Expand All @@ -184,8 +192,20 @@ func (suite *KeeperTestSuite) TestHandleRecvPacket() {
_, exists := suite.chainB.GetSimApp().ScopedIBCMockKeeper.GetCapability(suite.chainB.GetContext(), ibcmock.GetMockRecvCanaryCapabilityName(packet))
if tc.expRevert {
suite.Require().False(exists, "capability exists in store even after callback reverted")

// context events should contain error events
suite.Require().Contains(events, types.ConvertToErrorEvents(sdk.Events{ibcmock.NewMockRecvPacketEvent()})[0])
} else {
suite.Require().True(exists, "callback state not persisted when revert is false")

if replay {
// context should not contain application events
suite.Require().NotContains(events, ibcmock.NewMockRecvPacketEvent())
suite.Require().NotContains(events, types.ConvertToErrorEvents(sdk.Events{ibcmock.NewMockRecvPacketEvent()})[0])
} else {
// context events should contain application events
suite.Require().Contains(events, ibcmock.NewMockRecvPacketEvent())
}
}

// verify if ack was written
Expand Down Expand Up @@ -291,6 +311,7 @@ func (suite *KeeperTestSuite) TestHandleAcknowledgePacket() {
var (
packet channeltypes.Packet
path *ibctesting.Path
replay bool // indicate replay (no-op)
)

testCases := []struct {
Expand Down Expand Up @@ -361,6 +382,7 @@ func (suite *KeeperTestSuite) TestHandleAcknowledgePacket() {
}, false},
{"successful no-op: ORDERED - packet already acknowledged (replay)", func() {
suite.coordinator.Setup(path)
replay = true

sequence, err := path.EndpointA.SendPacket(timeoutHeight, 0, ibctesting.MockPacketData)
suite.Require().NoError(err)
Expand All @@ -374,6 +396,7 @@ func (suite *KeeperTestSuite) TestHandleAcknowledgePacket() {
}, true},
{"successful no-op: UNORDERED - packet already acknowledged (replay)", func() {
suite.coordinator.Setup(path)
replay = true

sequence, err := path.EndpointA.SendPacket(timeoutHeight, 0, ibctesting.MockPacketData)
suite.Require().NoError(err)
Expand All @@ -392,6 +415,7 @@ func (suite *KeeperTestSuite) TestHandleAcknowledgePacket() {

suite.Run(tc.name, func() {
suite.SetupTest() // reset
replay = false // reset
path = ibctesting.NewPath(suite.chainA, suite.chainB)

tc.malleate()
Expand All @@ -407,7 +431,10 @@ func (suite *KeeperTestSuite) TestHandleAcknowledgePacket() {

msg := channeltypes.NewMsgAcknowledgement(packet, ibcmock.MockAcknowledgement.Acknowledgement(), proof, proofHeight, suite.chainA.SenderAccount.GetAddress().String())

_, err := keeper.Keeper.Acknowledgement(*suite.chainA.App.GetIBCKeeper(), suite.chainA.GetContext(), msg)
ctx := suite.chainA.GetContext()
_, err := keeper.Keeper.Acknowledgement(*suite.chainA.App.GetIBCKeeper(), ctx, msg)

events := ctx.EventManager().Events()

if tc.expPass {
suite.Require().NoError(err)
Expand All @@ -419,6 +446,14 @@ func (suite *KeeperTestSuite) TestHandleAcknowledgePacket() {
// replay should not error as it is treated as a no-op
_, err := keeper.Keeper.Acknowledgement(*suite.chainA.App.GetIBCKeeper(), suite.chainA.GetContext(), msg)
suite.Require().NoError(err)

Copy link
Contributor

Choose a reason for hiding this comment

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

you could consider refactoring these test cases to use a function that asserts the expected test outcomes as in this pr rather than checking against these if conditions, might be easier to read. but can also be done in a followup

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we could do it. I think this better be a followup because these "if conditions" are already used commonly in this file for async. So, such a refactor would change unrelated testing code too

Copy link
Member Author

Choose a reason for hiding this comment

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

I've moved some parameters to the test case like @chatton suggested. However, we could still open an issue for your suggestion since I think it could be a nice improvement

if replay {
Copy link
Contributor

Choose a reason for hiding this comment

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

small nit, to be more consistent maybe we could make this a tc.replay, we seem to be mixing things like tc.expPass and standalone bools like replay.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

// context should not contain application events
suite.Require().NotContains(events, ibcmock.NewMockAckPacketEvent())
} else {
// context events should contain application events
suite.Require().Contains(events, ibcmock.NewMockAckPacketEvent())
}
} else {
suite.Require().Error(err)
}
Expand All @@ -436,6 +471,7 @@ func (suite *KeeperTestSuite) TestHandleTimeoutPacket() {
packet channeltypes.Packet
packetKey []byte
path *ibctesting.Path
noop bool // indicate no-op
srdtrk marked this conversation as resolved.
Show resolved Hide resolved
)

testCases := []struct {
Expand Down Expand Up @@ -528,6 +564,8 @@ func (suite *KeeperTestSuite) TestHandleTimeoutPacket() {
}, false},
{"successful no-op: UNORDERED - packet not sent", func() {
suite.coordinator.Setup(path)
noop = true

packet = channeltypes.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, clienttypes.NewHeight(0, 1), 0)
packetKey = host.PacketReceiptKey(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
}, true},
Expand All @@ -538,6 +576,7 @@ func (suite *KeeperTestSuite) TestHandleTimeoutPacket() {

suite.Run(tc.name, func() {
suite.SetupTest() // reset
noop = false // reset
path = ibctesting.NewPath(suite.chainA, suite.chainB)

tc.malleate()
Expand All @@ -552,7 +591,10 @@ func (suite *KeeperTestSuite) TestHandleTimeoutPacket() {

msg := channeltypes.NewMsgTimeout(packet, 1, proof, proofHeight, suite.chainA.SenderAccount.GetAddress().String())

_, err := keeper.Keeper.Timeout(*suite.chainA.App.GetIBCKeeper(), suite.chainA.GetContext(), msg)
ctx := suite.chainA.GetContext()
_, err := keeper.Keeper.Timeout(*suite.chainA.App.GetIBCKeeper(), ctx, msg)

events := ctx.EventManager().Events()

if tc.expPass {
suite.Require().NoError(err)
Expand All @@ -565,6 +607,14 @@ func (suite *KeeperTestSuite) TestHandleTimeoutPacket() {
has := suite.chainA.App.GetIBCKeeper().ChannelKeeper.HasPacketCommitment(suite.chainA.GetContext(), packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())
suite.Require().False(has)

if noop {
// context should not contain application events
suite.Require().NotContains(events, ibcmock.NewMockTimeoutPacketEvent())
} else {
// context should contain application events
suite.Require().Contains(events, ibcmock.NewMockTimeoutPacketEvent())
}

} else {
suite.Require().Error(err)
}
Expand Down
29 changes: 29 additions & 0 deletions modules/core/types/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package types

import (
sdk "github.com/cosmos/cosmos-sdk/types"
)

const ErrorAttributeKeyPrefix = "ibccallbackerror-"

// ConvertToErrorEvents converts all events to error events by appending the
// error attribute prefix to each event's attribute key.
func ConvertToErrorEvents(events sdk.Events) sdk.Events {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
if events == nil {
return nil
}

newEvents := make(sdk.Events, len(events))
for i, event := range events {
newAttributes := make([]sdk.Attribute, len(event.Attributes))
for j, attribute := range event.Attributes {
newAttributes[j] = sdk.NewAttribute(ErrorAttributeKeyPrefix+attribute.Key, attribute.Value)
}

// no need to append the error attribute prefix to the event type because
// the event type is not associated to a value that can be misinterpreted
newEvents[i] = sdk.NewEvent(event.Type, newAttributes...)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we elaborate on this reasoning? I feel it is safer to prefix the type as well. Is there a direct benefit to not prefixing the type?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it might be easier for users to index events by module that emitted the event. In that it wouldn't break their pre-existing clients too much.

The reasoning isn't solid but what I meant was that event types are only used for indexing attributes by module. Event types aren't the key to some value, they are only used to retrieve the attributes.

But I'm open to prefixing this too

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm yea. My main concern is accidental misuse, that is using the event type to indicate an action occurred without caring about particular values. But I think you make a fair point that event types aren't the key to some value. I'm not too concerned about breaking pre-existing clients as pre-existing clients currently cannot access error events. If the primary use case here is simply for debugging, I'm not sure I see much benefit in not error prefixing the type defensively, but I don't feel strongly, just feel slightly uncomfortable allowing any surface area for misinterpretation of events

Copy link
Member Author

@srdtrk srdtrk Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah fair enough. Added the prefix here as well

}

return newEvents
}
105 changes: 105 additions & 0 deletions modules/core/types/events_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
package types_test

import (
"testing"

"github.com/stretchr/testify/require"

sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/v8/modules/core/types"
)

func TestConvertToErrorEvents(t *testing.T) {
var (
events sdk.Events
expEvents sdk.Events
)

tc := []struct {
name string
malleate func()
}{
{
"success: nil events",
func() {
events = nil
expEvents = nil
},
},
{
"success: empty events",
func() {
events = sdk.Events{}
expEvents = sdk.Events{}
},
},
{
"success: event with no attributes",
func() {
events = sdk.Events{
sdk.NewEvent("testevent"),
}
expEvents = sdk.Events{
sdk.NewEvent("testevent"),
}
},
},
{
"success: event with attributes",
func() {
events = sdk.Events{
sdk.NewEvent("testevent",
sdk.NewAttribute("key1", "value1"),
sdk.NewAttribute("key2", "value2"),
),
}
expEvents = sdk.Events{
sdk.NewEvent("testevent",
sdk.NewAttribute(types.ErrorAttributeKeyPrefix+"key1", "value1"),
sdk.NewAttribute(types.ErrorAttributeKeyPrefix+"key2", "value2"),
),
}
},
},
{
"success: multiple events with attributes",
func() {
events = sdk.Events{
sdk.NewEvent("testevent1",
sdk.NewAttribute("key1", "value1"),
sdk.NewAttribute("key2", "value2"),
),
sdk.NewEvent("testevent2",
sdk.NewAttribute("key3", "value3"),
sdk.NewAttribute("key4", "value4"),
),
}
expEvents = sdk.Events{
sdk.NewEvent("testevent1",
sdk.NewAttribute(types.ErrorAttributeKeyPrefix+"key1", "value1"),
sdk.NewAttribute(types.ErrorAttributeKeyPrefix+"key2", "value2"),
),
sdk.NewEvent("testevent2",
sdk.NewAttribute(types.ErrorAttributeKeyPrefix+"key3", "value3"),
sdk.NewAttribute(types.ErrorAttributeKeyPrefix+"key4", "value4"),
),
}
},
},
}

for _, tc := range tc {
t.Run(tc.name, func(t *testing.T) {
// initial events and expected events are reset so that the test fails if
// the malleate function does not set them
events = nil
expEvents = sdk.Events{}

tc.malleate()

newEvents := types.ConvertToErrorEvents(events)
require.Equal(t, expEvents, newEvents)
})
}
}
39 changes: 39 additions & 0 deletions testing/mock/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package mock

import sdk "github.com/cosmos/cosmos-sdk/types"

const (
MockEventTypeRecvPacket = "mock-recv-packet"
MockEventTypeAckPacket = "mock-ack-packet"
MockEventTypeTimeoutPacket = "mock-timeout"

MockAttributeKey1 = "mock-attribute-key-1"
MockAttributeKey2 = "mock-attribute-key-2"

MockAttributeValue1 = "mock-attribute-value-1"
MockAttributeValue2 = "mock-attribute-value-2"
)

// NewMockRecvPacketEvent returns a mock receive packet event
func NewMockRecvPacketEvent() sdk.Event {
return newMockEvent(MockEventTypeRecvPacket)
}

// NewMockAckPacketEvent returns a mock acknowledgement packet event
func NewMockAckPacketEvent() sdk.Event {
return newMockEvent(MockEventTypeAckPacket)
}

// NewMockTimeoutPacketEvent emits a mock timeout packet event
func NewMockTimeoutPacketEvent() sdk.Event {
return newMockEvent(MockEventTypeTimeoutPacket)
}

// emitMockEvent returns a mock event with the given event type
func newMockEvent(eventType string) sdk.Event {
return sdk.NewEvent(
eventType,
sdk.NewAttribute(MockAttributeKey1, MockAttributeValue1),
sdk.NewAttribute(MockAttributeKey2, MockAttributeValue2),
)
}
6 changes: 6 additions & 0 deletions testing/mock/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ func (im IBCModule) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, re
panic(err)
}

ctx.EventManager().EmitEvent(NewMockRecvPacketEvent())

if bytes.Equal(MockPacketData, packet.GetData()) {
return MockAcknowledgement
} else if bytes.Equal(MockAsyncPacketData, packet.GetData()) {
Expand All @@ -158,6 +160,8 @@ func (im IBCModule) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes
panic(err)
}

ctx.EventManager().EmitEvent(NewMockAckPacketEvent())

return nil
}

Expand All @@ -174,6 +178,8 @@ func (im IBCModule) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet,
panic(err)
}

ctx.EventManager().EmitEvent(NewMockTimeoutPacketEvent())

return nil
}

Expand Down
Loading