From e5b3d8aa74ae99c9a4f2b960da881608c597df61 Mon Sep 17 00:00:00 2001
From: Thomas Nguy <81727899+thomas-nguy@users.noreply.github.com>
Date: Tue, 2 Aug 2022 18:00:24 +0900
Subject: [PATCH] Problem: fix mistake on acknowledgement error in ibc
 middleware (#611)

* fix mistake on ibc middleware

* add changelog
---
 CHANGELOG.md                                 |  1 +
 docs/architecture/adr-009.md                 | 96 ++++++++++++++++++++
 x/cronos/middleware/conversion_middleware.go |  2 +-
 3 files changed, 98 insertions(+), 1 deletion(-)
 create mode 100644 docs/architecture/adr-009.md

diff --git a/CHANGELOG.md b/CHANGELOG.md
index a765291aea..4d8b0c0ebc 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,7 @@
 - [cronos#429](https://github.com/crypto-org-chain/cronos/pull/429) Update ethermint to main, ibc-go to v3.0.0, cosmos sdk to v0.45.4 and gravity to latest, remove v0.7.0 related upgradeHandler.
 - [cronos#532](https://github.com/crypto-org-chain/cronos/pull/532) Add SendtoChain and CancelSendToChain support from evm call.
 - [cronos#600](https://github.com/crypto-org-chain/cronos/pull/600) Implement bidirectional token mapping.
+- [cronos#611](https://github.com/crypto-org-chain/cronos/pull/611) Fix mistake on acknowledgement error in ibc middleware.
 
 ### Bug Fixes
 
diff --git a/docs/architecture/adr-009.md b/docs/architecture/adr-009.md
new file mode 100644
index 0000000000..4d2b9872ad
--- /dev/null
+++ b/docs/architecture/adr-009.md
@@ -0,0 +1,96 @@
+# ADR 009: Adding extensibility to bridge methods of CRC-21 standard
+
+## Changelog
+* 2022-08-02: first draft
+
+## Context
+
+Following the acceptance of [ADR-008](./adr-008.md), a revision to the token contract standard named CRC-21 is introduced. For existing tokens to enjoy the new bridge capabilities, they need to upgrade or migrate their existing contracts to the CRC-21 standard.
+
+On the other hand, there are some foreseeable bridge changes to be introduced in the coming versions. For example IBC relay fee.
+
+This proposal aims to add certain extensibility to the bridge methods of CRC-21 standard to reduce the need for multiple migrations of existing tokens to support new bridge features in the future. This proposal will also provide guidelines for future bridge updates to leverage the proposed standard.
+
+## Decision
+
+### __CronosSendToChainV2 Event
+
+#### Changes
+
+- Introduce event `__CronosSendToChainV2`, successor of event `__CronosSendToChain`;
+- Add new argument `string channelId` to accept destination channel Id;
+- Add new argument `bytes extraData` to accept extra arguments in the future.
+
+#### Signature
+
+```solidity
+// Current
+event __CronosSendToIbc(address sender, string recipient, uint256 amount);
+
+// Proposal
+event __CronosSendToIbcV2(address sender, string recipient, uint256 amount, string channelId, bytes extraData);
+```
+
+#### Description
+
+- Upgrade the event version for the argument changes
+  - Current event `__CronosSendToIbc` is kept to avoid introducing confusion and avoid extra chain logic to check the emitted event format at runtime.
+- Channel ID argument is added to support sending to specified IBC channel
+  - For token originated from Cronos, this channel ID could be any available IBC channel;
+  - For token originated from IBC channels, the initial implementation can add a restriction to enforce the same channel as the source channel before it is ready.
+
+### __CronosSendToChain Event
+
+#### Changes
+
+- Change `chain_id` to `string` type to accept arbitrary chain ID;
+- Add new argument `extraData bytes` to accept extra arguments in the future.
+
+#### Signature
+
+```solidity
+// Current
+event __CronosSendToChain(address sender, address recipient, uint256 amount, uint256 bridge_fee, uint256 chain_id);
+
+// Proposal
+event __CronosSendToChain(address sender, address recipient, uint256 amount, uint256 bridge_fee, string chain_id, extraData bytes);
+```
+
+#### Description
+
+- Chain ID argument is chagned from unsigned integer to string. This allow sending token to destination chains other than Ethereum (Gravity Bridge).
+  - The Chain ID naming standard is expected to be defined on another ADR.
+
+### Bridge Destination Restriction
+
+- The new event formats add support to arbitrary destination chains according to the capability of the chain.
+- It is up to the contract to define restriction on the destination, if needed.
+
+### Extra Arguments Guidelines
+
+This is a guideline to future bridge features on leveraging the `bytes extraData` arguement:
+
+1. The first 2 bytes are reserved be a version number, to help the chain logic recognize the format of elements in `extraData` array.
+
+
+## Status
+
+Proposed
+
+## Consequences
+
+### Positive
+
+- Add extensibility to future bridge features without introducing new token standard;
+- Reduce number of upgrade or migrations requirement to existing token contracts.
+
+### Negative
+
+- Extra data defined in byte array will make the contract less readable.
+- Manpulation of argument inside a dynamic-sized byte array (bytes) is not trivial.
+
+### Neutral
+
+## References
+
+- Solidity Types: https://docs.soliditylang.org/en/v0.6.8/types.html
\ No newline at end of file
diff --git a/x/cronos/middleware/conversion_middleware.go b/x/cronos/middleware/conversion_middleware.go
index 38018fa2f4..5f9994a377 100644
--- a/x/cronos/middleware/conversion_middleware.go
+++ b/x/cronos/middleware/conversion_middleware.go
@@ -125,7 +125,7 @@ func (im IBCConversionModule) OnAcknowledgementPacket(
 	relayer sdk.AccAddress,
 ) error {
 	err := im.app.OnAcknowledgementPacket(ctx, packet, acknowledgement, relayer)
-	if err != nil {
+	if err == nil {
 		// Call the middle ware only at the "refund" case
 		var ack channeltypes.Acknowledgement
 		if err := transferTypes.ModuleCdc.UnmarshalJSON(acknowledgement, &ack); err != nil {