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

feat: ics 29 packet callbacks #357

Merged
merged 43 commits into from
Dec 21, 2021
Merged

feat: ics 29 packet callbacks #357

Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
7b51ebd
do handshake logic, create test file
AdityaSripal Aug 3, 2021
150211d
do cap logic and fix build
AdityaSripal Aug 5, 2021
412f3b6
Merge remote-tracking branch 'upstream/aditya/handshake' into feat/ic…
charleenfei Aug 27, 2021
734ead7
initial scaffolding
charleenfei Aug 12, 2021
1e894fe
added ics4 callbacks
charleenfei Aug 27, 2021
f735e57
Merge branch 'ics29-fee-middleware' into feat/ics-29-packetcallbacks
charleenfei Oct 6, 2021
435a2fa
Merge branch 'feat/ics-29-packetcallbacks' of github.com:cosmos/ibc-g…
charleenfei Nov 10, 2021
8d0c194
Merge branch 'ics29-fee-middleware' into feat/ics-29-packetcallbacks
charleenfei Nov 10, 2021
bfa8795
wip
charleenfei Nov 16, 2021
09985ce
update fee proto to include refund acc in storage of IdentifiedPacketFee
charleenfei Nov 16, 2021
ff7db8a
update yaml
charleenfei Nov 16, 2021
8c6d8ec
Merge branch 'update/fee_proto' into feat/ics-29-packetcallbacks
charleenfei Nov 16, 2021
9d9bcae
finish logic in packet callbacks
charleenfei Nov 16, 2021
d8e4429
wip
charleenfei Nov 17, 2021
e514f1a
store refund address in IdentifiedPacketFee
AdityaSripal Nov 17, 2021
f561cc0
Merge branch 'store_refund_address' into feat/ics-29-packetcallbacks
charleenfei Nov 18, 2021
97cc863
wip
charleenfei Nov 24, 2021
0a0022d
Merge branch 'ics29-fee-middleware' into feat/ics-29-packetcallbacks
charleenfei Nov 24, 2021
8e83ba2
proto file
charleenfei Nov 26, 2021
40951eb
incentivized ack proto
charleenfei Nov 26, 2021
e8b2ddc
Merge branch 'feat/incentivised_ack_proto' into feat/ics-29-packetcal…
charleenfei Nov 26, 2021
04763b6
wip
charleenfei Nov 30, 2021
e574499
Merge branch 'ics29-fee-middleware' of github.com:cosmos/ibc-go into …
charleenfei Nov 30, 2021
e7297a8
Merge branch 'ics29-fee-middleware' into feat/ics-29-packetcallbacks
charleenfei Nov 30, 2021
db93ac9
packet callbacks w ack
charleenfei Nov 30, 2021
2870993
testing wip
charleenfei Dec 1, 2021
64f77b9
Merge branch 'ics29-fee-middleware' of github.com:cosmos/ibc-go into …
charleenfei Dec 1, 2021
721a6c7
Merge branch 'ics29-fee-middleware' into feat/ics-29-packetcallbacks
charleenfei Dec 1, 2021
9f00d29
finished testing
charleenfei Dec 6, 2021
0844485
app changes
charleenfei Dec 7, 2021
e374cb2
initial pr review comments
charleenfei Dec 7, 2021
232e906
initial comments
charleenfei Dec 8, 2021
a8c69db
add tests
charleenfei Dec 8, 2021
6c85913
semantics
charleenfei Dec 8, 2021
9d84834
logic changes re: comments
charleenfei Dec 10, 2021
141d281
add cacheCtx
charleenfei Dec 10, 2021
76b7de9
tests
charleenfei Dec 10, 2021
7ced819
comments
charleenfei Dec 10, 2021
dc33d66
diff channel, test
charleenfei Dec 20, 2021
903b9d5
fix escrow bug
charleenfei Dec 20, 2021
8fc3aac
fix test
charleenfei Dec 20, 2021
2a061e4
sdk.AccAddress casting
charleenfei Dec 21, 2021
aae78ce
comments
charleenfei Dec 21, 2021
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
48 changes: 40 additions & 8 deletions modules/apps/29-fee/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ import (
"github.com/cosmos/ibc-go/modules/apps/29-fee/types"
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types"
porttypes "github.com/cosmos/ibc-go/modules/core/05-port/types"
ibcexported "github.com/cosmos/ibc-go/modules/core/exported"
"github.com/cosmos/ibc-go/modules/core/exported"
)

