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: Make a state entry to find total amount of native assets IBC'd out #3019

Merged
merged 63 commits into from
Apr 22, 2023
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
63 commits
Select commit Hold shift + click to select a range
8290b79
started
stackman27 Jan 16, 2023
1421e5a
added state and query
stackman27 Jan 16, 2023
93d8454
fixed balance
stackman27 Jan 16, 2023
f0768e9
nit
stackman27 Jan 16, 2023
7c868c6
carlos and nicholas feedback
stackman27 Jan 18, 2023
03023fc
addressed carlos comments 2
stackman27 Feb 8, 2023
2f5c571
rebased
stackman27 Feb 8, 2023
ffa737f
add extra tests and handle situation where source tokens have IBC denom
Feb 17, 2023
468f00f
fix typo
Feb 17, 2023
c6ba4c5
fix typo
Feb 17, 2023
e4972f0
alignment
Feb 17, 2023
6201c08
fix alignment
Feb 17, 2023
c8f6087
gofumpt
Feb 17, 2023
f6b6079
add error checking
Feb 17, 2023
7d44018
remove cache context and check escrow amount only on success tests cases
Feb 19, 2023
3948566
Update modules/apps/transfer/keeper/keeper.go
Feb 21, 2023
914cae0
add IsNativeDenom function to denom trace
Feb 23, 2023
df3f7e6
add support to track total escrow for IBC denoms when sender is source
Mar 1, 2023
f77f7a8
added panics
stackman27 Mar 13, 2023
f532101
nit
stackman27 Mar 13, 2023
daeacf8
fixed test
stackman27 Mar 15, 2023
bd3600f
Merge branch 'main' into stack/total-ibc-out
stackman27 Mar 15, 2023
5c676dd
Update proto/ibc/applications/transfer/v1/query.proto
Mar 23, 2023
b18c78e
Update proto/ibc/applications/transfer/v1/query.proto
Mar 23, 2023
01ff13d
address review comment/extend e2e transfer test
Mar 23, 2023
b1ba2b7
Merge branch 'main' into stack/total-ibc-out
Mar 23, 2023
15c5ef4
fix conflicts
Mar 23, 2023
ea7e76c
revert changes
Mar 23, 2023
72e871c
missing argument
Mar 23, 2023
6071ad2
fix typos
Mar 23, 2023
94357f8
fix test
Mar 23, 2023
ce36ec1
Merge branch 'main' into stack/total-ibc-out
stackman27 Mar 23, 2023
c9b304c
fix e2e test / add genesis
Mar 25, 2023
9569e8a
add wip upgrade test / fix support for amounts larger than int64
Mar 26, 2023
dfab5f8
add missing files
Mar 26, 2023
f3213d8
return plain string for escrow amount
Mar 26, 2023
fb426c1
fixing tests
Mar 26, 2023
659b10a
add test for set/get total escrow functions
Mar 26, 2023
2abcfb1
variable renaming
Mar 26, 2023
6d68dc3
add check on key format
Mar 26, 2023
aa54c5f
add tests for get all denom escrows query
Mar 27, 2023
9b25880
add extra testcase
Mar 27, 2023
28627f9
check denomination is valid in grpc query
Mar 27, 2023
920ae09
gofumpt
Mar 27, 2023
0ecabea
Merge branch 'main' into stack/total-ibc-out
Mar 27, 2023
966df1f
Merge branch 'main' into stack/total-ibc-out
Mar 29, 2023
3c2e47b
adding denoms escrow checks in upgrade e2e tests / break API of trans…
Mar 29, 2023
38a91ab
rename test names
Mar 30, 2023
817c888
Merge branch 'main' into stack/total-ibc-out
Mar 30, 2023
1574d3f
addressing a bunch of review comments
Apr 11, 2023
540e3ac
addressing more review comments
Apr 15, 2023
7310277
review comment
Apr 15, 2023
07d3266
Merge branch 'main' into stack/total-ibc-out
Apr 15, 2023
b3fcf69
e2e: add transfer query client
Apr 15, 2023
8057568
change grpc gateway URI for total escrow
Apr 15, 2023
e3267e8
position nit
Apr 17, 2023
a0757c9
address walkthrough comments
Apr 18, 2023
5ba4f59
import formatting
Apr 18, 2023
4047be9
comment out failing e2e test for now / rename function / add comments
Apr 22, 2023
77655f2
Merge branch 'main' into stack/total-ibc-out
Apr 22, 2023
9c036e4
rename receiver
Apr 22, 2023
aac5321
rename receiver (2)
Apr 22, 2023
107f4d6
comment out import
Apr 22, 2023
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
33 changes: 33 additions & 0 deletions modules/apps/transfer/client/cli/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,3 +167,36 @@ func GetCmdQueryDenomHash() *cobra.Command {
flags.AddQueryFlagsToCmd(cmd)
return cmd
}

