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

chore: update grpc queries to handle multiple fees #967

Merged
merged 11 commits into from
Feb 24, 2022
Merged
4 changes: 2 additions & 2 deletions docs/ibc/proto-docs.md
Original file line number Diff line number Diff line change
Expand Up @@ -946,7 +946,7 @@ QueryIncentivizedPacketsResponse is the response type for the incentivized packe

| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| `incentivized_packet` | [IdentifiedPacketFee](#ibc.applications.fee.v1.IdentifiedPacketFee) | | Incentivized_packet |
| `incentivized_packet` | [IdentifiedPacketFees](#ibc.applications.fee.v1.IdentifiedPacketFees) | | Incentivized_packet |



Expand Down Expand Up @@ -977,7 +977,7 @@ QueryIncentivizedPacketsResponse is the response type for the incentivized packe

| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| `incentivized_packets` | [IdentifiedPacketFee](#ibc.applications.fee.v1.IdentifiedPacketFee) | repeated | Map of all incentivized_packets |
| `incentivized_packets` | [IdentifiedPacketFees](#ibc.applications.fee.v1.IdentifiedPacketFees) | repeated | Map of all incentivized_packets |



Expand Down
36 changes: 23 additions & 13 deletions modules/apps/29-fee/keeper/grpc_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package keeper

import (
"context"
"strconv"
"strings"

"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
Expand All @@ -12,6 +14,7 @@ import (
"github.com/cosmos/cosmos-sdk/types/query"

"github.com/cosmos/ibc-go/v3/modules/apps/29-fee/types"
channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
)

var _ types.QueryServer = Keeper{}
Expand All @@ -24,22 +27,30 @@ func (k Keeper) IncentivizedPackets(c context.Context, req *types.QueryIncentivi

ctx := sdk.UnwrapSDKContext(c).WithBlockHeight(int64(req.QueryHeight))

var packets []*types.IdentifiedPacketFee
store := prefix.NewStore(ctx.KVStore(k.storeKey), []byte(types.FeeInEscrowPrefix))
_, err := query.Paginate(store, req.Pagination, func(_, value []byte) error {
result := k.MustUnmarshalFee(value)
packets = append(packets, &result)
var identifiedPackets []types.IdentifiedPacketFees
store := prefix.NewStore(ctx.KVStore(k.storeKey), []byte(types.FeesInEscrowPrefix))
_, err := query.Paginate(store, req.Pagination, func(key, value []byte) error {
keySplit := strings.Split(string(key), "/")

channelID, portID := keySplit[2], keySplit[1]
seq, err := strconv.ParseUint(keySplit[3], 10, 64)
if err != nil {
panic(err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? From my investigation, pagination doesn't include the prefix iterated on in the return value. I guess there's a trailing "/" leading the key, which is subtle

So I think the key is something like "/transfer/channel-0/5"

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm kinda wondering if we should just reconstruct the full key and then use the parse functions in keys.go

Copy link
Member Author

@damiannolan damiannolan Feb 24, 2022

Choose a reason for hiding this comment

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

Yeah, correct, so a paginated query doesn't include the prefix, but because of the leading "/" when we split using the slash separator the first element is an empty string. So with your example, keySplit ends up as a 4 element slice:
[]string{"", "transfer", "channel-0", "5"}.

I do think since you've added the parse function we should try to leverage it here. It should be something like below, right?

packetID, err := types.ParseKeyFeesInEscrow(types.FeesInEscrowPrefix + string(key))

edit: Done ✅


packetID := channeltypes.NewPacketId(channelID, portID, seq)
packetFees := k.MustUnmarshalFees(value)

identifiedPackets = append(identifiedPackets, types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees))
return nil
})

if err != nil {
return nil, status.Error(
codes.NotFound, err.Error(),
)
return nil, status.Error(codes.NotFound, err.Error())
}

return &types.QueryIncentivizedPacketsResponse{
IncentivizedPackets: packets,
IncentivizedPackets: identifiedPackets,
}, nil
}

Expand All @@ -51,15 +62,14 @@ func (k Keeper) IncentivizedPacket(c context.Context, req *types.QueryIncentiviz

ctx := sdk.UnwrapSDKContext(c).WithBlockHeight(int64(req.QueryHeight))

fee, exists := k.GetFeeInEscrow(ctx, req.PacketId)
feesInEscrow, exists := k.GetFeesInEscrow(ctx, req.PacketId)
if !exists {
return nil, status.Error(
codes.NotFound,
sdkerrors.Wrap(types.ErrFeeNotFound, req.PacketId.String()).Error(),
)
sdkerrors.Wrapf(types.ErrFeeNotFound, "channel: %s, port: %s, sequence: %d", req.PacketId.ChannelId, req.PacketId.PortId, req.PacketId.Sequence).Error())
}

return &types.QueryIncentivizedPacketResponse{
IncentivizedPacket: &fee,
IncentivizedPacket: types.NewIdentifiedPacketFees(req.PacketId, feesInEscrow.PacketFees),
}, nil
}
138 changes: 60 additions & 78 deletions modules/apps/29-fee/keeper/grpc_query_test.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package keeper_test

import (
"fmt"

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

Expand All @@ -11,25 +9,10 @@ import (
ibctesting "github.com/cosmos/ibc-go/v3/testing"
)

func (suite *KeeperTestSuite) TestQueryIncentivizedPacketI() {

func (suite *KeeperTestSuite) TestQueryIncentivizedPackets() {
var (
req *types.QueryIncentivizedPacketRequest
)

// setup
suite.coordinator.Setup(suite.path) // setup channel
validPacketId := channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, 1)
invalidPacketId := channeltypes.NewPacketId(ibctesting.FirstChannelID, ibctesting.MockFeePort, 2)
identifiedPacketFee := types.NewIdentifiedPacketFee(
validPacketId,
types.Fee{
AckFee: sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))),
RecvFee: sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))),
TimeoutFee: sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))),
},
"", // leave empty here and then populate on each testcase since suite resets
[]string(nil),
req *types.QueryIncentivizedPacketsRequest
expectedPackets []types.IdentifiedPacketFees
)