// IBCModule implements the ICS26 callbacks for the fee middleware given the fee keeper and the underlying application.
// IncentivizedIBCModule implements the ICS26 callbacks for the fee middleware given the fee keeper and the underlying application.
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
type IBCModule struct {
keeper keeper.Keeper
app porttypes.IBCModule
Expand Down Expand Up @@ -175,9 +175,17 @@ func (im IBCModule) OnRecvPacket(
ctx sdk.Context,
packet channeltypes.Packet,
relayer sdk.AccAddress,
) ibcexported.Acknowledgement {
// TODO: Implement fee specific logic if fee is enabled for the given channel
return im.app.OnRecvPacket(ctx, packet, relayer)
) exported.Acknowledgement {
ack := im.app.OnRecvPacket(ctx, packet, relayer)
sourceRelayer, found := im.keeper.GetCounterpartyAddress(ctx, relayer.String())
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
if !found {
sourceRelayer = ""
}

return types.IncentivizedAcknowledgement{
Result: ack.Acknowledgement(),
ForwardRelayerAddress: sourceRelayer,
Copy link
Contributor

Choose a reason for hiding this comment

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

should we keep the same naming for both rather than source/forward?

}
}

// OnAcknowledgementPacket implements the IBCModule interface
Expand All @@ -187,8 +195,23 @@ func (im IBCModule) OnAcknowledgementPacket(
acknowledgement []byte,
relayer sdk.AccAddress,
) error {
// TODO: Implement fee specific logic if fee is enabled for the given channel
return im.app.OnAcknowledgementPacket(ctx, packet, acknowledgement, relayer)
ack := &types.IncentivizedAcknowledgement{}
if im.keeper.IsFeeEnabled(ctx, packet.SourcePort, packet.SourceChannel) {
if err := types.ModuleCdc.UnmarshalJSON(acknowledgement, ack); err != nil {
return sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal ICS-29 incentivized packet acknowledgement: %v", err)
}

packetId := channeltypes.NewPacketId(packet.SourceChannel, packet.SourcePort, packet.Sequence)
identifiedPacketFee, found := im.keeper.GetFeeInEscrow(ctx, packetId)
if !found {
return sdkerrors.Wrapf(types.ErrFeeNotFound, "fee not found for packet id %s", packetId)
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
}

if err := im.keeper.DistributeFee(ctx, sdk.AccAddress(identifiedPacketFee.RefundAddress), sdk.AccAddress(ack.ForwardRelayerAddress), relayer, packetId); err != nil {
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
return err
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
}
}
return im.app.OnAcknowledgementPacket(ctx, packet, ack.Result, relayer)
}

// OnTimeoutPacket implements the IBCModule interface
Expand All @@ -197,6 +220,15 @@ func (im IBCModule) OnTimeoutPacket(
packet channeltypes.Packet,
relayer sdk.AccAddress,
) error {
// TODO: Implement fee specific logic if fee is enabled for the given channel
packetId := channeltypes.NewPacketId(packet.SourceChannel, packet.SourcePort, packet.Sequence)
identifiedPacketFee, exists := im.keeper.GetFeeInEscrow(ctx, packetId)
if !exists {
return sdkerrors.Wrapf(types.ErrFeeNotFound, "fee not found for packet id %s", packetId)
}
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
if im.keeper.IsFeeEnabled(ctx, packet.SourcePort, packet.SourceChannel) {
if err := im.keeper.DistributeFeeTimeout(ctx, sdk.AccAddress(identifiedPacketFee.RefundAddress), relayer, channeltypes.NewPacketId(packet.SourceChannel, packet.SourcePort, packet.Sequence)); err != nil {
return err
}
}
return im.app.OnTimeoutPacket(ctx, packet, relayer)
}
300 changes: 300 additions & 0 deletions modules/apps/29-fee/ibc_module_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ import (

"github.com/cosmos/ibc-go/modules/apps/29-fee/types"
transfertypes "github.com/cosmos/ibc-go/modules/apps/transfer/types"
clienttypes "github.com/cosmos/ibc-go/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/modules/core/24-host"
ibctesting "github.com/cosmos/ibc-go/testing"
"github.com/cosmos/ibc-go/testing/simapp"
)

var (
Expand Down Expand Up @@ -468,3 +470,301 @@ func (suite *FeeTestSuite) TestOnChanCloseConfirm() {
})
}
}