// GetCmdQueryTotalEscrowForDenom defines the command to query the total amount of tokens in escrow for a native denom
func GetCmdQueryTotalEscrowForDenom() *cobra.Command {
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
cmd := &cobra.Command{
Use: "token-escrow [denom]",
Short: "Query the total amount of tokens of a native denom in escrow",
Long: "Query the total amount of tokens of a native denom in escrow",
Example: fmt.Sprintf("%s query ibc-transfer token-escrow uosmo", version.AppName),
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
clientCtx, err := client.GetClientQueryContext(cmd)
if err != nil {
return err
}

queryClient := types.NewQueryClient(clientCtx)

req := &types.QueryTotalEscrowFormDenomRequest{
Denom: args[0],
}

res, err := queryClient.TotalEscrowForDenom(cmd.Context(), req)
if err != nil {
return err
}

return clientCtx.PrintProto(res)
},
}

flags.AddQueryFlagsToCmd(cmd)
return cmd
}
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
19 changes: 19 additions & 0 deletions modules/apps/transfer/keeper/grpc_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,3 +121,22 @@ func (q Keeper) EscrowAddress(c context.Context, req *types.QueryEscrowAddressRe
EscrowAddress: addr.String(),
}, nil
}

// TotalEscrowForDenom implements the TotalEscrowForDenom gRPC method.
func (q Keeper) TotalEscrowForDenom(c context.Context, req *types.QueryTotalEscrowFormDenomRequest) (*types.QueryTotalEscrowForDenomResponse, 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 (q Keeper) TotalEscrowForDenom(c context.Context, req *types.QueryTotalEscrowFormDenomRequest) (*types.QueryTotalEscrowForDenomResponse, error) {
func (k Keeper) TotalEscrowForDenom(c context.Context, req *types.QueryTotalEscrowFormDenomRequest) (*types.QueryTotalEscrowForDenomResponse, error) {

Copy link
Contributor

Choose a reason for hiding this comment

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

We use q in the rest of the file, and I believe that's the convention we are following for gRPC queries in general.

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember there being a note to change these from q to k, but I can't find the comment or issue. It isn't consistent in the code base, see ics27. My general preference would be to use k as that makes logical sense to me, but I'm okay to open a separate issue

if req == nil {
return nil, status.Error(codes.InvalidArgument, "empty request")
}

ctx := sdk.UnwrapSDKContext(c)

if q.IsIBCDenom(ctx, req.Denom) {
return nil, status.Error(codes.InvalidArgument, "denom is not a native token denomination")
}
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

denomAmount := q.GetTotalEscrowForDenom(ctx, req.Denom)

return &types.QueryTotalEscrowForDenomResponse{
Amount: denomAmount.Int64(),
}, nil
}
77 changes: 77 additions & 0 deletions modules/apps/transfer/keeper/grpc_query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -261,3 +261,80 @@ func (suite *KeeperTestSuite) TestEscrowAddress() {
})
}
}

