Change ICS 20 packet data amount to be string (cosmos#350)

* modify proto defintions * fix non string usage in code and various tests * fix mbt tests * fix bug in data validation * fix various build issues fix unaddressed issues from changing amount from uint64 to string * add changelog entry * apply review suggestions Add check that amount is strictly positive Construct granular error messages to indicate invalid amount value or failure to parse amount * verify and fix telemetry bug Verify msg panics on amounts > int64 by adding tests Add checks to telemetry emission of transfer amounts to handle when the amount cannot be casted to float32
decentrio · Sep 10, 2021 · 06c2d76 · 06c2d76
1 parent a647546
commit 06c2d76
Show file tree

Hide file tree

Showing 29 changed files with 687 additions and 463 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -39,6 +39,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 ### API Breaking
 
 * (core) [\#227](https://github.com/cosmos/ibc-go/pull/227) Remove sdk.Result from application callbacks
+* (transfer) [\#350](https://github.com/cosmos/ibc-go/pull/350) Change FungibleTokenPacketData to use a string for the Amount field. This enables token transfers with amounts previously restricted by uint64. Up to the maximum uint256 value is supported. 
 
 ### State Machine Breaking
 

diff --git a/docs/ibc/proto-docs.md b/docs/ibc/proto-docs.md
@@ -6,7 +6,6 @@
 
 - [ibc/applications/transfer/v1/transfer.proto](#ibc/applications/transfer/v1/transfer.proto)
     - [DenomTrace](#ibc.applications.transfer.v1.DenomTrace)
-    - [FungibleTokenPacketData](#ibc.applications.transfer.v1.FungibleTokenPacketData)
     - [Params](#ibc.applications.transfer.v1.Params)
 
 - [ibc/applications/transfer/v1/genesis.proto](#ibc/applications/transfer/v1/genesis.proto)
@@ -37,6 +36,9 @@
 
     - [Msg](#ibc.applications.transfer.v1.Msg)
 
+- [ibc/applications/transfer/v2/packet.proto](#ibc/applications/transfer/v2/packet.proto)
+    - [FungibleTokenPacketData](#ibc.applications.transfer.v2.FungibleTokenPacketData)
+
 - [ibc/core/channel/v1/channel.proto](#ibc/core/channel/v1/channel.proto)
     - [Acknowledgement](#ibc.core.channel.v1.Acknowledgement)
     - [Channel](#ibc.core.channel.v1.Channel)
@@ -270,26 +272,6 @@ source tracing information path.
 
 
 
-<a name="ibc.applications.transfer.v1.FungibleTokenPacketData"></a>
-
-### FungibleTokenPacketData
-FungibleTokenPacketData defines a struct for the packet payload
-See FungibleTokenPacketData spec:
-https://github.com/cosmos/ics/tree/master/spec/ics-020-fungible-token-transfer#data-structures
-
-
-| Field | Type | Label | Description |
-| ----- | ---- | ----- | ----------- |
-| `denom` | [string](#string) |  | the token denomination to be transferred |
-| `amount` | [uint64](#uint64) |  | the token amount to be transferred |
-| `sender` | [string](#string) |  | the sender address |
-| `receiver` | [string](#string) |  | the recipient address on the destination chain |
-
-
-
-
-
-
 <a name="ibc.applications.transfer.v1.Params"></a>
 
 ### Params
@@ -675,6 +657,42 @@ Msg defines the ibc/transfer Msg service.
 
 
 
+<a name="ibc/applications/transfer/v2/packet.proto"></a>
+<p align="right"><a href="#top">Top</a></p>
+
+## ibc/applications/transfer/v2/packet.proto
+
+
+
+<a name="ibc.applications.transfer.v2.FungibleTokenPacketData"></a>
+
+### FungibleTokenPacketData
+FungibleTokenPacketData defines a struct for the packet payload
+See FungibleTokenPacketData spec:
+https://github.com/cosmos/ics/tree/master/spec/ics-020-fungible-token-transfer#data-structures
+
+
+| Field | Type | Label | Description |
+| ----- | ---- | ----- | ----------- |
+| `denom` | [string](#string) |  | the token denomination to be transferred |
+| `amount` | [string](#string) |  | the token amount to be transferred |
+| `sender` | [string](#string) |  | the sender address |
+| `receiver` | [string](#string) |  | the recipient address on the destination chain |
+
+
+
+
+
+ <!-- end messages -->
+
+ <!-- end enums -->
+
+ <!-- end HasExtensions -->
+
+ <!-- end services -->
+
+
+
 <a name="ibc/core/channel/v1/channel.proto"></a>
 <p align="right"><a href="#top">Top</a></p>
 

diff --git a/modules/apps/transfer/keeper/MBT_README.md b/modules/apps/transfer/keeper/MBT_README.md
@@ -48,4 +48,4 @@ docker run --rm -v $(pwd):/var/apalache apalache/mc $@
 ```    
 
 
-In case of any questions please don't hesitate to contact Andrey Kuprianov ([email protected]).
+In case of any questions please don't hesitate to contact Andrey Kuprianov ([email protected]).
diff --git a/modules/apps/transfer/keeper/mbt_relay_test.go b/modules/apps/transfer/keeper/mbt_relay_test.go
@@ -30,7 +30,7 @@ type TlaBalance struct {
 type TlaFungibleTokenPacketData struct {
 	Sender   string   `json:"sender"`
 	Receiver string   `json:"receiver"`
-	Amount   int      `json:"amount"`
+	Amount   string   `json:"amount"`
 	Denom    []string `json:"denom"`
 }
 
@@ -143,7 +143,7 @@ func FungibleTokenPacketFromTla(packet TlaFungibleTokenPacket) FungibleTokenPack
 		DestPort:      packet.DestPort,
 		Data: types.NewFungibleTokenPacketData(
 			DenomFromTla(packet.Data.Denom),
-			uint64(packet.Data.Amount),
+			packet.Data.Amount,
 			AddressFromString(packet.Data.Sender),
 			AddressFromString(packet.Data.Receiver)),
 	}
@@ -333,11 +333,15 @@ func (suite *KeeperTestSuite) TestModelBasedRelay() {
 					denom := denomTrace.IBCDenom()
 					err = sdk.ValidateDenom(denom)
 					if err == nil {
+						amount, ok := sdk.NewIntFromString(tc.packet.Data.Amount)
+						if !ok {
+							panic("MBT failed to parse amount from string")
+						}
 						err = suite.chainB.GetSimApp().TransferKeeper.SendTransfer(
 							suite.chainB.GetContext(),
 							tc.packet.SourcePort,
 							tc.packet.SourceChannel,
-							sdk.NewCoin(denom, sdk.NewIntFromUint64(tc.packet.Data.Amount)),
+							sdk.NewCoin(denom, amount),
 							sender,
 							tc.packet.Data.Receiver,
 							clienttypes.NewHeight(0, 110),

diff --git a/modules/apps/transfer/keeper/model_based_tests/Test5Packets.json b/modules/apps/transfer/keeper/model_based_tests/Test5Packets.json
@@ -8,7 +8,7 @@
       "data": {
         "sender": "a3",
         "receiver": "a3",
-        "amount": 2,
+        "amount": "2",
         "denom": [
           "",
           "",
@@ -79,7 +79,7 @@
       "data": {
         "sender": "a1",
         "receiver": "a3",
-        "amount": 1,
+        "amount": "1",
         "denom": [
           "cosmos-hub",
           "",
@@ -165,7 +165,7 @@
       "data": {
         "sender": "a2",
         "receiver": "a2",
-        "amount": 4,
+        "amount": "4",
         "denom": [
           "",
           "",
@@ -266,7 +266,7 @@
       "data": {
         "sender": "",
         "receiver": "a2",
-        "amount": 4,
+        "amount": "4",
         "denom": [
           "",
           "",
@@ -382,7 +382,7 @@
       "data": {
         "sender": "a1",
         "receiver": "",
-        "amount": 1,
+        "amount": "1",
         "denom": [
           "transfer",
           "channel-0",
@@ -489,4 +489,4 @@
     ],
     "error": true
   }
-]
+]
diff --git a/modules/apps/transfer/keeper/model_based_tests/Test5PacketsAllDifferentPass.json b/modules/apps/transfer/keeper/model_based_tests/Test5PacketsAllDifferentPass.json
@@ -8,7 +8,7 @@
       "data": {
         "sender": "a3",
         "receiver": "a2",
-        "amount": 3,
+        "amount": "3",
         "denom": [
           "",
           "",
@@ -79,7 +79,7 @@
       "data": {
         "sender": "a2",
         "receiver": "a1",
-        "amount": 3,
+        "amount": "3",
         "denom": [
           "transfer",
           "channel-1",
@@ -180,7 +180,7 @@
       "data": {
         "sender": "a1",
         "receiver": "a2",
-        "amount": 3,
+        "amount": "3",
         "denom": [
           "",
           "",
@@ -311,7 +311,7 @@
       "data": {
         "sender": "a1",
         "receiver": "",
-        "amount": 2,
+        "amount": "2",
         "denom": [
           "",
           "channel-0",
@@ -457,7 +457,7 @@
       "data": {
         "sender": "a3",
         "receiver": "a3",
-        "amount": 1,
+        "amount": "1",
         "denom": [
           "",
           "",
@@ -609,4 +609,4 @@
     ],
     "error": false
   }
-]
+]
diff --git a/modules/apps/transfer/keeper/model_based_tests/TestOnRecvAcknowledgementErrorFail.json b/modules/apps/transfer/keeper/model_based_tests/TestOnRecvAcknowledgementErrorFail.json
@@ -8,7 +8,7 @@
       "data": {
         "sender": "a1",
         "receiver": "a2",
-        "amount": 1,
+        "amount": "1",
         "denom": [
           "cosmos-hub",
           "transfer",
@@ -55,4 +55,4 @@
     ],
     "error": true
   }
-]
+]
diff --git a/modules/apps/transfer/keeper/model_based_tests/TestOnRecvAcknowledgementErrorPass.json b/modules/apps/transfer/keeper/model_based_tests/TestOnRecvAcknowledgementErrorPass.json
@@ -8,7 +8,7 @@
       "data": {
         "sender": "",
         "receiver": "a1",
-        "amount": 1,
+        "amount": "1",
         "denom": [
           "",
           "",
@@ -79,7 +79,7 @@
       "data": {
         "sender": "a1",
         "receiver": "a2",
-        "amount": 1,
+        "amount": "1",
         "denom": [
           "transfer",
           "channel-1",
@@ -156,4 +156,4 @@
     ],
     "error": false
   }
-]
+]
diff --git a/modules/apps/transfer/keeper/model_based_tests/TestOnRecvAcknowledgementResultFail.json b/modules/apps/transfer/keeper/model_based_tests/TestOnRecvAcknowledgementResultFail.json
@@ -8,7 +8,7 @@
       "data": {
         "sender": "a1",
         "receiver": "a2",
-        "amount": 1,
+        "amount": "1",
         "denom": [
           "cosmos-hub",
           "transfer",
@@ -55,4 +55,4 @@
     ],
     "error": true
   }
-]
+]
diff --git a/modules/apps/transfer/keeper/model_based_tests/TestOnRecvAcknowledgementResultPass.json b/modules/apps/transfer/keeper/model_based_tests/TestOnRecvAcknowledgementResultPass.json
@@ -8,7 +8,7 @@
       "data": {
         "sender": "a1",
         "receiver": "a2",
-        "amount": 1,
+        "amount": "1",
         "denom": [
           "cosmos-hub",
           "transfer",
@@ -55,4 +55,4 @@
     ],
     "error": false
   }
-]
+]
diff --git a/modules/apps/transfer/keeper/model_based_tests/TestOnRecvPacketFail.json b/modules/apps/transfer/keeper/model_based_tests/TestOnRecvPacketFail.json
@@ -8,7 +8,7 @@
       "data": {
         "sender": "",
         "receiver": "",
-        "amount": 1,
+        "amount": "1",
         "denom": [
           "",
           "",
@@ -55,4 +55,4 @@
     ],
     "error": true
   }
-]
+]
diff --git a/modules/apps/transfer/keeper/model_based_tests/TestOnRecvPacketPass.json b/modules/apps/transfer/keeper/model_based_tests/TestOnRecvPacketPass.json
@@ -8,7 +8,7 @@
       "data": {
         "sender": "",
         "receiver": "a2",
-        "amount": 1,
+        "amount": "1",
         "denom": [
           "",
           "",
@@ -70,4 +70,4 @@
     ],
     "error": false
   }
-]
+]
diff --git a/modules/apps/transfer/keeper/model_based_tests/TestOnTimeoutFail.json b/modules/apps/transfer/keeper/model_based_tests/TestOnTimeoutFail.json
@@ -8,7 +8,7 @@
       "data": {
         "sender": "a1",
         "receiver": "a2",
-        "amount": 1,
+        "amount": "1",
         "denom": [
           "cosmos-hub",
           "transfer",
@@ -55,4 +55,4 @@
     ],
     "error": true
   }
-]
+]
diff --git a/modules/apps/transfer/keeper/model_based_tests/TestOnTimeoutPass.json b/modules/apps/transfer/keeper/model_based_tests/TestOnTimeoutPass.json
@@ -8,7 +8,7 @@
       "data": {
         "sender": "a3",
         "receiver": "a1",
-        "amount": 1,
+        "amount": "1",
         "denom": [
           "",
           "",
@@ -79,7 +79,7 @@
       "data": {
         "sender": "a1",
         "receiver": "",
-        "amount": 1,
+        "amount": "1",
         "denom": [
           "transfer",
           "channel-1",
@@ -156,4 +156,4 @@
     ],
     "error": false
   }
-]
+]