func (suite *FeeTestSuite) TestOnRecvPacket() {
testCases := []struct {
name string
malleate func(suite *FeeTestSuite)
sourceRelayer bool
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}{
{
"success",
func(suite *FeeTestSuite) {
suite.chainA.GetSimApp().IBCFeeKeeper.SetCounterpartyAddress(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String())
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
},
true,
},
{
"source relayer is empty string",
func(suite *FeeTestSuite) {},
false,
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest()
suite.coordinator.SetupConnections(suite.path)

// open incentivized channel
suite.path.EndpointA.ChanOpenInit()
suite.path.EndpointB.ChanOpenTry()
suite.path.EndpointA.ChanOpenAck()
suite.path.EndpointB.ChanOpenConfirm()
charleenfei marked this conversation as resolved.
Show resolved Hide resolved

// set up coins
coin := ibctesting.TestCoin
fungibleTokenPacket := transfertypes.NewFungibleTokenPacketData(
coin.Denom,
sdk.NewInt(100).Uint64(),
suite.chainA.SenderAccount.GetAddress().String(),
suite.chainB.SenderAccount.GetAddress().String(),
)
packet := channeltypes.NewPacket(
fungibleTokenPacket.GetBytes(),
suite.chainA.SenderAccount.GetSequence(),
suite.path.EndpointA.ChannelConfig.PortID,
suite.path.EndpointA.ChannelID,
suite.path.EndpointB.ChannelConfig.PortID,
suite.path.EndpointB.ChannelID,
clienttypes.NewHeight(0, 100),
0,
)

// set up & escrow packet fees
refundAcc := suite.chainA.SenderAccount.GetAddress()
packetId := channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, 1)
identifiedFee := types.NewIdentifiedPacketFee(
packetId,
types.Fee{
ReceiveFee: validCoins,
AckFee: validCoins2,
TimeoutFee: validCoins3,
},
refundAcc.String(),
[]string{},
)
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), identifiedFee)
suite.Require().NoError(err)

// set up module and callbacks
module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.TransferPort)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module)
suite.Require().True(ok)

// malleate test case
tc.malleate(suite)

result := cbs.OnRecvPacket(suite.chainA.GetContext(), packet, refundAcc)

if tc.sourceRelayer {
suite.Require().NoError(err, "unexpected error for case: %s", tc.name)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
ack := types.IncentivizedAcknowledgement{
Result: channeltypes.NewResultAcknowledgement([]byte{1}).Acknowledgement(),
ForwardRelayerAddress: suite.chainB.SenderAccount.GetAddress().String(),
}
suite.Require().Equal(result, ack)
} else {
suite.Require().NoError(err, "unexpected error for case: %s", tc.name)
ack := types.IncentivizedAcknowledgement{
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
Result: channeltypes.NewResultAcknowledgement([]byte{1}).Acknowledgement(),
ForwardRelayerAddress: "",
}
suite.Require().Equal(result, ack)
}
})
}
}

func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
testCases := []struct {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
name string
malleate func(suite *FeeTestSuite)
expPass bool
}{
{
"success",
func(suite *FeeTestSuite) {
packetId := channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, suite.chainA.SenderAccount.GetSequence())
identifiedFee := types.NewIdentifiedPacketFee(
packetId,
types.Fee{
ReceiveFee: validCoins,
AckFee: validCoins2,
TimeoutFee: validCoins3,
},
suite.chainA.SenderAccount.GetAddress().String(),
[]string{},
)
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), identifiedFee)
suite.Require().NoError(err)
},
true,
},
{
"identified packet fee doesn't exist",
func(suite *FeeTestSuite) {
suite.Require().Error(types.ErrFeeNotFound)
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't actually testing the OnAcknowledgementPacket call, it's just testing your setup functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh, I think see what you're doing here. The setup in suite.run fails when you don't explicitly escrow the packet etc in each test case. I think it would be easier to read (and also reduce code) if you called EscrowPacketFee after calling malleate and used each case to malleate/change the data to force the test to fail :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not really that the setup fails but that the OnAck callback checks if an identified fee exists, so if it hasn't been escrowed then that error should surface

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I see. I feel like it's a bit hard to read using the malleate functions like this (I think the general approach is you change (malleate) data here to force an error rather than have the default case fail) but I think it's ok for now :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the purpose of suite.Require().Error(types.ErrFeeNotFound). It is just asserting that types.ErrFeeNotFound is of type error, it isn't checking any logic related to ibc_module.go

},
false,
},
{
"refundAccount doesn't exist",
func(suite *FeeTestSuite) {
packetId := channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, suite.chainA.SenderAccount.GetSequence())
identifiedFee := types.NewIdentifiedPacketFee(
packetId,
types.Fee{
ReceiveFee: validCoins,
AckFee: validCoins2,
TimeoutFee: validCoins3,
},
suite.chainB.SenderAccount.GetAddress().String(),
[]string{},
)
suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), identifiedFee)
suite.Require().Error(types.ErrRefundAccNotFound)
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
},
false,
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest()
suite.coordinator.SetupConnections(suite.path)

