imp: transfer total escrow follow ups (cosmos#3558)

* transfer (total escrow): add documentation and migration docs (cosmos#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 (cosmos#3519) * some more review comments * Rename pathAndEscrowAMount to pathAndEscrowAmount --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> * transfer (total escrow): add invariant for total escrow (cosmos#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 <damiannolan@gmail.com> --------- Co-authored-by: Colin Axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> * imp: do not store total escrow when amount is zero (cosmos#3585) * do not store 0 escrow amout * adapt success test * Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com> * Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Damian Nolan <damiannolan@gmail.com> * add comments --------- Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> * add feature release for total escrow state entry to conditionally query the endpoint in e2e (cosmos#3605) * fix total escrow cli documentation * Apply suggestions from code review Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> * address review comments * docs: adr 011 total escrow state entry (cosmos#3641) * docs: add adr 011 for total escrow state entry * indentation * code formatting * Apply suggestions from code review Co-authored-by: Damian Nolan <damiannolan@gmail.com> * Apply suggestions from code review Co-authored-by: Charly <charly@interchain.io> --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: Charly <charly@interchain.io> --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: Charly <charly@interchain.io>
faddat · Jun 1, 2023 · 6f628d9 · 6f628d9
1 parent 750134a
commit 6f628d9
Show file tree

Hide file tree

Showing 17 changed files with 405 additions and 30 deletions.
diff --git 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
@@ -1,3 +1,7 @@
+<!--
+order: 8
+-->
+
 # `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
@@ -0,0 +1,66 @@
+<!--
+order: 9
+-->
+
+# 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"
+}
+```
diff --git 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
@@ -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)
diff --git 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.
 
 <!-- TODO: Update the links to use release version instead of feat branch -->
-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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
+	}
+}