Skip to content

[r2r] cosmos ibc transfer implementation#1636

Merged
shamardy merged 19 commits intodevfrom
cosmos-ibc-transfer
Mar 9, 2023
Merged

[r2r] cosmos ibc transfer implementation#1636
shamardy merged 19 commits intodevfrom
cosmos-ibc-transfer

Conversation

@onur-ozkan
Copy link
Copy Markdown

@onur-ozkan onur-ozkan commented Jan 27, 2023

Implements:

This implementation requires chain_registry_name field to be added in coins configuration. It's an optional field, so if chain doesn't have directory in the repository, we can simply leave it as it is.
e.g:

  {
    "coin":"IRIS-TEST",
    "avg_blocktime": 5,
    "protocol":{
      "type":"TENDERMINT",
      "protocol_data":{
        "decimals":6,
        "denom":"unyan",
        "account_prefix":"iaa",
        "chain_id":"nyancat-9",
        "chain_registry_name":"irisnet"
      }
    }
  }

We need this field so we can find the proper path of chains to find informations in cosmos/chain-registry repository.

Examples of implemented RPC methods

ibc_chains

List of cosmos chains that supports IBC operations.

curl -v --url "http://127.0.0.1:7783" -s --data '{
	"userpass": "'$userpass'",
	"mmrpc": "2.0",
	"method": "ibc_chains",
	"id": 0
}'

ibc_transfer_channels

Get IBC channel for doing IBC operation (useful for people who doesn't want to check channels manually from cosmos chains)

curl -v --url "http://127.0.0.1:7783" -s --data '{
	"userpass": "'$userpass'",
	"mmrpc": "2.0",
	"method": "ibc_transfer_channels",
  	"params": {
		"coin": "IRIS-TEST",
		"destination_chain_registry_name": "osmosis" // a chain name from ibc_chains response
	},
	"id": 0
}'

ibc_withdraw

Just a witdhdraw, but through the cosmos IBC channels.

curl -v --url "http://127.0.0.1:7783" -s --data '{
	"userpass": "'$userpass'",
	"mmrpc": "2.0",
	"method": "ibc_withdraw",
	"params": {
		"ibc_source_channel": "channel-81", // IBC source channel(also known as channel id) to use for withdraw operation
		"coin": "IRIS-TEST",
		"to": "cosmos1r5v5srda7xfth3hn2s26txvrcrntldjumt8mhl",
		"amount": "1",
		"memo": "asd"
	},
	"id": 0
}'

Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
@onur-ozkan onur-ozkan mentioned this pull request Jan 27, 2023
@onur-ozkan onur-ozkan changed the title [wip] cosmos ibc transfer implementation [r2r] cosmos ibc transfer implementation Jan 30, 2023
Signed-off-by: ozkanonur <work@onurozkan.dev>
@onur-ozkan onur-ozkan changed the title [r2r] cosmos ibc transfer implementation [wip] cosmos ibc transfer implementation Jan 31, 2023
Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
@onur-ozkan onur-ozkan changed the title [wip] cosmos ibc transfer implementation [r2r] cosmos ibc transfer implementation Feb 7, 2023
Copy link
Copy Markdown
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work as always! Just a few questions/suggestions at this stage of the review :)

Ok(TendermintToken(Arc::new(token_impl)))
}

pub fn ibc_withdraw(&self, req: IBCWithdrawRequest) -> WithdrawFut {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parts of this function and the ibc_withdraw function for tendermint_coin can probably be made into a common function, do you think this is a good idea?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already unified their shared flows, but I realized if I do it too much, it will be harder to debug and maintain.

It's even worst if we combine them into single function because their fee calculations are pretty different.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a trait with some common implementations alongside the builder pattern might work in this case, similar to UtxoWithdraw
https://github.com/KomodoPlatform/atomicDEX-API/blob/dcba19bf11a1e29602cffc291d41808c36775b21/mm2src/coins/utxo/utxo_withdraw.rs#L93
It's up to you in the end, since this suggestion might have some drawbacks too. Just please consider this refactor in next PRs, since more withdraw methods might be added in the future if hardware wallet is to be implemented for tendermint.

@onur-ozkan onur-ozkan changed the title [r2r] cosmos ibc transfer implementation [wip] cosmos ibc transfer implementation Feb 15, 2023
Signed-off-by: ozkanonur <work@onurozkan.dev>
@onur-ozkan onur-ozkan changed the title [wip] cosmos ibc transfer implementation [r2r] cosmos ibc transfer implementation Feb 15, 2023
@onur-ozkan onur-ozkan requested a review from shamardy February 15, 2023 11:59
@onur-ozkan onur-ozkan mentioned this pull request Feb 18, 2023
shamardy
shamardy previously approved these changes Feb 22, 2023
Copy link
Copy Markdown
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the fixes! Only non-blocker question.

Signed-off-by: ozkanonur <work@onurozkan.dev>
…smos-ibc-transfer

Signed-off-by: ozkanonur <work@onurozkan.dev>
…smos-ibc-transfer

Signed-off-by: ozkanonur <work@onurozkan.dev>
Copy link
Copy Markdown

@borngraced borngraced left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! 🔥

Signed-off-by: ozkanonur <work@onurozkan.dev>
…smos-ibc-transfer

Signed-off-by: ozkanonur <work@onurozkan.dev>
laruh
laruh previously approved these changes Mar 9, 2023
Copy link
Copy Markdown

@laruh laruh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome job!
I just have a small note. Could you please fix this typo? :) I think its contains

Signed-off-by: ozkanonur <work@onurozkan.dev>
Copy link
Copy Markdown
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approve

@shamardy shamardy merged commit 45d7285 into dev Mar 9, 2023
@shamardy shamardy deleted the cosmos-ibc-transfer branch March 9, 2023 14:52
assert_eq!(tx_details.to, vec![IBC_TARGET_ADDRESS.to_owned()]);
assert_eq!(tx_details.from, vec![MY_ADDRESS.to_owned()]);

let send_raw_tx = block_on(send_raw_transaction(&mm, token, &tx_details.tx_hex));
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add some additional checks to this test to make sure that the transaction does what it's supposed to do, e.g. you can compare the balances of the sender and the receiver before/after the send_raw_transaction. I see that the PR is merged now so you can do it on the next PR.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the relayer I used in this test doesn't work properly on testnet, that's why I couldn't compare balances. So either we have to deploy relayer for testnet that works properly, or use nucleus chain for this specific test because it works without any issue

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, didn't know you were reviewing this. I merged it since it got the minimum of 2 approvals and was under review for a long time. From now on we should be adding only 2 reviewers on a PR (1 is also enough for a small PR), if someone doesn't plan to review a PR, they can remove themselves and another team member can review it instead :)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah no problem, I only had this one comment anyways.

@ca333 ca333 mentioned this pull request Mar 27, 2023
@laruh laruh mentioned this pull request Apr 4, 2023
@smk762
Copy link
Copy Markdown

smk762 commented Aug 31, 2023

@onur-ozkan I just noticed these methods are not documented. Can you please provide a description of each method and its parameters used in your examples?

Docs issue: GLEECBTC/komodo-docs-mdx#100

@onur-ozkan
Copy link
Copy Markdown
Author

@onur-ozkan I just noticed these methods are not documented. Can you please provide a description of each method and its parameters used in your examples?

Sure, updated the PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants