improvement(tendermint): safer IBC channel handler by onur-ozkan · Pull Request #2298 · GLEECBTC/komodo-defi-framework

onur-ozkan · 2024-12-18T05:57:29Z

This change removes the logic that parses the chain-registry repository to find IBC channels, as that approach often included outdated or unreliable information. Instead, with this PR, we embed only a highly reliable and trusted IBC channels into our coins file. KDF will now look the coins file to find the channel ID and validate it (for now it only checks if channel is available). If it can't find the channel in coins file, it will return an error:
IBC channel could not be found in the coins file for $address. Please provide it manually by including ibc_source_channel in the request. See https://ibc.iobscan.io/channels for reference.

Example of ibc_channels usage in coins file:

{
  "coin":"IRIS",
  "avg_blocktime": 5,
  "protocol":{
    "type":"TENDERMINT",
    "protocol_data":{
      "decimals":6,
      "denom":"uiris",
      "account_prefix":"iaa",
      "chain_id":"irishub-1",
      "ibc_channels": {
          "cosmos": 0
      }
    }
  }
},
{
  "coin": "ATOM",
  "avg_blocktime": 7,
  "name": "cosmos",
  "fname": "Cosmos",
  "mm2": 1,
  "protocol": {
    "type": "TENDERMINT",
    "protocol_data": {
      "decimals": 6,
      "denom": "uatom",
      "account_prefix": "cosmos",
      "chain_id": "cosmoshub-4",
      "ibc_channels": {
          "iaa": 91
      }
    }
  }
}