testCases := []struct {
Expand All @@ -40,117 +23,116 @@ func (suite *KeeperTestSuite) TestQueryIncentivizedPacketI() {
{
"success",
func() {
req = &types.QueryIncentivizedPacketRequest{
PacketId: validPacketId,
suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), ibctesting.MockFeePort, ibctesting.FirstChannelID)

fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee)
packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), []string(nil))

for i := 0; i < 3; i++ {
// escrow packet fees for three different packets
packetID := channeltypes.NewPacketId(ibctesting.FirstChannelID, ibctesting.MockFeePort, uint64(i+1))
suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee)

expectedPackets = append(expectedPackets, types.NewIdentifiedPacketFees(packetID, []types.PacketFee{packetFee}))
}

req = &types.QueryIncentivizedPacketsRequest{
Pagination: &query.PageRequest{
Limit: 5,
CountTotal: false,
},
QueryHeight: 0,
}
},
true,
},
{
"packetId not found",
"empty pagination",
func() {
req = &types.QueryIncentivizedPacketRequest{
PacketId: invalidPacketId,
QueryHeight: 0,
}
expectedPackets = nil
req = &types.QueryIncentivizedPacketsRequest{}
},
false,
true,
},
}

for _, tc := range testCases {
suite.Run(tc.name, func() {
suite.SetupTest() // reset

refundAcc := suite.chainA.SenderAccount.GetAddress()
// populate RefundAddress field
identifiedPacketFee.RefundAddress = refundAcc.String()

tc.malleate()

suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), validPacketId.PortId, validPacketId.ChannelId)
suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeInEscrow(suite.chainA.GetContext(), identifiedPacketFee)
tc.malleate() // malleate mutates test data

ctx := sdk.WrapSDKContext(suite.chainA.GetContext())
res, err := suite.queryClient.IncentivizedPacket(ctx, req)
res, err := suite.queryClient.IncentivizedPackets(ctx, req)

if tc.expPass {
suite.Require().NoError(err)
suite.Require().NotNil(res)
suite.Require().Equal(&identifiedPacketFee, res.IncentivizedPacket)
suite.Require().Equal(expectedPackets, res.IncentivizedPackets)
} else {
suite.Require().Error(err)
}
})
}
}

func (suite *KeeperTestSuite) TestQueryIncentivizedPackets() {
func (suite *KeeperTestSuite) TestQueryIncentivizedPacket() {
var (
req *types.QueryIncentivizedPacketsRequest
expPackets []*types.IdentifiedPacketFee
req *types.QueryIncentivizedPacketRequest
)

fee := types.Fee{
AckFee: sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(100)}},
RecvFee: sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(100)}},
TimeoutFee: sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(100)}},
}