func (suite *KeeperTestSuite) TestTotalEscrowForDenom() {
var req *types.QueryTotalEscrowFormDenomRequest

testCases := []struct {
msg string
malleate func()
expPass bool
}{
{
"success",
func() {
req = &types.QueryTotalEscrowFormDenomRequest{
Denom: sdk.DefaultBondDenom,
}
},
true,
},
{
"not found denom trace",
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
func() {
denomTrace := types.DenomTrace{
Path: "transfer/channel-0",
BaseDenom: sdk.DefaultBondDenom,
}

req = &types.QueryTotalEscrowFormDenomRequest{
Denom: denomTrace.IBCDenom(),
}
},
true, // consider the denom is of a native token
},
{
"invalid ibc denom",
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
func() {
req = &types.QueryTotalEscrowFormDenomRequest{
Denom: "ibc/𓃠🐾",
}
},
true, // consider the denom is of a native token
},
{
"non-native denom",
func() {
denomTrace := types.DenomTrace{
Path: "transfer/channel-0",
BaseDenom: sdk.DefaultBondDenom,
}

suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace(suite.chainA.GetContext(), denomTrace)

req = &types.QueryTotalEscrowFormDenomRequest{
Denom: denomTrace.IBCDenom(),
}
},
false,
},
}

for _, tc := range testCases {
suite.Run(fmt.Sprintf("Case %s", tc.msg), func() {
suite.SetupTest() // reset

tc.malleate()
ctx := sdk.WrapSDKContext(suite.chainA.GetContext())

res, err := suite.queryClient.TotalEscrowForDenom(ctx, req)

if tc.expPass {
suite.Require().NoError(err)
suite.Require().Equal(int64(0), res.Amount)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
} else {
suite.Require().Error(err)
}
})
}
}
45 changes: 45 additions & 0 deletions modules/apps/transfer/keeper/keeper.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
package keeper

import (
"fmt"
"strings"

"cosmossdk.io/math"
tmbytes "github.com/cometbft/cometbft/libs/bytes"
"github.com/cometbft/cometbft/libs/log"
"github.com/cosmos/cosmos-sdk/codec"
Expand Down Expand Up @@ -151,3 +155,44 @@ func (k Keeper) AuthenticateCapability(ctx sdk.Context, cap *capabilitytypes.Cap
func (k Keeper) ClaimCapability(ctx sdk.Context, cap *capabilitytypes.Capability, name string) error {
return k.scopedKeeper.ClaimCapability(ctx, cap, name)
}

// GetTotalEscrowForDenom gets the total amount of source chain tokens that are in escrow.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
func (k Keeper) GetTotalEscrowForDenom(ctx sdk.Context, denom string) math.Int {
store := ctx.KVStore(k.storeKey)
bz := store.Get(types.TotalEscrowForDenomKey(denom))
if bz == nil {
return math.ZeroInt()
}

var amount math.Int
if err := amount.Unmarshal(bz); err != nil {
panic(err)
}
return amount
}

// SetTotalEscrowForDenom stores the total amount of source chain tokens that are in escrow.
func (k Keeper) SetTotalEscrowForDenom(ctx sdk.Context, denom string, amount math.Int) {
if amount.LT(math.ZeroInt()) {
panic(fmt.Sprintf("amount cannot be negative: %s", amount))
}

store := ctx.KVStore(k.storeKey)
bz, err := amount.Marshal()
if err != nil {
panic(err)
}

store.Set(types.TotalEscrowForDenomKey(denom), bz)
}

// IsIBCDenom returns true if the denomination is an known on-chain IBC denomination.
func (k Keeper) IsIBCDenom(ctx sdk.Context, denom string) bool {
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
if strings.HasPrefix(denom, fmt.Sprintf("%s/", types.DenomPrefix)) {
_, err := k.DenomPathFromHash(ctx, denom)
if err == nil {
return true
}
}
return false
}
35 changes: 35 additions & 0 deletions modules/apps/transfer/keeper/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package keeper
import (
"fmt"

"cosmossdk.io/math"
sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"
Expand Down Expand Up @@ -53,6 +54,40 @@ func (m Migrator) MigrateTraces(ctx sdk.Context) error {
return nil
}

// MigrateTotalEscrowForDenom migrates the total amount of source chain tokens in escrow.
func (m Migrator) MigrateTotalEscrowForDenom(ctx sdk.Context) error {
nativeTokens := make(map[string]math.Int)

transferChannels := m.keeper.channelKeeper.GetAllChannelsWithPortPrefix(ctx, types.PortID)
for _, channel := range transferChannels {
escrowAddress := types.GetEscrowAddress(types.PortID, channel.ChannelId)
escrowBalances := m.keeper.bankKeeper.GetAllBalances(ctx, escrowAddress)

for _, escrowBalance := range escrowBalances {
// Denom possibilities:
// - "atom" = native denom
// - "ibc/<hash>" = non native denom
// - "atom/ibc/osmo" = native denom

if !m.keeper.IsIBCDenom(ctx, escrowBalance.Denom) {
stackman27 marked this conversation as resolved.
Show resolved Hide resolved
if val, ok := nativeTokens[escrowBalance.Denom]; ok {
nativeTokens[escrowBalance.Denom] = val.Add(escrowBalance.Amount)
} else {
nativeTokens[escrowBalance.Denom] = escrowBalance.Amount
}
}
}
}

if len(nativeTokens) != 0 {
for denom, amount := range nativeTokens {
m.keeper.SetTotalEscrowForDenom(ctx, denom, amount)
}
Fixed Show fixed Hide fixed
}

return nil
}

func equalTraces(dtA, dtB types.DenomTrace) bool {
return dtA.BaseDenom == dtB.BaseDenom && dtA.Path == dtB.Path
}
80 changes: 80 additions & 0 deletions modules/apps/transfer/keeper/migrations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,12 @@ package keeper_test
import (
"fmt"

"cosmossdk.io/math"
sdk "github.com/cosmos/cosmos-sdk/types"
banktestutil "github.com/cosmos/cosmos-sdk/x/bank/testutil"
transferkeeper "github.com/cosmos/ibc-go/v7/modules/apps/transfer/keeper"
transfertypes "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"
ibctesting "github.com/cosmos/ibc-go/v7/testing"
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
)

func (suite *KeeperTestSuite) TestMigratorMigrateTraces() {
Expand Down Expand Up @@ -119,3 +123,79 @@ func (suite *KeeperTestSuite) TestMigratorMigrateTracesCorruptionDetection() {
migrator.MigrateTraces(suite.chainA.GetContext()) //nolint:errcheck // we shouldn't check the error here because we want to ensure that a panic occurs.
})
}

func (suite *KeeperTestSuite) TestMigrateTotalEscrowForDenom() {
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
var path *ibctesting.Path

testCases := []struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

missing test case for multiple denoms in one balance

msg string
malleate func()
expectedEscrowAmt math.Int
}{
{
"success: one native denom escrowed in one channel",
func() {
escrowAddress := transfertypes.GetEscrowAddress(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
coin := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))

// funds the escrow account to have balance
suite.Require().NoError(banktestutil.FundAccount(suite.chainA.GetSimApp().BankKeeper, suite.chainA.GetContext(), escrowAddress, sdk.NewCoins(coin)))
},
math.NewInt(100),
},
{
"success: one native denom escrowed in two channels",
func() {
extraPath := NewTransferPath(suite.chainA, suite.chainB)
suite.coordinator.Setup(extraPath)

escrowAddress1 := transfertypes.GetEscrowAddress(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
escrowAddress2 := transfertypes.GetEscrowAddress(extraPath.EndpointA.ChannelConfig.PortID, extraPath.EndpointA.ChannelID)
coin1 := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))
coin2 := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))

// funds the escrow accounts to have balance
suite.Require().NoError(banktestutil.FundAccount(suite.chainA.GetSimApp().BankKeeper, suite.chainA.GetContext(), escrowAddress1, sdk.NewCoins(coin1)))
suite.Require().NoError(banktestutil.FundAccount(suite.chainA.GetSimApp().BankKeeper, suite.chainA.GetContext(), escrowAddress2, sdk.NewCoins(coin2)))
},
math.NewInt(200),
},
{
"success: valid ibc denom escrowed in one channel",
func() {
escrowAddress := transfertypes.GetEscrowAddress(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
trace := transfertypes.ParseDenomTrace(transfertypes.GetPrefixedDenom(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, sdk.DefaultBondDenom))
coin := sdk.NewCoin(trace.IBCDenom(), sdk.NewInt(100))

suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace(suite.chainA.GetContext(), trace)

// funds the escrow accounts to have balance
suite.Require().NoError(banktestutil.FundAccount(suite.chainA.GetSimApp().BankKeeper, suite.chainA.GetContext(), escrowAddress, sdk.NewCoins(coin)))
},
math.NewInt(0),
},
}

