Skip to content
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

ICS100 Atomic Swap #867

Merged
merged 98 commits into from
Mar 6, 2023
Merged

ICS100 Atomic Swap #867

merged 98 commits into from
Mar 6, 2023

Conversation

liangping
Copy link
Contributor

Users may wish to exchange tokens without transfering tokens away from its native chain. ICS-100 enabled chains can facilitate atomic swaps between users and their tokens located on the different chains. This is useful for exchanges between specific users at specific prices, and opens opportunities for new application designs.

Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

From a high-level this seems fine. It looks like the only time that the swap is locked in is after the TakeSwapPacket is successfully received. So there doesn't seem to be a way to have a one-sided transfer here. Though the transfer on the taking side might be delayed.

I have larger questions on how this might work for potential DeFi users. It seems like the maker has a free hand to cancel the order, after the take order is in-flight but before it gets received. Is this acceptable?

It would be helpful for someone with a Defi perspective to review this to see if it is making the right design decisions

@liangping
Copy link
Contributor Author

First of all, thanks for the feedback!

I have larger questions on how this might work for potential DeFi users. It seems like the maker has a free hand to cancel the order, after the take order is in-flight but before it gets received. Is this acceptable?

This is a good question, Normally, Cancel Order has to cancel the order on the encounter-party chain first. then do it on itself. Therefore both Make Order and Cancel Order have long in-flight. It would be no problem if txs are executed in order, but seems it's no way to guarantee the executing order.

I believe either of following solutions will solve this issue.

  • Remove cancel, the only way close the order is to fill the order on the encounter-party chain.
  • Add timeout for Make Order, refund in a fixed time if it's not completed

What's your opinions?

Copy link
Contributor

@mpoke mpoke left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @liangping. See my comments below, mostly related to improving the understanding of the protocol.

Regarding the high-level protocol, I do have the following concerns:

  • Does the Maker module check when receiving a TakeSwap that the order_id is part of the OrderBook in the Taker module? Otherwise, the Maker could send a MakeSwap, and a malicious relayer could send back a TakeSwap followed by a Timeout (the MakeSwap is never received). As a result, the funds in the Maker module are both refunded and transferred to the Taker's address. Maybe the order should be saved on the Maker module onAcknowledgePacket.

  • What happens if CancelSwap is received by the Taker module before the ACK for a previously sent TakeSwap?

spec/app/ics-100-atomic-swap/README.md Outdated Show resolved Hide resolved
spec/app/ics-100-atomic-swap/README.md Outdated Show resolved Hide resolved
spec/app/ics-100-atomic-swap/README.md Outdated Show resolved Hide resolved
spec/app/ics-100-atomic-swap/README.md Outdated Show resolved Hide resolved
spec/app/ics-100-atomic-swap/README.md Outdated Show resolved Hide resolved
spec/app/ics-100-atomic-swap/README.md Outdated Show resolved Hide resolved
spec/app/ics-100-atomic-swap/README.md Outdated Show resolved Hide resolved
spec/app/ics-100-atomic-swap/README.md Outdated Show resolved Hide resolved
spec/app/ics-100-atomic-swap/README.md Outdated Show resolved Hide resolved
spec/app/ics-100-atomic-swap/README.md Show resolved Hide resolved
@liangping liangping requested a review from mpoke November 12, 2022 04:03
@liangping
Copy link
Contributor Author

liangping commented Feb 22, 2023

@mpoke Sorry for delay. I missed your comments, hope this answer you questions.

  • Does the Maker module check when receiving a TakeSwap that the order_id is part of the OrderBook in the Taker module? Otherwise, the Maker could send a MakeSwap, and a malicious relayer could send back a TakeSwap followed by a Timeout (the MakeSwap is never received). As a result, the funds in the Maker module are both refunded and transferred to the Taker's address. Maybe the order should be saved on the Maker module onAcknowledgePacket.

That's why we have two status on order, INITIAL or SYNCINITIAL means just created. SYNC means taker chain confirmed(status updated onAcknowledgePacket). Now we use the hash of the packet as the order_id, which implies the port and channel on both chains

  • What happens if CancelSwap is received by the Taker module before the ACK for a previously sent TakeSwap?

The order will be locked when takeSwap is sent, the cancel swap will be rejected when the order is locked. Only unlocked orders can be canceled.

@liangping liangping requested a review from mpoke February 22, 2023 01:18
@crodriguezvega
Copy link
Contributor

Hi @liangping. If it's ok with you, I will push some changes on your branch for the code owners file and will add both you and @egunawan85 as owners of the spec to indicate that you will be the main maintainers. Please let me know if there's somebody else that I should add.

I will also add some text on the top of the spec to inform readers that the spec has been reviewed by the spec committee but will not be maintained by it.

@liangping
Copy link
Contributor Author

liangping commented Feb 27, 2023

@crodriguezvega I am ok. add egunawan85 and me to the owner for now. thanks

Copy link
Contributor

@mpoke mpoke left a comment

Choose a reason for hiding this comment

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

LGTM

@AdityaSripal
Copy link
Member

Hello @liangping please let me know if I can squash and merge

@liangping
Copy link
Contributor Author

Hello @liangping please let me know if I can squash and merge

Yeah. Please!

@AdityaSripal AdityaSripal merged commit 3535456 into cosmos:main Mar 6, 2023
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.

7 participants