The ibc_channels fields above allow KDF to determine the IRIS channel from ATOM and the ATOM channel from IRIS during IBC operations. The list of IDs can be extended to support fallback strategies (e.g., from "iaa": [91, $some_other_ID] KDF will try to use $some_other_ID when 91 isn't healhty/available), and the list of address prefixes can also be expanded to discover IBC channels on other networks without any limitation on all Tendermint assets. https://ibc.iobscan.io/channels should be used for finding most reliable channels for the Tendermint assets in coins file.

Note: We need to update coins file before merging this (PR is up), otherwise IBC operations will not work if ibc_source_channel isn't provided manually.

Please be aware of these breaking changes (cc @KomodoPlatform/gui):

Breaking change 1: improvement(tendermint): safer IBC channel handler #2298 (comment)
Breaking change 2: improvement(tendermint): safer IBC channel handler #2298 (comment)

Signed-off-by: onur-ozkan <work@onurozkan.dev>

… into safer-ibc

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan · 2025-04-07T13:41:56Z

mm2src/mm2_main/src/rpc/dispatcher/dispatcher.rs

-        "ibc_chains" => handle_mmrpc(ctx, request, ibc_chains).await,
-        "ibc_transfer_channels" => handle_mmrpc(ctx, request, ibc_transfer_channels).await,


Breakage 1: I am confident enough to say that none of our clients are using these. 😄

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan · 2025-04-09T08:52:29Z

mm2src/coins/lp_coins.rs

    memo: Option<String>,
    /// Tendermint specific field used for manually providing the IBC channel IDs.
-    ibc_source_channel: Option<String>,
+    ibc_source_channel: Option<ChannelId>,


Breakage 2: This now expects a numeric value (previously it accepted any String value without validation by hoping it was using in the channel-$id format) to align with the logic used in the coins file.

Signed-off-by: onur-ozkan <work@onurozkan.dev>

borngraced

Well done. I have a note and a question

mm2src/coins/tendermint/tendermint_coin.rs

laruh

Looks good in general, I have small notes

mm2src/coins/rpc_command/tendermint/staking.rs

mm2src/coins/tendermint/tendermint_coin.rs

Signed-off-by: onur-ozkan <work@onurozkan.dev>

mm2src/coins/tendermint/tendermint_coin.rs

laruh

LGTM!

shamardy

LGTM! Only a nit! Does this need testing first or can be tested in dev?

mm2src/coins/lp_coins.rs

Signed-off-by: onur-ozkan <work@onurozkan.dev>

shamardy

Awaiting 2.4.0-beta release to merge this.

onur-ozkan · 2025-05-05T07:13:42Z

I assume we can land this now?

shamardy · 2025-05-05T16:16:03Z

@onur-ozkan @smk762 is the docs issue or PR already opened for these changes, if not, this is just a reminder.

Edit: Nevermind, found docs and coins issues/PRs mentioned

* dev: (26 commits) chore(deps): remove base58 and replace it completely with bs58 (GLEECBTC#2427) feat(tron): initial groundwork for full TRON integration (GLEECBTC#2425) fix(UTXO): improve tx fee calculation and min relay fee handling (GLEECBTC#2316) deps(timed-map): bump to 1.3.1 (GLEECBTC#2413) improvement(tendermint): safer IBC channel handler (GLEECBTC#2298) chore(release): complete v2.4.0-beta changelogs (GLEECBTC#2436) fix(event-streaming): initial addresses registration in utxo balance streaming (GLEECBTC#2431) improvement(watchers): re-write use-watchers handling (GLEECBTC#2430) fix(evm): make withdraw_nft work in HD mode (GLEECBTC#2424) feat(taproot): support parsing taproot output address types chore(RPC): use consistent param name for QTUM delegation (GLEECBTC#2419) fix(makerbot): add LiveCoinWatch price provider (GLEECBTC#2416) chore(release): add changelog entries for v2.4.0-beta (GLEECBTC#2415) fix(wallets): prevent path traversal in `wallet_file_path` and update file extension (GLEECBTC#2400) fix(nft): make `update_nft` work with hd wallets using the enabled address (GLEECBTC#2386) fix(wasm): unify error handling for mm2_main (GLEECBTC#2389) fix(tx-history): token information and query (GLEECBTC#2404) test(electrums): fix failing test_one_unavailable_electrum_proto_version (GLEECBTC#2412) improvement(network): remove static IPs from seed lists (GLEECBTC#2407) improvement(best-orders): return an rpc error when we can't find best orders (GLEECBTC#2318) ...

* dev: (29 commits) fix(p2pk): validate expected pubkey correctly for p2pk inputs (#2408) chore(docs): update old urls referencing atomicdex or old docs pages (#2428) improvement(p2p): remove hardcoded seeds (#2439) fix(evm-api): find enabled erc20 token using platform ticker (#2445) chore(docs): add DeepWiki badge to README (#2463) chore(core): organize deps using workspace.dependencies (#2449) feat(db-arch): more dbdir to address_dir replacements (#2398) chore(build-artifacts): remove duplicated mm2 build artifacts (#2448) feat(pubkey-banning): expirable bans (#2455) fix(eth-balance-events): serialize eth address using AddrToString (#2440) chore(deps): remove base58 and replace it completely with bs58 (#2427) feat(tron): initial groundwork for full TRON integration (#2425) fix(UTXO): improve tx fee calculation and min relay fee handling (#2316) deps(timed-map): bump to 1.3.1 (#2413) improvement(tendermint): safer IBC channel handler (#2298) chore(release): complete v2.4.0-beta changelogs (#2436) fix(event-streaming): initial addresses registration in utxo balance streaming (#2431) improvement(watchers): re-write use-watchers handling (#2430) fix(evm): make withdraw_nft work in HD mode (#2424) feat(taproot): support parsing taproot output address types ...

onur-ozkan added 2 commits December 18, 2024 05:26

read new field ibc_channels from coins file

3368dec

Signed-off-by: onur-ozkan <work@onurozkan.dev>

remove old IBC logics

8696db6

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan added status: in progress improvement: protocol breaking-change: RPC labels Dec 18, 2024

update channel finding logic

a8ce167

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan added status: blocked status: pending infra and removed status: in progress labels Dec 18, 2024

onur-ozkan added 3 commits April 7, 2025 12:54

Merge branch 'dev' of github.com:KomodoPlatform/komodo-defi-framework…

edad587

… into safer-ibc

use default HashMap if ibc_channels isn't provided

963ae4e

Signed-off-by: onur-ozkan <work@onurozkan.dev>

support backup IBC clients for recovery when first one isn't available

a412636

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan added status: in progress and removed status: blocked status: pending infra labels Apr 7, 2025

onur-ozkan commented Apr 7, 2025

View reviewed changes

onur-ozkan added 4 commits April 8, 2025 11:40

implement channel querying

effa2d2

Signed-off-by: onur-ozkan <work@onurozkan.dev>

finish test coverage

0bc8220

Signed-off-by: onur-ozkan <work@onurozkan.dev>

better error message

a85b082

Signed-off-by: onur-ozkan <work@onurozkan.dev>

handle IBC channel IDs stricter

9cd189d

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan commented Apr 9, 2025

View reviewed changes

onur-ozkan changed the title ~~improvement(tendermint): robust IBC channel handling~~ improvement(tendermint): safer IBC channel handling Apr 9, 2025

more fee for IBC withdraws

cad4e60

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan added status: pending review improvement: security Improvements made on runtime security status: pending test and removed status: in progress labels Apr 10, 2025

onur-ozkan mentioned this pull request Apr 10, 2025

a change on ibc_source_channel field in withdraw RPC GLEECBTC/komodo-docs-mdx#437

Closed

onur-ozkan changed the title ~~improvement(tendermint): safer IBC channel handling~~ improvement(tendermint): safer IBC channel handler Apr 10, 2025

accept only one channel id on per network

c9858fd

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan mentioned this pull request Apr 14, 2025

re-write IBC related fields GLEECBTC/coins#1335

Closed

shamardy requested review from borngraced and laruh April 14, 2025 23:03

shamardy added the 2.5.0-beta label Apr 14, 2025

borngraced suggested changes Apr 15, 2025

View reviewed changes

mm2src/coins/tendermint/tendermint_coin.rs Outdated Show resolved Hide resolved

mm2src/coins/tendermint/tendermint_coin.rs Outdated Show resolved Hide resolved

laruh suggested changes Apr 15, 2025

View reviewed changes

mm2src/coins/rpc_command/tendermint/staking.rs Show resolved Hide resolved

mm2src/coins/tendermint/tendermint_coin.rs Outdated Show resolved Hide resolved

address nits

d4ccb83

Signed-off-by: onur-ozkan <work@onurozkan.dev>

laruh suggested changes Apr 16, 2025

View reviewed changes

mm2src/coins/tendermint/tendermint_coin.rs Outdated Show resolved Hide resolved

borngraced previously approved these changes Apr 16, 2025

View reviewed changes

laruh previously approved these changes Apr 16, 2025

View reviewed changes

shamardy previously approved these changes Apr 17, 2025

View reviewed changes

mm2src/coins/lp_coins.rs Outdated Show resolved Hide resolved

mm2src/coins/lp_coins.rs Outdated Show resolved Hide resolved

nits

2857691

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan dismissed stale reviews from shamardy, laruh, and borngraced via 2857691 April 18, 2025 14:40

onur-ozkan force-pushed the safer-ibc branch from 7329688 to 2857691 Compare April 18, 2025 14:40

remove iobscan ref link

b2add2f

Signed-off-by: onur-ozkan <work@onurozkan.dev>

shamardy approved these changes Apr 21, 2025

View reviewed changes

shamardy merged commit bccdb93 into dev May 5, 2025
21 of 24 checks passed

shamardy deleted the safer-ibc branch May 5, 2025 16:15

CharlVS mentioned this pull request May 23, 2025

fix: add IBC channel option for withdrawals GLEECBTC/gleec-wallet#2682

Merged

shamardy mentioned this pull request Jun 16, 2025

chore(release): v2.5.0-beta #2491

Merged

		"ibc_chains" => handle_mmrpc(ctx, request, ibc_chains).await,
		"ibc_transfer_channels" => handle_mmrpc(ctx, request, ibc_transfer_channels).await,

Conversation

onur-ozkan commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

onur-ozkan Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onur-ozkan Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borngraced left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

laruh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

laruh left a comment

Choose a reason for hiding this comment

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

onur-ozkan commented May 5, 2025

Uh oh!

Uh oh!

shamardy commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

onur-ozkan commented Dec 18, 2024 •

edited

Loading

onur-ozkan Apr 7, 2025 •

edited

Loading

onur-ozkan Apr 9, 2025 •

edited

Loading

shamardy commented May 5, 2025 •

edited

Loading