for _, tc := range testCases {
suite.Run(fmt.Sprintf("Case %s", tc.msg), func() {
suite.SetupTest() // reset

path = NewTransferPath(suite.chainA, suite.chainB)
suite.coordinator.Setup(path)

tc.malleate() // explicitly fund escrow account

migrator := transferkeeper.NewMigrator(suite.chainA.GetSimApp().TransferKeeper)
suite.Require().NoError(migrator.MigrateTotalEscrowForDenom(suite.chainA.GetContext()))

// check that the migration set the expected amount for the native tokens
amount := suite.chainA.GetSimApp().TransferKeeper.GetTotalEscrowForDenom(suite.chainA.GetContext(), sdk.DefaultBondDenom)
suite.Require().Equal(tc.expectedEscrowAmt, amount)

// check that the migration did not set amount for non-native tokens
trace := transfertypes.ParseDenomTrace(transfertypes.GetPrefixedDenom(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, sdk.DefaultBondDenom))
amount = suite.chainA.GetSimApp().TransferKeeper.GetTotalEscrowForDenom(suite.chainA.GetContext(), trace.IBCDenom())
suite.Require().Equal(math.ZeroInt(), amount)
})
}
}
24 changes: 22 additions & 2 deletions modules/apps/transfer/keeper/relay.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,13 @@ func (k Keeper) sendTransfer(
); err != nil {
return 0, err
}

// get the existing total amount in escrow
currentTotalEscrow := k.GetTotalEscrowForDenom(ctx, token.GetDenom())
newTotalEscrow := currentTotalEscrow.Add(token.Amount)

// store the new total amount in escrow
k.SetTotalEscrowForDenom(ctx, token.GetDenom(), newTotalEscrow)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can update the comments to indicate why we are doing this as opposed to what the function means. Maybe something like:

Suggested change
// get the existing total amount in escrow
currentTotalEscrow := k.GetTotalEscrowForDenom(ctx, token.GetDenom())
newTotalEscrow := currentTotalEscrow.Add(token.Amount)
// store the new total amount in escrow
k.SetTotalEscrowForDenom(ctx, token.GetDenom(), newTotalEscrow)
// track the total amount in escrow keyed by denomination to allow for efficient iteration
currentTotalEscrow := k.GetTotalEscrowForDenom(ctx, token.GetDenom())
newTotalEscrow := currentTotalEscrow.Add(token.Amount)
k.SetTotalEscrowForDenom(ctx, token.GetDenom(), newTotalEscrow)

I wonder if we should dump the attached issue into an ADR and then reference the ADR number. If there's an architectural change a few years from now that makes this extra storing unnecessary, it might help to have context of why we made this decision

} else {
labels = append(labels, telemetry.NewLabel(coretypes.LabelSource, "false"))

Expand Down Expand Up @@ -197,7 +204,6 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t
// NOTE: We use SourcePort and SourceChannel here, because the counterparty
// chain would have prefixed with DestPort and DestChannel when originally
// receiving this coin as seen in the "sender chain is the source" condition.

if types.ReceiverChainIsSource(packet.GetSourcePort(), packet.GetSourceChannel(), data.Denom) {
// sender chain is not the source, unescrow tokens

Expand All @@ -211,7 +217,7 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t
// The denomination used to send the coins is either the native denom or the hash of the path
// if the denomination is not native.
denomTrace := types.ParseDenomTrace(unprefixedDenom)
if denomTrace.Path != "" {
if !denomTrace.IsNativeDenom() {
denom = denomTrace.IBCDenom()
}
token := sdk.NewCoin(denom, transferAmount)
Expand All @@ -230,6 +236,13 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t
return errorsmod.Wrap(err, "unable to unescrow tokens, this may be caused by a malicious counterparty module or a bug: please open an issue on counterparty module")
}

// get the existing total amount in escrow
currentTotalEscrow := k.GetTotalEscrowForDenom(ctx, token.GetDenom())
newTotalEscrow := currentTotalEscrow.Sub(token.Amount)

// store the new total amount in escrow
k.SetTotalEscrowForDenom(ctx, token.GetDenom(), newTotalEscrow)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

defer func() {
if transferAmount.IsInt64() {
telemetry.SetGaugeWithLabels(
Expand Down Expand Up @@ -366,6 +379,13 @@ func (k Keeper) refundPacketToken(ctx sdk.Context, packet channeltypes.Packet, d
return errorsmod.Wrap(err, "unable to unescrow tokens, this may be caused by a malicious counterparty module or a bug: please open an issue on counterparty module")
}

// get the existing total amount in escrow
currentTotalEscrow := k.GetTotalEscrowForDenom(ctx, token.GetDenom())
newTotalEscrow := currentTotalEscrow.Sub(token.Amount)

// store the new total amount in escrow
k.SetTotalEscrowForDenom(ctx, token.GetDenom(), newTotalEscrow)

return nil
}

Expand Down
Loading