-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[r2r] cosmos ibc transfer implementation #1636
Conversation
Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
4a7018b
to
f5b4237
Compare
0129533
to
14ef12c
Compare
Signed-off-by: ozkanonur <work@onurozkan.dev>
23f9972
to
99c0e39
Compare
Signed-off-by: ozkanonur <work@onurozkan.dev>
71b5cd2
to
177658b
Compare
Signed-off-by: ozkanonur <work@onurozkan.dev>
9f6ec81
to
9794039
Compare
Signed-off-by: ozkanonur <work@onurozkan.dev>
…smos-ibc-transfer
There was a problem hiding this 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 :)
@@ -97,6 +99,140 @@ impl TendermintToken { | |||
}; | |||
Ok(TendermintToken(Arc::new(token_impl))) | |||
} | |||
|
|||
pub fn ibc_withdraw(&self, req: IBCWithdrawRequest) -> WithdrawFut { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
…smos-ibc-transfer
There was a problem hiding this 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>
There was a problem hiding this 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>
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-approve
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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
@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: KomodoPlatform/komodo-docs-mdx#100 |
Sure, updated the PR description. |
Implements:
mm2_git
crateibc_withdraw
,ibc_chains
andibc_transfer_channels
RPC methodsThis 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:
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.
ibc_transfer_channels
Get IBC channel for doing IBC operation (useful for people who doesn't want to check channels manually from cosmos chains)
ibc_withdraw
Just a witdhdraw, but through the cosmos IBC channels.