testCases := []struct {
msg string
name string
malleate func()
expPass bool
}{
{
"empty pagination",
func() {
req = &types.QueryIncentivizedPacketsRequest{}
},
"success",
func() {},
true,
},
{
"success",
"fees not found for packet id",
func() {
refundAcc := suite.chainA.SenderAccount.GetAddress()

fee1 := types.NewIdentifiedPacketFee(channeltypes.NewPacketId(ibctesting.FirstChannelID, ibctesting.MockFeePort, 1), fee, refundAcc.String(), []string(nil))
fee2 := types.NewIdentifiedPacketFee(channeltypes.NewPacketId(ibctesting.FirstChannelID, ibctesting.MockFeePort, 2), fee, refundAcc.String(), []string(nil))
fee3 := types.NewIdentifiedPacketFee(channeltypes.NewPacketId(ibctesting.FirstChannelID, ibctesting.MockFeePort, 3), fee, refundAcc.String(), []string(nil))

expPackets = []*types.IdentifiedPacketFee{}
expPackets = append(expPackets, &fee1, &fee2, &fee3)

suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), ibctesting.MockFeePort, ibctesting.FirstChannelID)
for _, packetFee := range expPackets {
suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeInEscrow(suite.chainA.GetContext(), *packetFee)
}

req = &types.QueryIncentivizedPacketsRequest{
Pagination: &query.PageRequest{
Limit: 5,
CountTotal: false,
},
req = &types.QueryIncentivizedPacketRequest{
PacketId: channeltypes.NewPacketId(ibctesting.FirstChannelID, ibctesting.MockFeePort, 100),
QueryHeight: 0,
}
},
true,
false,
},
}

for _, tc := range testCases {
suite.Run(fmt.Sprintf("Case %s", tc.msg), func() {
suite.Run(tc.name, func() {
suite.SetupTest() // reset
tc.malleate()
ctx := sdk.WrapSDKContext(suite.chainA.GetContext())

res, err := suite.queryClient.IncentivizedPackets(ctx, req)
suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), ibctesting.MockFeePort, ibctesting.FirstChannelID)

packetID := channeltypes.NewPacketId(ibctesting.FirstChannelID, ibctesting.MockFeePort, 1)
fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee)
packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), []string(nil))

for i := 0; i < 3; i++ {
// escrow three packet fees for the same packet
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee)
suite.Require().NoError(err)
}

req = &types.QueryIncentivizedPacketRequest{
PacketId: packetID,
QueryHeight: 0,
Comment on lines +110 to +124
Copy link
Contributor

Choose a reason for hiding this comment

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

haha I just thought to do the same on #983

}

tc.malleate() // malleate mutates test data

ctx := sdk.WrapSDKContext(suite.chainA.GetContext())
res, err := suite.queryClient.IncentivizedPacket(ctx, req)

if tc.expPass {
suite.Require().NoError(err)
suite.Require().NotNil(res)
suite.Require().Equal(expPackets, res.IncentivizedPackets)
suite.Require().Equal(types.NewIdentifiedPacketFees(packetID, []types.PacketFee{packetFee, packetFee, packetFee}), res.IncentivizedPacket)
} else {
suite.Require().Error(err)
}
Expand Down
9 changes: 9 additions & 0 deletions modules/apps/29-fee/types/fee.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
)

// NewPacketFee creates and returns a new PacketFee struct including the incentivization fees, refund addres and relayers
Expand All @@ -23,6 +24,14 @@ func NewPacketFees(packetFees []PacketFee) PacketFees {
}
}

// NewIdentifiedPacketFees creates and returns a new IdentifiedPacketFees struct containing a packet ID and packet fees
func NewIdentifiedPacketFees(packetID channeltypes.PacketId, packetFees []PacketFee) IdentifiedPacketFees {
return IdentifiedPacketFees{
PacketId: packetID,
PacketFees: packetFees,
}
}

// NewFee creates and returns a new Fee struct encapsulating the receive, acknowledgement and timeout fees as sdk.Coins
func NewFee(recvFee, ackFee, timeoutFee sdk.Coins) Fee {
return Fee{
Expand Down
Loading