x/ibc-transfer: ADR001 source tracing implementation (#6871)

* x/ibc-transfer: ADR001 source tracing implementation * gRPC proto file * validation * fix validation * import export genesis * relay.go updates * gRPC service methods * client CLI * update implementation * build * trace test * fix CLI tx args * genesis import/export tests * update comments * update proto files * GRPC tests * remove field from packet * fix coin validation bug * more validations * update comments * minor refactor * update relay.go * try fix test * minor updates * fix tests * fix test * ADR updates and comments * build * Apply suggestions from code review Co-authored-by: Aditya <adityasripal@gmail.com> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * address a few comments from review * gRPC annotations * update proto files * Apply suggestions from code review Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * address comments * docs and changelog * sort traces * final changes to ADR * client support for full path denom prefixes * address @AdityaSripal comments * address TODO * increase test timeouts Co-authored-by: Aditya <adityasripal@gmail.com> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
cosmos · Aug 14, 2020 · 3022fe9 · 3022fe9
1 parent 8de96d1
commit 3022fe9
Show file tree

Hide file tree

Showing 43 changed files with 2,788 additions and 235 deletions.
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -44,7 +44,7 @@ jobs:
   test-coverage-run-1:
     runs-on: ubuntu-latest
     needs: split-test-files
-    timeout-minutes: 10
+    timeout-minutes: 15
     steps:
       - uses: actions/checkout@v2
       - uses: technote-space/get-diff-action@v3
@@ -60,7 +60,7 @@ jobs:
         if: "env.GIT_DIFF != ''"
       - name: test & coverage report creation
         run: |
-          cat xaa.txt | xargs go test -mod=readonly -timeout 8m -coverprofile=coverage.txt -covermode=atomic -tags='ledger test_ledger_mock'
+          cat xaa.txt | xargs go test -mod=readonly -timeout 15m -coverprofile=coverage.txt -covermode=atomic -tags='ledger test_ledger_mock'
         if: "env.GIT_DIFF != ''"
       - name: filter out DONTCOVER
         run: |
@@ -81,7 +81,7 @@ jobs:
   test-coverage-run-2:
     runs-on: ubuntu-latest
     needs: split-test-files
-    timeout-minutes: 10
+    timeout-minutes: 15
     steps:
       - uses: actions/checkout@v2
       - uses: technote-space/get-diff-action@v3
@@ -97,7 +97,7 @@ jobs:
         if: "env.GIT_DIFF != ''"
       - name: test & coverage report creation
         run: |
-          cat xab.txt | xargs go test -mod=readonly -timeout 6m -coverprofile=coverage.txt -covermode=atomic -tags='ledger test_ledger_mock'
+          cat xab.txt | xargs go test -mod=readonly -timeout 15m -coverprofile=coverage.txt -covermode=atomic -tags='ledger test_ledger_mock'
         if: "env.GIT_DIFF != ''"
       - name: filter out DONTCOVER
         run: |
@@ -118,7 +118,7 @@ jobs:
   test-coverage-run-3:
     runs-on: ubuntu-latest
     needs: split-test-files
-    timeout-minutes: 10
+    timeout-minutes: 15
     steps:
       - uses: actions/checkout@v2
       - uses: technote-space/get-diff-action@v3
@@ -134,7 +134,7 @@ jobs:
         if: "env.GIT_DIFF != ''"
       - name: test & coverage report creation
         run: |
-          cat xac.txt | xargs go test -mod=readonly -timeout 6m -coverprofile=coverage.txt -covermode=atomic -tags='ledger test_ledger_mock'
+          cat xac.txt | xargs go test -mod=readonly -timeout 15m -coverprofile=coverage.txt -covermode=atomic -tags='ledger test_ledger_mock'
         if: "env.GIT_DIFF != ''"
       - name: filter out DONTCOVER
         run: |
@@ -155,7 +155,7 @@ jobs:
   test-coverage-run-4:
     runs-on: ubuntu-latest
     needs: split-test-files
-    timeout-minutes: 10
+    timeout-minutes: 15
     steps:
       - uses: actions/checkout@v2
       - uses: technote-space/get-diff-action@v3
@@ -171,7 +171,7 @@ jobs:
         if: "env.GIT_DIFF != ''"
       - name: test & coverage report creation
         run: |
-          cat xad.txt | xargs go test -mod=readonly -timeout 6m -coverprofile=coverage.txt -covermode=atomic -tags='ledger test_ledger_mock'
+          cat xad.txt | xargs go test -mod=readonly -timeout 15m -coverprofile=coverage.txt -covermode=atomic -tags='ledger test_ledger_mock'
         if: "env.GIT_DIFF != ''"
       - name: filter out DONTCOVER
         run: |

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -283,9 +283,10 @@ Buffers for state serialization instead of Amino.
 
 ### Improvements
 
+* (x/ibc-transfer) [\#6871](https://github.com/cosmos/cosmos-sdk/pull/6871) Implement [ADR 001 - Coin Source Tracing](./docs/architecture/adr-001-coin-source-tracing.md).
 * (types) [\#7027](https://github.com/cosmos/cosmos-sdk/pull/7027) `Coin(s)` and `DecCoin(s)` updates:
   * Bump denomination max length to 128
-  * Allow unicode letters and numbers in denominations
+  * Allow uppercase letters and numbers in denominations to support [ADR 001](./docs/architecture/adr-001-coin-source-tracing.md)
   * Added `Validate` function that returns a descriptive error
 * (baseapp) [\#6186](https://github.com/cosmos/cosmos-sdk/issues/6186) Support emitting events during `AnteHandler` execution.
 * (x/auth) [\#5702](https://github.com/cosmos/cosmos-sdk/pull/5702) Add parameter querying support for `x/auth`.

diff --git a/docs/architecture/adr-001-coin-source-tracing.md b/docs/architecture/adr-001-coin-source-tracing.md
@@ -3,10 +3,11 @@
 ## Changelog
 
 - 2020-07-09: Initial Draft
+- 2020-08-11: Implementation changes
 
 ## Status
 
-Proposed
+Accepted, Implemented
 
 ## Context
 
@@ -114,7 +115,7 @@ trace the token back to the originating chain, as specified on ICS20.
 The new proposed format will be the following:
 
 ```golang
-ibcDenom = "ibc/" + hash(trace + "/" + base denom)
+ibcDenom = "ibc/" + hash(trace path + "/" + base denom)
 ```
 
 The hash function will be a SHA256 hash of the fields of the `DenomTrace`:
@@ -124,7 +125,7 @@ The hash function will be a SHA256 hash of the fields of the `DenomTrace`:
 // information
 message DenomTrace {
   // chain of port/channel identifiers used for tracing the source of the fungible token
-  string trace = 1;
+  string path = 1;
   // base denomination of the relayed fungible token
   string base_denom = 2;
 }
@@ -133,50 +134,23 @@ message DenomTrace {
 The `IBCDenom` function constructs the `Coin` denomination used when creating the ICS20 fungible token packet data:
 
 ```golang
-// Hash returns the hex bytes of the SHA256 hash of the DenomTrace fields.
+// Hash returns the hex bytes of the SHA256 hash of the DenomTrace fields using the following formula:
+//
+// hash = sha256(tracePath + "/" + baseDenom)
 func (dt DenomTrace) Hash() tmbytes.HexBytes {
-  return tmhash.Sum(dt.Trace + "/" + dt.BaseDenom)
+  return tmhash.Sum(dt.Path + "/" + dt.BaseDenom)
 }
 
-// IBCDenom a coin denomination for an ICS20 fungible token in the format 'ibc/{hash(trace + baseDenom)}'. If the trace is empty, it will return the base denomination.
+// IBCDenom a coin denomination for an ICS20 fungible token in the format 'ibc/{hash(tracePath + baseDenom)}'. 
+// If the trace is empty, it will return the base denomination.
 func (dt DenomTrace) IBCDenom() string {
-  if dt.Trace != "" {
+  if dt.Path != "" {
     return fmt.Sprintf("ibc/%s", dt.Hash())
   }
   return dt.BaseDenom
 }
 ```
 
-In order to trim the denomination trace prefix when sending/receiving fungible tokens, the `RemovePrefix` function is provided.
-
-> NOTE: the prefix addition must be done on the client side.
-
-```golang
-// RemovePrefix trims the first portID/channelID pair from the trace info. If the trace is already empty it will perform a no-op. If the trace is incorrectly constructed or doesn't have separators it will return an error.
-func (dt *DenomTrace) RemovePrefix() error {
-  if dt.Trace == "" {
-    return nil
-  }
-
-  traceSplit := strings.SplitN(dt.Trace, "/", 3)
-
-  var err error
-  switch {
-  // NOTE: other cases are checked during msg validation
-  case len(traceSplit) == 2:
-    dt.Trace = ""
-  case len(traceSplit) == 3:
-    dt.Trace = traceSplit[2]
-  }
-
-  if err != nil {
-    return err
-  }
-
-  return nil
-}
-```
-
 ### `x/ibc-transfer` Changes
 
 In order to retrieve the trace information from an IBC denomination, a lookup table needs to be
@@ -217,44 +191,30 @@ hash, if the trace info is provided, or that the base denominations matches:
 ```golang
 func (msg MsgTransfer) ValidateBasic() error {
   // ...
-  if err := msg.Trace.Validate(); err != nil {
-    return err
-  }
-  if err := ValidateIBCDenom(msg.Token.Denom, msg.Trace); err != nil {
-    return err
-  }
-  // ...
+  return ValidateIBCDenom(msg.Token.Denom)
 }
 ```
 
 ```golang
-// ValidateIBCDenom checks that the denomination for an IBC fungible token is valid. It returns error if the trace `hash` is invalid
-func ValidateIBCDenom(denom string, trace DenomTrace) error {
-  // Validate that base denominations are equal if the trace info is not provided
-  if trace.Trace == "" {
-    if trace.BaseDenom != denom {
-      return Wrapf(
-        ErrInvalidDenomForTransfer,
-        "token denom must match the trace base denom (%s ≠ %s)",
-        denom, trace.BaseDenom,
-      )
-    }
-    return nil
-  }
-
+// ValidateIBCDenom validates that the given denomination is either:
+//
+//  - A valid base denomination (eg: 'uatom')
+//  - A valid fungible token representation (i.e 'ibc/{hash}') per ADR 001 https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-001-coin-source-tracing.md
+func ValidateIBCDenom(denom string) error {
   denomSplit := strings.SplitN(denom, "/", 2)
 
   switch {
-    case denomSplit[0] != "ibc":
-      return Wrapf(ErrInvalidDenomForTransfer, "denomination should be prefixed with the format 'ibc/{hash(trace + \"/\" + %s)}'", denom)
-    case len(denomSplit) == 2:
-      return tmtypes.ValidateHash([]byte(denomSplit[1]))
+  case strings.TrimSpace(denom) == "",
+    len(denomSplit) == 1 && denomSplit[0] == "ibc",
+    len(denomSplit) == 2 && (denomSplit[0] != "ibc" || strings.TrimSpace(denomSplit[1]) == ""):
+    return sdkerrors.Wrapf(ErrInvalidDenomForTransfer, "denomination should be prefixed with the format 'ibc/{hash(trace + \"/\" + %s)}'", denom)
+
+  case denomSplit[0] == denom && strings.TrimSpace(denom) != "":
+    return sdk.ValidateDenom(denom)
   }
 
-  denomTraceHash := denomSplit[1]
-  traceHash := trace.Hash()
-  if !bytes.Equal(traceHash.Bytes(), denomTraceHash.Bytes()) {
-    return Errorf("token denomination trace hash mismatch, expected %s got %s", traceHash, denomTraceHash)
+  if _, err := ParseHexHash(denomSplit[1]); err != nil {
+    return Wrapf(err, "invalid denom trace hash %s", denomSplit[1])
   }
 
   return nil
@@ -266,6 +226,46 @@ The denomination trace info only needs to be updated when token is received:
 - Receiver is **source** chain: The receiver created the token and must have the trace lookup already stored (if necessary _ie_ native token case wouldn't need a lookup).
 - Receiver is **not source** chain: Store the received info. For example, during step 1, when chain `B` receives `transfer/channelToA/denom`.
 
+```golang
+// SendTransfer
+// ...
+
+  fullDenomPath := token.Denom
+
+// deconstruct the token denomination into the denomination trace info
+// to determine if the sender is the source chain
+if strings.HasPrefix(token.Denom, "ibc/") {
+  fullDenomPath, err = k.DenomPathFromHash(ctx, token.Denom)
+  if err != nil {
+    return err
+  }
+}
+
+if types.SenderChainIsSource(sourcePort, sourceChannel, fullDenomPath) {
+//...
+```
+
+```golang
+// DenomPathFromHash returns the full denomination path prefix from an ibc denom with a hash
+// component.
+func (k Keeper) DenomPathFromHash(ctx sdk.Context, denom string) (string, error) {
+  hexHash := denom[4:]
+  hash, err := ParseHexHash(hexHash)
+  if err != nil {
+    return "", Wrap(ErrInvalidDenomForTransfer, err.Error())
+  }
+
+  denomTrace, found := k.GetDenomTrace(ctx, hash)
+  if !found {
+    return "", Wrap(ErrTraceNotFound, hexHash)
+  }
+
+  fullDenomPath := denomTrace.GetFullDenomPath()
+  return fullDenomPath, nil
+}
+```
+
+
 ```golang
 // OnRecvPacket
 // ...
@@ -277,19 +277,13 @@ The denomination trace info only needs to be updated when token is received:
 // 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.
-voucherPrefix := GetDenomPrefix(packet.GetSourcePort(), packet.GetSourceChannel())
-
 if ReceiverChainIsSource(packet.GetSourcePort(), packet.GetSourceChannel(), data.Denom) {
   // sender chain is not the source, unescrow tokens
 
   // remove prefix added by sender chain
-  if err := denomTrace.RemovePrefix(); err != nil {
-    return err
-  }
-
-  // NOTE: since the sender is a sink chain, we already know the unprefixed denomination trace info
-
-  token := sdk.NewCoin(denomTrace.IBCDenom(), sdk.NewIntFromUint64(data.Amount))
+  voucherPrefix := types.GetDenomPrefix(packet.GetSourcePort(), packet.GetSourceChannel())
+  unprefixedDenom := data.Denom[len(voucherPrefix):]
+  token := sdk.NewCoin(unprefixedDenom, sdk.NewIntFromUint64(data.Amount))
 
   // unescrow tokens
   escrowAddress := types.GetEscrowAddress(packet.GetDestPort(), packet.GetDestChannel())
@@ -298,19 +292,22 @@ if ReceiverChainIsSource(packet.GetSourcePort(), packet.GetSourceChannel(), data
 
 // sender chain is the source, mint vouchers
 
-// construct the denomination trace from the full raw denomination
-denomTrace := NewDenomTraceFromRawDenom(data.Denom)
+// since SendPacket did not prefix the denomination, we must prefix denomination here
+sourcePrefix := types.GetDenomPrefix(packet.GetDestPort(), packet.GetDestChannel())
+// NOTE: sourcePrefix contains the trailing "/"
+prefixedDenom := sourcePrefix + data.Denom
 
-// since SendPacket did not prefix the denomination with the voucherPrefix, we must add it here
-denomTrace.AddPrefix(packet.GetDestPort(), packet.GetDestChannel())
+// construct the denomination trace from the full raw denomination
+denomTrace := types.ParseDenomTrace(prefixedDenom)
 
 // set the value to the lookup table if not stored already
 traceHash := denomTrace.Hash()
 if !k.HasDenomTrace(ctx, traceHash) {
   k.SetDenomTrace(ctx, traceHash, denomTrace)
 }
 
-voucher := sdk.NewCoin(denomTrace.IBCDenom(), sdk.NewIntFromUint64(data.Amount))
+voucherDenom := denomTrace.IBCDenom()
+voucher := sdk.NewCoin(voucherDenom, sdk.NewIntFromUint64(data.Amount))
 
 // mint new tokens if the source of the transfer is the same chain
 if err := k.bankKeeper.MintCoins(
@@ -347,7 +344,8 @@ The coin denomination validation will need to be updated to reflect these change
 function will now:
 
 - Accept slash separators (`"/"`) and uppercase characters (due to the `HexBytes` format)
-- Bump the maximum character length to 64
+- Bump the maximum character length to 128, as the hex representation used by Tendermint's
+  `HexBytes` type contains 64 characters.
 
 Additional validation logic, such as verifying the length of the hash, the  may be added to the bank module in the future if the [custom base denomination validation](https://github.com/cosmos/cosmos-sdk/pull/6755) is integrated into the SDK.
 

diff --git a/docs/architecture/adr-template.md b/docs/architecture/adr-template.md
@@ -7,7 +7,7 @@
 ## Status
 
 > A decision may be "proposed" if the project stakeholders haven't agreed with it yet, or "accepted" once it is agreed. If a later ADR changes or reverses a decision, it may be marked as "deprecated" or "superseded" with a reference to its replacement.
-> {Deprecated|Proposed|Accepted}
+> {Deprecated|Proposed|Accepted} {Implemented|Not Implemented}
 
 ## Context
 

diff --git a/proto/ibc/transfer/genesis.proto b/proto/ibc/transfer/genesis.proto
@@ -4,11 +4,16 @@ package ibc.transfer;
 option go_package = "github.com/cosmos/cosmos-sdk/x/ibc-transfer/types";
 
 import "gogoproto/gogo.proto";
+import "ibc/transfer/transfer.proto";
 
-// GenesisState is currently only used to ensure that the InitGenesis gets run
-// by the module manager
-message GenesisState {
-  string port_id = 1 [
-    (gogoproto.moretags)   = "yaml:\"port_id\""
-  ];
+// GenesisState defines the ibc-transfer genesis state
+message GenesisState{
+	string port_id  = 1 [
+	  (gogoproto.moretags) = "yaml:\"port_id\""
+	];
+	repeated DenomTrace denom_traces = 2 [
+		(gogoproto.castrepeated) = "Traces",
+		(gogoproto.nullable) = false,
+		(gogoproto.moretags) = "yaml:\"denom_traces\""
+	];
 }