// open incentivized channel
suite.path.EndpointA.ChanOpenInit()
suite.path.EndpointB.ChanOpenTry()
suite.path.EndpointA.ChanOpenAck()
suite.path.EndpointB.ChanOpenConfirm()

// set up coins
coin := ibctesting.TestCoin
fungibleTokenPacket := transfertypes.NewFungibleTokenPacketData(
coin.Denom,
sdk.NewInt(100).Uint64(),
suite.chainA.SenderAccount.GetAddress().String(),
suite.chainB.SenderAccount.GetAddress().String(),
)
packet := channeltypes.NewPacket(
fungibleTokenPacket.GetBytes(),
suite.chainA.SenderAccount.GetSequence(),
suite.path.EndpointA.ChannelConfig.PortID,
suite.path.EndpointA.ChannelID,
suite.path.EndpointB.ChannelConfig.PortID,
suite.path.EndpointB.ChannelID,
clienttypes.NewHeight(0, 100),
0,
)

// set up module and callbacks
module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.TransferPort)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module)
suite.Require().True(ok)

// malleate test case
tc.malleate(suite)
charleenfei marked this conversation as resolved.
Show resolved Hide resolved

ack := types.IncentivizedAcknowledgement{
Result: channeltypes.NewResultAcknowledgement([]byte{1}).Acknowledgement(),
ForwardRelayerAddress: suite.chainB.SenderAccount.GetAddress().String(),
}
ackBytes := ack.Acknowledgement()

err = cbs.OnAcknowledgementPacket(suite.chainA.GetContext(), packet, ackBytes, suite.chainB.SenderAccount.GetAddress())
if tc.expPass {
suite.Require().NoError(err, "unexpected error for case: %s", tc.name)
} else {
suite.Require().Error(err, "%s expected error but returned none", tc.name)
}
})
}
}

func (suite *FeeTestSuite) TestOnTimeoutPacket() {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
testCases := []struct {
name string
malleate func(suite *FeeTestSuite)
expPass bool
}{
{
"success",
func(suite *FeeTestSuite) {
packetId := channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, suite.chainA.SenderAccount.GetSequence())
identifiedFee := types.NewIdentifiedPacketFee(
packetId,
types.Fee{
ReceiveFee: validCoins,
AckFee: validCoins2,
TimeoutFee: validCoins3,
},
suite.chainA.SenderAccount.GetAddress().String(),
[]string{},
)
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), identifiedFee)
suite.Require().NoError(err)

// fund chain A's escrow path so that tokens can be unescrowed upon timeout
escrow := transfertypes.GetEscrowAddress(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)
trace := transfertypes.ParseDenomTrace(sdk.DefaultBondDenom)
coin := sdk.NewCoin(trace.IBCDenom(), sdk.NewInt(100))

suite.Require().NoError(simapp.FundAccount(suite.chainA.GetSimApp(), suite.chainA.GetContext(), escrow, sdk.NewCoins(coin)))
},
true,
},
{
"identified packet fee doesn't exist",
func(suite *FeeTestSuite) {},
false,
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest()
suite.coordinator.SetupConnections(suite.path)

// open incentivized channel
suite.path.EndpointA.ChanOpenInit()
suite.path.EndpointB.ChanOpenTry()
suite.path.EndpointA.ChanOpenAck()
suite.path.EndpointB.ChanOpenConfirm()

// set up coins
coin := ibctesting.TestCoin
fungibleTokenPacket := transfertypes.NewFungibleTokenPacketData(
coin.Denom,
sdk.NewInt(100).Uint64(),
suite.chainA.SenderAccount.GetAddress().String(),
suite.chainB.SenderAccount.GetAddress().String(),
)
packet := channeltypes.NewPacket(
fungibleTokenPacket.GetBytes(),
suite.chainA.SenderAccount.GetSequence(),
suite.path.EndpointA.ChannelConfig.PortID,
suite.path.EndpointA.ChannelID,
suite.path.EndpointB.ChannelConfig.PortID,
suite.path.EndpointB.ChannelID,
clienttypes.NewHeight(0, 100),
0,
)
charleenfei marked this conversation as resolved.
Show resolved Hide resolved

// set up module and callbacks
module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.TransferPort)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module)
suite.Require().True(ok)

// malleate test case
tc.malleate(suite)
err = cbs.OnTimeoutPacket(suite.chainA.GetContext(), packet, suite.chainA.SenderAccount.GetAddress())

if tc.expPass {
suite.Require().NoError(err, "unexpected error for case: %s", tc.name)
} else {
suite.Require().Error(err, "%s expected error but returned none", tc.name)
}
})
}
}
Loading