From 6f628d9ff048309648e3cbba3afb1414dcb49f51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 1 Jun 2023 09:05:32 +0200 Subject: [PATCH] imp: transfer total escrow follow ups (#3558) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * transfer (total escrow): add documentation and migration docs (#3509) * add docs for total escrow feature * revert change * fix tag * Update docs/migrations/v7-to-v7_1.md --------- Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * transfer (total escrow): some more review comments (#3519) * some more review comments * Rename pathAndEscrowAMount to pathAndEscrowAmount --------- Co-authored-by: DimitrisJim * transfer (total escrow): add invariant for total escrow (#3506) * add invariant for total escrow * address review comment * refactor: simplify logic for asserting invariant * fix: use safeAdd instead of append * Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Damian Nolan --------- Co-authored-by: Colin Axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Damian Nolan * imp: do not store total escrow when amount is zero (#3585) * do not store 0 escrow amout * adapt success test * Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Jim Fasarakis-Hilliard * Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Damian Nolan * add comments --------- Co-authored-by: Jim Fasarakis-Hilliard Co-authored-by: Damian Nolan * add feature release for total escrow state entry to conditionally query the endpoint in e2e (#3605) * fix total escrow cli documentation * Apply suggestions from code review Co-authored-by: Jim Fasarakis-Hilliard Co-authored-by: Damian Nolan * address review comments * docs: adr 011 total escrow state entry (#3641) * docs: add adr 011 for total escrow state entry * indentation * code formatting * Apply suggestions from code review Co-authored-by: Damian Nolan * Apply suggestions from code review Co-authored-by: Charly --------- Co-authored-by: Damian Nolan Co-authored-by: Charly --------- Co-authored-by: Carlos Rodriguez Co-authored-by: DimitrisJim Co-authored-by: Damian Nolan Co-authored-by: Charly --- docs/.vuepress/config.js | 5 + docs/apps/transfer/authorizations.md | 4 + docs/apps/transfer/client.md | 66 ++++++++ docs/architecture/README.md | 6 + ...r-011-transfer-total-escrow-state-entry.md | 145 ++++++++++++++++++ docs/migrations/v7-to-v7_1.md | 10 +- e2e/testconfig/testconfig.go | 6 +- e2e/tests/transfer/base_test.go | 8 +- modules/apps/transfer/client/cli/query.go | 4 +- modules/apps/transfer/keeper/genesis_test.go | 6 +- modules/apps/transfer/keeper/invariants.go | 51 ++++++ .../apps/transfer/keeper/invariants_test.go | 71 +++++++++ modules/apps/transfer/keeper/keeper.go | 22 +-- modules/apps/transfer/keeper/keeper_test.go | 20 ++- modules/apps/transfer/keeper/relay_test.go | 6 +- modules/apps/transfer/module.go | 4 +- .../ibc/applications/transfer/v1/query.proto | 1 + 17 files changed, 405 insertions(+), 30 deletions(-) create mode 100644 docs/apps/transfer/client.md create mode 100644 docs/architecture/adr-011-transfer-total-escrow-state-entry.md create mode 100644 modules/apps/transfer/keeper/invariants.go create mode 100644 modules/apps/transfer/keeper/invariants_test.go diff --git a/docs/.vuepress/config.js b/docs/.vuepress/config.js index 3613fdcc13d..e67e2ec4abb 100644 --- a/docs/.vuepress/config.js +++ b/docs/.vuepress/config.js @@ -351,6 +351,11 @@ module.exports = { directory: false, path: "/apps/transfer/authorizations.html", }, + { + title: "Client", + directory: false, + path: "/apps/transfer/client.html", + }, ], }, ], diff --git a/docs/apps/transfer/authorizations.md b/docs/apps/transfer/authorizations.md index ed6979a6f1d..1bb26260553 100644 --- a/docs/apps/transfer/authorizations.md +++ b/docs/apps/transfer/authorizations.md @@ -1,3 +1,7 @@ + + # `TransferAuthorization` `TransferAuthorization` implements the `Authorization` interface for `ibc.applications.transfer.v1.MsgTransfer`. It allows a granter to grant a grantee the privilege to submit `MsgTransfer` on its behalf. Please see the [Cosmos SDK docs](https://docs.cosmos.network/v0.47/modules/authz) for more details on granting privileges via the `x/authz` module. diff --git a/docs/apps/transfer/client.md b/docs/apps/transfer/client.md new file mode 100644 index 00000000000..515fad57d5c --- /dev/null +++ b/docs/apps/transfer/client.md @@ -0,0 +1,66 @@ + + +# Client + +## CLI + +A user can query and interact with the `transfer` module using the CLI. Use the `--help` flag to discover the available commands: + +### Query + +The `query` commands allow users to query `transfer` state. + +```shell +simd query ibc-transfer --help +``` + +#### `total-escrow` + +The `total-escrow` command allows users to query the total amount in escrow for a particular coin denomination regardless of the transfer channel from where the coins were sent out. + +```shell +simd query ibc-transfer total-escrow [denom] [flags] +``` + +Example: + +```shell +simd query ibc-transfer total-escrow samoleans +``` + +Example Output: + +```shell +amount: "100" +``` + +## gRPC + +A user can query the `transfer` module using gRPC endpoints. + +### `TotalEscrowForDenom` + +The `TotalEscrowForDenom` endpoint allows users to query the total amount in escrow for a particular coin denomination regardless of the transfer channel from where the coins were sent out. + +```shell +ibc.applications.transfer.v1.Query/TotalEscrowForDenom +``` + +Example: + +```shell +grpcurl -plaintext \ + -d '{"denom":"samoleans"}' \ + localhost:9090 \ + ibc.applications.transfer.v1.Query/TotalEscrowForDenom +``` + +Example output: + +```shell +{ + "amount": "100" +} +``` \ No newline at end of file diff --git a/docs/architecture/README.md b/docs/architecture/README.md index cbea1a892f6..d83b7a00857 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -31,7 +31,13 @@ To suggest an ADR, please make use of the [ADR template](./adr-template.md) prov | [002](./adr-002-go-module-versioning.md) | Go module versioning | Accepted | | [003](./adr-003-ics27-acknowledgement.md) | ICS27 acknowledgement format | Accepted | | [004](./adr-004-ics29-lock-fee-module.md) | ICS29 module locking upon escrow out of balance | Accepted | +| [005](./adr-005-consensus-height-events.md) | `UpdateClient` events - `ClientState` consensus heights | Accepted | +| [006](./adr-006-02-client-refactor.md) | ICS02 client refactor | Accepted | +| [007](./adr-007-solomachine-signbytes.md) | ICS06 Solo machine sign bytes | | [008](./adr-008-app-caller-cbs/adr-008-app-caller-cbs.md) | Callback to IBC ACtors | Accepted | +| [009](./adr-009-v6-ics27-msgserver.md) | ICS27 message server addition | Accepted | +| [010](./adr-010-light-clients-as-sdk-modules.md) | IBC light clients as SDK modules | Accepted | +| [011](./adr-011-transfer-total-escrow-state-entry.md) | ICS20 state entry for total amount of tokens in escrow | Accepted | | [015](./adr-015-ibc-packet-receiver.md) | IBC Packet Routing | Accepted | | [025](./adr-025-ibc-passive-channels.md) | IBC passive channels | Deprecated | | [026](./adr-026-ibc-client-recovery-mechanisms.md) | IBC client recovery mechansisms | Accepted | diff --git a/docs/architecture/adr-011-transfer-total-escrow-state-entry.md b/docs/architecture/adr-011-transfer-total-escrow-state-entry.md new file mode 100644 index 00000000000..18241965386 --- /dev/null +++ b/docs/architecture/adr-011-transfer-total-escrow-state-entry.md @@ -0,0 +1,145 @@ +# ADR 011: ICS-20 transfer state entry for total amount of tokens in escrow + +## Changelog + +* 2023-05-24: Initial draft + +## Status + +Accepted and applied in v7.1 of ibc-go + +## Context + +Every ICS-20 transfer channel has its own escrow bank account. This account is used to lock tokens that are transferred out of a chain that acts as the source of the tokens (i.e. when the tokens being transferred have not returned to the originating chain). This design makes it easy to query the balance of the escrow accounts and find out the total amount of tokens in escrow in a particular channel. However, there are use cases where it would be useful to determine the total escrowed amount of a given denomination across all channels where those tokens have been transferred out. + +For example: assuming that there are three channels between Cosmos Hub to Osmosis and 10 ATOM have been transferred from the Cosmos Hub to Osmosis on each of those channels, then we would like to know that 30 ATOM have been transferred (i.e. are locked in the escrow accounts of each channel) without needing to iterate over each escrow account to add up the balances of each. + +For a sample use case where this feature would be useful, please refer to Osmosis' rate limiting use case described in [#2664](https://github.com/cosmos/ibc-go/issues/2664). + +## Decision + +### State entry denom -> amount + +The total amount of tokens in escrow (across all transfer channels) for a given denomination is stored in state in an entry keyed by the denomination: `totalEscrowForDenom/{denom}`. + +### Panic if amount is negative + +If a negative amount is ever attempted to be stored, then the keeper function will panic: + +```go +if coin.Amount.IsNegative() { + panic(fmt.Sprintf("amount cannot be negative: %s", coin.Amount)) +} +``` + +### Delete state entry if amount is zero + +When setting the amount for a particular denomination, the value might be zero if all tokens that were transferred out of the chain have been transferred back. If this happens, then the state entry for this particular denomination will be deleted, since Cosmos SDK's `x/bank` module prunes any non-zero balances: + +```go +if coin.Amount.IsZero() { + store.Delete(key) // delete the key since Cosmos SDK x/bank module will prune any non-zero balances + return +} +``` + +### Bundle escrow/unescrow with setting state entry + +Two new functions are implemented that bundle together the operations of escrowing/unescrowing and setting the total escrow amount in state, since these operations need to be executed together. + +For escrowing tokens: + +```go +// escrowToken will send the given token from the provided sender to the escrow address. It will also +// update the total escrowed amount by adding the escrowed token to the current total escrow. +func (k Keeper) escrowToken(ctx sdk.Context, sender, escrowAddress sdk.AccAddress, token sdk.Coin) error { + if err := k.bankKeeper.SendCoins(ctx, sender, escrowAddress, sdk.NewCoins(token)); err != nil { + // failure is expected for insufficient balances + return err + } + + // track the total amount in escrow keyed by denomination to allow for efficient iteration + currentTotalEscrow := k.GetTotalEscrowForDenom(ctx, token.GetDenom()) + newTotalEscrow := currentTotalEscrow.Add(token) + k.SetTotalEscrowForDenom(ctx, newTotalEscrow) + + return nil +} +``` + +For unescrowing tokens: + +```go +// unescrowToken will send the given token from the escrow address to the provided receiver. It will also +// update the total escrow by deducting the unescrowed token from the current total escrow. +func (k Keeper) unescrowToken(ctx sdk.Context, escrowAddress, receiver sdk.AccAddress, token sdk.Coin) error { + if err := k.bankKeeper.SendCoins(ctx, escrowAddress, receiver, sdk.NewCoins(token)); err != nil { + // NOTE: this error is only expected to occur given an unexpected bug or a malicious + // counterparty module. The bug may occur in bank or any part of the code that allows + // the escrow address to be drained. A malicious counterparty module could drain the + // escrow address by allowing more tokens to be sent back then were escrowed. + 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") + } + + // track the total amount in escrow keyed by denomination to allow for efficient iteration + currentTotalEscrow := k.GetTotalEscrowForDenom(ctx, token.GetDenom()) + newTotalEscrow := currentTotalEscrow.Sub(token) + k.SetTotalEscrowForDenom(ctx, newTotalEscrow) + + return nil +} +``` + +When tokens need to be escrowed in `sendTransfer`, then `escrowToken` is called; when tokens need to be unescrowed on execution of the `OnRecvPacket`, `OnAcknowledgementPacket` or `OnTimeoutPacket` callbacks, then `unescrowToken` is called. + +### gRPC query endpoint and CLI to retrieve amount + +A gRPC query endpoint is added so that it is possible to retrieve the total amount for a given denomination: + +```proto +// TotalEscrowForDenom returns the total amount of tokens in escrow based on the denom. +rpc TotalEscrowForDenom(QueryTotalEscrowForDenomRequest) returns (QueryTotalEscrowForDenomResponse) { + option (google.api.http).get = "/ibc/apps/transfer/v1/denoms/{denom=**}/total_escrow"; +} + +// QueryTotalEscrowForDenomRequest is the request type for TotalEscrowForDenom RPC method. +message QueryTotalEscrowForDenomRequest { + string denom = 1; +} + +// QueryTotalEscrowForDenomResponse is the response type for TotalEscrowForDenom RPC method. +message QueryTotalEscrowForDenomResponse { + cosmos.base.v1beta1.Coin amount = 1 [(gogoproto.nullable) = false]; +} +``` + +And a CLI query is also available to retrieve the total amount via the command line: + +```shell +query ibc-transfer total-escrow [denom] +``` + +## Consequences + +### Positive + +* Possibility to retrieve the total amount of a particular denomination in escrow across all transfer channels without iteration. + +### Negative + +No notable consequences + +### Neutral + +* A new entry is added to state for every denomination that is transferred out of the chain. + +## References + +Issues: + +* [#2664](https://github.com/cosmos/ibc-go/issues/2664) + +PRs: + +* [#3019](https://github.com/cosmos/ibc-go/pull/3019) +* [#3558](https://github.com/cosmos/ibc-go/pull/3558) \ No newline at end of file diff --git a/docs/migrations/v7-to-v7_1.md b/docs/migrations/v7-to-v7_1.md index d024c2cb999..18cf64df01f 100644 --- a/docs/migrations/v7-to-v7_1.md +++ b/docs/migrations/v7-to-v7_1.md @@ -14,15 +14,17 @@ There are four sections based on the four potential user groups of this document ## Chains +### 09-localhost migration + In the previous release of ibc-go, the localhost `v1` light client module was deprecated and removed. The ibc-go `v7.1.0` release introduces `v2` of the 09-localhost light client module. -An [automatic migration handler](https://github.com/cosmos/ibc-go/blob/09-localhost/modules/core/module.go#L133-L145) is configured in the core IBC module to set the localhost `ClientState` and sentinel `ConnectionEnd` in state. +An [automatic migration handler](https://github.com/cosmos/ibc-go/blob/release/v7.1.x/modules/core/module.go#L127-L145) is configured in the core IBC module to set the localhost `ClientState` and sentinel `ConnectionEnd` in state. In order to use the 09-localhost client chains must update the `AllowedClients` parameter in the 02-client submodule of core IBC. This can be configured directly in the application upgrade handler or alternatively updated via the legacy governance parameter change proposal. We __strongly__ recommend chains to perform this action so that intra-ledger communication can be carried out using the familiar IBC interfaces. -See the upgrade handler code sample provided below or [follow this link](https://github.com/cosmos/ibc-go/blob/09-localhost/testing/simapp/upgrades/upgrades.go#L85) for the upgrade handler used by the ibc-go simapp. +See the upgrade handler code sample provided below or [follow this link](https://github.com/cosmos/ibc-go/blob/release/v7.1.x/testing/simapp/upgrades/upgrades.go#L85) for the upgrade handler used by the ibc-go simapp. ```go func CreateV7LocalhostUpgradeHandler( @@ -41,7 +43,9 @@ func CreateV7LocalhostUpgradeHandler( } ``` -[For more information please refer to the 09-localhost light client module documentation](../ibc/light-clients/localhost/overview.md). +### Transfer migration + +An automatic migration handler (TODO: add link after backport to v7.1.x is merged) is configured in the transfer module to set the total amount in escrow for all denominations of coins that have been sent out. For each denomination a state entry is added with the total amount of coins in escrow regardless of the channel from which they were transferred. ## IBC Apps diff --git a/e2e/testconfig/testconfig.go b/e2e/testconfig/testconfig.go index ec0415f7607..f5b015ab7f1 100644 --- a/e2e/testconfig/testconfig.go +++ b/e2e/testconfig/testconfig.go @@ -394,10 +394,10 @@ func getGenesisModificationFunction(cc ChainConfig) func(ibc.ChainConfig, []byte binary := cc.Binary version := cc.Tag - doesSimdSupportGovv1Genesis := binary == defaultBinary && testvalues.GovGenesisFeatureReleases.IsSupported(version) - doesIcadSupportGovv1Genesis := testvalues.IcadGovGenesisFeatureReleases.IsSupported(version) + simdSupportsGovV1Genesis := binary == defaultBinary && testvalues.GovGenesisFeatureReleases.IsSupported(version) + icadSupportsGovV1Genesis := testvalues.IcadGovGenesisFeatureReleases.IsSupported(version) - if doesSimdSupportGovv1Genesis || doesIcadSupportGovv1Genesis { + if simdSupportsGovV1Genesis || icadSupportsGovV1Genesis { return defaultGovv1ModifyGenesis() } diff --git a/e2e/tests/transfer/base_test.go b/e2e/tests/transfer/base_test.go index 97c65ccdf82..901c5e9b1a8 100644 --- a/e2e/tests/transfer/base_test.go +++ b/e2e/tests/transfer/base_test.go @@ -126,13 +126,13 @@ func (s *TransferTestSuite) TestMsgTransfer_Succeeds_Nonincentivized() { s.Require().Equal(expected, actualBalance) }) - t.Run("tokens are un-escrowed", func(t *testing.T) { - if testvalues.TotalEscrowFeatureReleases.IsSupported(chainAVersion) { + if testvalues.TotalEscrowFeatureReleases.IsSupported(chainAVersion) { + t.Run("tokens are un-escrowed", func(t *testing.T) { actualTotalEscrow, err := s.QueryTotalEscrowForDenom(ctx, chainA, chainADenom) s.Require().NoError(err) s.Require().Equal(sdk.NewCoin(chainADenom, sdk.NewInt(0)), actualTotalEscrow) // total escrow is zero because tokens have come back - } - }) + }) + } } // TestMsgTransfer_Fails_InvalidAddress attempts to send an IBC transfer to an invalid address and ensures diff --git a/modules/apps/transfer/client/cli/query.go b/modules/apps/transfer/client/cli/query.go index d8d90b9dfee..8ebab138c66 100644 --- a/modules/apps/transfer/client/cli/query.go +++ b/modules/apps/transfer/client/cli/query.go @@ -171,10 +171,10 @@ func GetCmdQueryDenomHash() *cobra.Command { // GetCmdQueryTotalEscrowForDenom defines the command to query the total amount of tokens in escrow for a denom func GetCmdQueryTotalEscrowForDenom() *cobra.Command { cmd := &cobra.Command{ - Use: "token-escrow [denom]", + Use: "total-escrow [denom]", Short: "Query the total amount of tokens in escrow for a denom", Long: "Query the total amount of tokens in escrow for a denom", - Example: fmt.Sprintf("%s query ibc-transfer token-escrow uosmo", version.AppName), + Example: fmt.Sprintf("%s query ibc-transfer total-escrow uosmo", version.AppName), Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { clientCtx, err := client.GetClientQueryContext(cmd) diff --git a/modules/apps/transfer/keeper/genesis_test.go b/modules/apps/transfer/keeper/genesis_test.go index 0ea92efcf52..99c25eab242 100644 --- a/modules/apps/transfer/keeper/genesis_test.go +++ b/modules/apps/transfer/keeper/genesis_test.go @@ -29,16 +29,16 @@ func (suite *KeeperTestSuite) TestGenesis() { } ) - for _, pathAndEscrowMount := range pathsAndEscrowAmounts { + for _, pathAndEscrowAmount := range pathsAndEscrowAmounts { denomTrace := types.DenomTrace{ BaseDenom: "uatom", - Path: pathAndEscrowMount.path, + Path: pathAndEscrowAmount.path, } traces = append(types.Traces{denomTrace}, traces...) suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace(suite.chainA.GetContext(), denomTrace) denom := denomTrace.IBCDenom() - amount, ok := math.NewIntFromString(pathAndEscrowMount.escrow) + amount, ok := math.NewIntFromString(pathAndEscrowAmount.escrow) suite.Require().True(ok) escrows = append(sdk.NewCoins(sdk.NewCoin(denom, amount)), escrows...) suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(suite.chainA.GetContext(), sdk.NewCoin(denom, amount)) diff --git a/modules/apps/transfer/keeper/invariants.go b/modules/apps/transfer/keeper/invariants.go new file mode 100644 index 00000000000..93e5aa36259 --- /dev/null +++ b/modules/apps/transfer/keeper/invariants.go @@ -0,0 +1,51 @@ +package keeper + +import ( + "fmt" + + sdk "github.com/cosmos/cosmos-sdk/types" + + "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types" +) + +// RegisterInvariants registers all transfer invariants +func RegisterInvariants(ir sdk.InvariantRegistry, k *Keeper) { + ir.RegisterRoute(types.ModuleName, "total-escrow-per-denom", + TotalEscrowPerDenomInvariants(k)) +} + +// AllInvariants runs all invariants of the transfer module. +func AllInvariants(k *Keeper) sdk.Invariant { + return func(ctx sdk.Context) (string, bool) { + return TotalEscrowPerDenomInvariants(k)(ctx) + } +} + +// TotalEscrowPerDenomInvariants checks that the total amount escrowed for +// each denom is not smaller than the amount stored in the state entry. +func TotalEscrowPerDenomInvariants(k *Keeper) sdk.Invariant { + return func(ctx sdk.Context) (string, bool) { + var actualTotalEscrowed sdk.Coins + + expectedTotalEscrowed := k.GetAllTotalEscrowed(ctx) + + portID := k.GetPort(ctx) + transferChannels := k.channelKeeper.GetAllChannelsWithPortPrefix(ctx, portID) + for _, channel := range transferChannels { + escrowAddress := types.GetEscrowAddress(portID, channel.ChannelId) + escrowBalances := k.bankKeeper.GetAllBalances(ctx, escrowAddress) + + actualTotalEscrowed = actualTotalEscrowed.Add(escrowBalances...) + } + + // the actual escrowed amount must be greater than or equal to the expected amount for all denominations + if !actualTotalEscrowed.IsAllGTE(expectedTotalEscrowed) { + return sdk.FormatInvariant( + types.ModuleName, + "total escrow per denom invariance", + fmt.Sprintf("found denom(s) with total escrow amount lower than expected:\nactual total escrowed: %s\nexpected total escrowed: %s", actualTotalEscrowed, expectedTotalEscrowed)), true + } + + return "", false + } +} diff --git a/modules/apps/transfer/keeper/invariants_test.go b/modules/apps/transfer/keeper/invariants_test.go new file mode 100644 index 00000000000..972060ca6b7 --- /dev/null +++ b/modules/apps/transfer/keeper/invariants_test.go @@ -0,0 +1,71 @@ +package keeper_test + +import ( + "cosmossdk.io/math" + + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/ibc-go/v7/modules/apps/transfer/keeper" + "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types" +) + +func (suite *KeeperTestSuite) TestTotalEscrowPerDenomInvariant() { + testCases := []struct { + name string + malleate func() + expPass bool + }{ + { + "success", + func() {}, + true, + }, + { + "fails with broken invariant", + func() { + // set amount for denom higher than actual value in escrow + amount := math.NewInt(200) + suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(suite.chainA.GetContext(), sdk.NewCoin(sdk.DefaultBondDenom, amount)) + }, + false, + }, + } + + for _, tc := range testCases { + tc := tc + + suite.Run(tc.name, func() { + suite.SetupTest() // reset + path := NewTransferPath(suite.chainA, suite.chainB) + suite.coordinator.Setup(path) + + amount := math.NewInt(100) + + // send coins from chain A to chain B so that we have them in escrow + coin := sdk.NewCoin(sdk.DefaultBondDenom, amount) + msg := types.NewMsgTransfer( + path.EndpointA.ChannelConfig.PortID, + path.EndpointA.ChannelID, + coin, + suite.chainA.SenderAccount.GetAddress().String(), + suite.chainB.SenderAccount.GetAddress().String(), + suite.chainA.GetTimeoutHeight(), 0, "", + ) + + res, err := suite.chainA.SendMsgs(msg) + suite.Require().NoError(err) + suite.Require().NotNil(res) + + tc.malleate() + + out, broken := keeper.TotalEscrowPerDenomInvariants(&suite.chainA.GetSimApp().TransferKeeper)(suite.chainA.GetContext()) + + if tc.expPass { + suite.Require().False(broken) + suite.Require().Empty(out) + } else { + suite.Require().True(broken) + suite.Require().NotEmpty(out) + } + }) + } +} diff --git a/modules/apps/transfer/keeper/keeper.go b/modules/apps/transfer/keeper/keeper.go index f4909015af7..e9f9918dc98 100644 --- a/modules/apps/transfer/keeper/keeper.go +++ b/modules/apps/transfer/keeper/keeper.go @@ -188,7 +188,7 @@ func (k Keeper) IterateDenomTraces(ctx sdk.Context, cb func(denomTrace types.Den func (k Keeper) GetTotalEscrowForDenom(ctx sdk.Context, denom string) sdk.Coin { store := ctx.KVStore(k.storeKey) bz := store.Get(types.TotalEscrowForDenomKey(denom)) - if bz == nil { + if len(bz) == 0 { return sdk.NewCoin(denom, sdk.ZeroInt()) } @@ -199,21 +199,30 @@ func (k Keeper) GetTotalEscrowForDenom(ctx sdk.Context, denom string) sdk.Coin { } // SetTotalEscrowForDenom stores the total amount of source chain tokens that are in escrow. +// Amount is stored in state if and only if it is not equal to zero. The function will panic +// if the amount is negative. func (k Keeper) SetTotalEscrowForDenom(ctx sdk.Context, coin sdk.Coin) { if coin.Amount.IsNegative() { panic(fmt.Sprintf("amount cannot be negative: %s", coin.Amount)) } store := ctx.KVStore(k.storeKey) + key := types.TotalEscrowForDenomKey(coin.Denom) + + if coin.Amount.IsZero() { + store.Delete(key) // delete the key since Cosmos SDK x/bank module will prune any non-zero balances + return + } + bz := k.cdc.MustMarshal(&sdk.IntProto{Int: coin.Amount}) - store.Set(types.TotalEscrowForDenomKey(coin.Denom), bz) + store.Set(key, bz) } // GetAllTotalEscrowed returns the escrow information for all the denominations. func (k Keeper) GetAllTotalEscrowed(ctx sdk.Context) sdk.Coins { var escrows sdk.Coins k.IterateTokensInEscrow(ctx, []byte(types.KeyTotalEscrowPrefix), func(denomEscrow sdk.Coin) bool { - escrows = append(escrows, denomEscrow) + escrows = escrows.Add(denomEscrow) return false }) @@ -229,12 +238,7 @@ func (k Keeper) IterateTokensInEscrow(ctx sdk.Context, prefix []byte, cb func(de defer sdk.LogDeferred(ctx.Logger(), func() error { return iterator.Close() }) for ; iterator.Valid(); iterator.Next() { - keySplit := strings.Split(string(iterator.Key()), "/") - if len(keySplit) < 2 { - continue // key doesn't conform to expected format - } - - denom := strings.Join(keySplit[1:], "/") + denom := strings.TrimPrefix(string(iterator.Key()), fmt.Sprintf("%s/", types.KeyTotalEscrowPrefix)) if strings.TrimSpace(denom) == "" { continue // denom is empty } diff --git a/modules/apps/transfer/keeper/keeper_test.go b/modules/apps/transfer/keeper/keeper_test.go index c5bd09ebd4e..8e5dfc8b883 100644 --- a/modules/apps/transfer/keeper/keeper_test.go +++ b/modules/apps/transfer/keeper/keeper_test.go @@ -60,7 +60,7 @@ func (suite *KeeperTestSuite) TestSetGetTotalEscrowForDenom() { expPass bool }{ { - "success: with 0 escrow amount", + "success: with non-zero escrow amount", func() {}, true, }, @@ -71,6 +71,13 @@ func (suite *KeeperTestSuite) TestSetGetTotalEscrowForDenom() { }, true, }, + { + "success: escrow amount 0 is not stored", + func() { + expAmount = math.ZeroInt() + }, + true, + }, { "failure: setter panics with negative escrow amount", func() { @@ -85,7 +92,7 @@ func (suite *KeeperTestSuite) TestSetGetTotalEscrowForDenom() { suite.Run(tc.name, func() { suite.SetupTest() // reset - expAmount = math.ZeroInt() + expAmount = math.NewInt(100) ctx := suite.chainA.GetContext() tc.malleate() @@ -94,6 +101,15 @@ func (suite *KeeperTestSuite) TestSetGetTotalEscrowForDenom() { suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(denom, expAmount)) total := suite.chainA.GetSimApp().TransferKeeper.GetTotalEscrowForDenom(ctx, denom) suite.Require().Equal(expAmount, total.Amount) + + storeKey := suite.chainA.GetSimApp().GetKey(types.ModuleName) + store := ctx.KVStore(storeKey) + key := types.TotalEscrowForDenomKey(denom) + if expAmount.IsZero() { + suite.Require().False(store.Has(key)) + } else { + suite.Require().True(store.Has(key)) + } } else { suite.Require().PanicsWithError("negative coin amount: -1", func() { suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(denom, expAmount)) diff --git a/modules/apps/transfer/keeper/relay_test.go b/modules/apps/transfer/keeper/relay_test.go index c59c7326979..1239e61fc07 100644 --- a/modules/apps/transfer/keeper/relay_test.go +++ b/modules/apps/transfer/keeper/relay_test.go @@ -384,8 +384,10 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { err = suite.chainB.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainB.GetContext(), packet, data) // check total amount in escrow of received token denom on receiving chain - var denom string - var totalEscrow sdk.Coin + var ( + denom string + totalEscrow sdk.Coin + ) if tc.recvIsSource { denom = sdk.DefaultBondDenom } else { diff --git a/modules/apps/transfer/module.go b/modules/apps/transfer/module.go index 310d1fd5eca..8b23fa66da2 100644 --- a/modules/apps/transfer/module.go +++ b/modules/apps/transfer/module.go @@ -94,8 +94,8 @@ func NewAppModule(k keeper.Keeper) AppModule { } // RegisterInvariants implements the AppModule interface -func (AppModule) RegisterInvariants(ir sdk.InvariantRegistry) { - // TODO +func (am AppModule) RegisterInvariants(ir sdk.InvariantRegistry) { + keeper.RegisterInvariants(ir, &am.keeper) } // RegisterServices registers module services. diff --git a/proto/ibc/applications/transfer/v1/query.proto b/proto/ibc/applications/transfer/v1/query.proto index 9a41aa847b3..ff56cc30c8d 100644 --- a/proto/ibc/applications/transfer/v1/query.proto +++ b/proto/ibc/applications/transfer/v1/query.proto @@ -114,6 +114,7 @@ message QueryEscrowAddressResponse { message QueryTotalEscrowForDenomRequest { string denom = 1; } + // QueryTotalEscrowForDenomResponse is the response type for TotalEscrowForDenom RPC method. message QueryTotalEscrowForDenomResponse { cosmos.base.v1beta1.Coin amount = 1 [(gogoproto.nullable) = false];