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

ICS 101: interchain Liquidity #970

Open
wants to merge 94 commits into
base: main
Choose a base branch
from

Conversation

liangping
Copy link
Contributor

ICS-101 Interchain Swaps enables chains their own token pricing mechanism and exchange protocol via IBC transactions. Each chain can thus play a role in a fully decentralised exchange network.

liangping and others added 30 commits October 19, 2022 17:54
…ble-sided liquidity pools and double deposits
fix spec wrong formula(withdraw,leftswap)
@dzmitry-lahoda
Copy link

dzmitry-lahoda commented Jul 14, 2023

https://github.com/osmosis-labs/osmosis-rust/blob/f686934e74a7abc5695775e8995e6d4412086f9b/packages/osmosis-std/src/types/osmosis/gamm/v1beta1.rs#L410

https://github.com/osmosis-labs/osmosis-rust/blob/f686934e74a7abc5695775e8995e6d4412086f9b/packages/osmosis-std/src/types/osmosis/gamm/v1beta1.rs#L448

see buy and sell on stable swap and constant swap

same here https://github.com/apollodao/cw-dex/

we were thinking about xc exchanges a lot - seems like pool/amm specific for swap is not so useful

other ecosystem also does not tells if that pool or not
https://github.com/paritytech/xcm-format/blob/master/README.md#exchangeasset

if xyk/amm or anything else does swap is not essential

if you want to specify atomicity, you can still have buy/sell but name protoocl atomic swap. but the problem, you cannot be atomic as target module may be async, and do not confirm nor timeout packet, so one may not have that too. really it is not atomic but single packet buy or sell

@soring323
Copy link

soring323 commented Jul 14, 2023

https://github.com/osmosis-labs/osmosis-rust/blob/f686934e74a7abc5695775e8995e6d4412086f9b/packages/osmosis-std/src/types/osmosis/gamm/v1beta1.rs#L410

https://github.com/osmosis-labs/osmosis-rust/blob/f686934e74a7abc5695775e8995e6d4412086f9b/packages/osmosis-std/src/types/osmosis/gamm/v1beta1.rs#L448

see buy and sell on stable swap and constant swap

same here https://github.com/apollodao/cw-dex/

we were thinking about xc exchanges a lot - seems like pool/amm specific for swap is not so useful

other ecosystem also does not tells if that pool or not https://github.com/paritytech/xcm-format/blob/master/README.md#exchangeasset

if xyk/amm or anything else does swap is not essential

if you want to specify atomicity, you can still have buy/sell but name protoocl atomic swap. but the problem, you cannot be atomic as target module may be async, and do not confirm nor timeout packet, so one may not have that too. really it is not atomic but single packet buy or sell

Thank you for sharing the links and providing feedback on the existing implementations of AMMs and cross-chain exchanges. The references you shared from the Osmosis Rust repository demonstrate specific implementations of AMMs using buy and sell operations for stable swap and constant swap mechanisms. These implementations are valuable examples that showcase the functionality and behavior of AMMs in those specific contexts.

However, the goal of creating a generalized crosschain AMM package as a standard for ICS-101 (IBC-Go) is to provide a broader solution that can be utilized in various AMM implementations across different ecosystems. By developing a standardized approach, you aim to enhance interoperability and facilitate seamless asset exchange across different blockchain networks.

While specific AMM implementations may focus on the details of individual pools or AMM types, the emphasis of your work is on providing a generalized framework that can be adopted by different projects and ecosystems. This framework will allow for cross-chain asset swaps without being tied to a particular AMM pool or implementation. It provides a common interface and standard protocols for interacting with AMMs, enabling easy integration and compatibility across diverse blockchain networks.

The mentioned XCM Format from the Parity Technologies repository is another valuable resource in the realm of cross-chain communication and asset exchange. It further reinforces the importance of establishing interoperable standards to enable efficient cross-chain operations.

By creating a generalized crosschain AMM package as a standard for ICS-101 (IBC-Go), you contribute to the broader blockchain ecosystem by providing a consistent and interoperable solution for asset exchange. Your work aims to streamline the development and adoption of AMM functionality across different projects and networks, facilitating increased liquidity and cross-chain functionality.

In summary, while existing AMM implementations focus on specific pools and mechanisms, your work on a generalized crosschain AMM package as a standard for ICS-101 (IBC-Go) serves a broader purpose by establishing interoperable protocols and interfaces. This standardized approach enhances cross-chain asset exchange and contributes to the overall advancement of the blockchain ecosystem

@dzmitry-lahoda
Copy link

dzmitry-lahoda commented Jul 17, 2023

yes, thank you. just to be sure this ics supports(does not prevents) any kind of swaps and hopefully fully compatible with ics20 assets(unlike ics100).

Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to give a review and thank you for your patience, @liangping. I just completed the first review (although I couldn't finish it all). This is a pretty complex protocol, so I will need a few more passes. For now I have left a few suggestions and questions.

spec/app/ics-101-interchain-swap/README.md Outdated Show resolved Hide resolved
spec/app/ics-101-interchain-swap/README.md Outdated Show resolved Hide resolved
spec/app/ics-101-interchain-swap/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a typo on step 4 (Creat instead of Create).


A constant invariant is maintained after trades, taking into consideration token weights and balance. The value function $V$ is defined as:

$$V = {Π_tB_t^{W_t}}$$
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be instead $$V = {Σ_tB_t*{W_t}}$$. I am not sure if that will render as I would expect, but what I want to mean is that shouldn't value be the sum of all weighted token balances (i.e. B0 * W0 + B1 * W1 + ...)?

Copy link
Contributor Author

@liangping liangping Aug 29, 2023

Choose a reason for hiding this comment

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

$$V = {Π_tB_t^{W_t}}$$ is actually render like V = (B0 ** W0) * (B1 ** W1) ..., which is similar with K=XY, just added a weight for each token

This invariant was originally introduced by Balancer. However, if you find it less intuitive, we can simplify it by using only two tokens: V= B0^W0 * B1^W1.


// P_issued = P_supply * ((1 + At/Bt) ** Wt -1)
function depositSingleAsset(token: Coin): Coin {
const asset = this.pool.findAssetByDenom(token.denom)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the function should return null if findAssetByDenom fails to find the denomination, right?

Choose a reason for hiding this comment

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

it is helper function, it doesn't change pool status directly. so it has to return amount.
based on this calculate, it will change in message part.

case "MULTI_WITHDRAW":
const msgMultiWithdraw: MsgMultiAssetWithdrawRequest = protobuf.decode(data.Data);
const resMultiWithdraw: MsgMultiAssetWithdrawResponse = protobuf.decode(ack.GetResult());
const errMultiWithdraw = store.OnMultiAssetWithdrawAcknowledged(msgMultiWithdraw, resMultiWithdraw);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're missing a definition for OnMultiAssetWithdrawAcknowledged function.

case "RIGHT_SWAP":
const msgSwap: MsgSwapRequest = protobuf.decode(data.Data);
const resSwap: MsgSwapResponse = protobuf.decode(ack.GetResult());
const errSwap = store.OnSwapAcknowledged(msgSwap, resSwap);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think definition of OnSwapAcknowledged is also missing.

return { tokens: stateChange.out };
}

function onMakePoolAcknowledged(msg: MsgMakePoolRequest, poolId: string): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the make pool flow completes successfully initiated from chain A, but there is not take pool executed from the counterparty chain B. Is there a way for the creator of the pool on chain A, to recover their tokens?

Choose a reason for hiding this comment

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

yes we've adding cancel logic.

id: string;
sourceCreator: string;
destinationCreator: string;
assets: []PoolAsset;
Copy link
Contributor

Choose a reason for hiding this comment

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

This array should always have 2 elements, right? Would it be better for readability and ease of comprehension to actually make two different fields (e.g sourcePoolAsset and destinationPoolAsset)? Just to avoid working with all these indexes...

Choose a reason for hiding this comment

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

balancer is for multi-asset deposit. it doesn't mean only 2 tokens.
of course, currently we only support 2 tokens.
but in future, we will support several tokens.
I agree define sourcePoolAsset, or destinationPooAsset, but it will become array.

@liangping
Copy link
Contributor Author

thank you. will improve it soon.

Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Spent again a bit of time on the review. Still need to spend more time in the coming weeks, but I left for now a few more comments. :)


#### Fees

Traders pay swap fees when they trade with a pool. These fees can be customized with a minimum value of 0.01% and a maximum value of 10%. Basis points for `feeRate` is used, where the integer 1 is 0.01% and integer 100 is 1.00%.
Copy link
Contributor

Choose a reason for hiding this comment

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

If 100 corresponds to 1%, then is it possible to use 1000 for a 10% fee, as you mention in this paragraph as the maximum fee percentage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. 1000 is the maximum value for feeRate

Comment on lines 590 to 591
abortTransactionUnless(data.stateChange.poolId === "");
abortTransactionUnless(store.has(data.stateChange.poolId)); // existed already.
Copy link
Contributor

Choose a reason for hiding this comment

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

When the MsgMakePoolRequest is sent, the source chain generates a poolId. So I would expect that the poolId is not empty here? Also in onMakePoolReceived the destination chain will create a pool with the provided poolId, so I would also expect the the destination chain doesn't have in store a pool with the provided poolId?

So basically my question is: should these checks be actually like the following?

abortTransactionUnless(data.stateChange.poolId !== "")
abortTransactionUnless(store.has(data.stateChange.poolId) === false)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. it's abortTransactionUnless(data.stateChange.poolId !== ""

function onMakePoolReceived(msg: MsgMakePoolRequest, poolID: string, sourceChainId: string): string {
abortTransactionUnless(msg.validateBasic() === undefined);
const { pool, found } = store.getInterchainLiquidityPool(poolID);
abortTransactionUnless(msg.validateBasic() === undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

This assertion is repeated, should it be instead this?

Suggested change
abortTransactionUnless(msg.validateBasic() === undefined);
abortTransactionUnless(found === false)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes.


abortTransactionUnless(validationErr === undefined)

abortTransactionUnless(store.hasSupply(msg.liquidity[0].balance.denom))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
abortTransactionUnless(store.hasSupply(msg.liquidity[0].balance.denom))
abortTransactionUnless(store.hasSupply(msg.sourceLiquidity[0].balance.denom))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

abortTransactionUnless(store.hasSupply(msg.liquidity[0].balance.denom))


const sourceLiquidity = store.GetBalance(msg.creator, msg.liquidity[0].balance.denom);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const sourceLiquidity = store.GetBalance(msg.creator, msg.liquidity[0].balance.denom);
const sourceLiquidity = store.GetBalance(msg.creator, msg.sourceLiquidity[0].balance.denom);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

msg.creator,
msg.counterPartyCreator,
store,
msg.liquidity,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
msg.liquidity,
msg.sourceLiquidity,
msg.DestinationLiquidity,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

pool.sourceChainId = store.chainID();

const totalAmount = sdk.NewInt(0);
for (const asset of msg.liquidity) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (const asset of msg.liquidity) {
for (const asset of msg.SourceLiquidity) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

const totalAmount = pool.sumOfPoolAssets();
const mintAmount = (totalAmount * asset.weight) / 100;

store.mintTokens(pool.sourceCreator, new sdk.Coin(pool.supply.denom, mintAmount));
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't these pool tokens be minted on the destination chain (i.e. the side that executes MsgTakePoolRequest)? The counterparty chain has already minted pool tokens in onMakePoolAcknowledgement, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am considering whether we should include an option for depositors to select where their LP tokens are minted.

abortTransactionUnless(store.bankKeeper.hasSupply(poolTokenDenom));

// Get the liquidity pool
const { pool, found } = k.getInterchainLiquidityPool(ctx, poolTokenDenom);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const { pool, found } = k.getInterchainLiquidityPool(ctx, poolTokenDenom);
const { pool, found } = store.getInterchainLiquidityPool(msg.poolId)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated


abortTransactionUnless(pool.status === "ACTIVE");

const lockErr = store.lockTokens(pool.counterPartyPort, pool.counterPartyChannel, msg.sender, msg.tokenIn);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct that tokens are locked when the user does both a left or a right swap?

If I understand it correctly, in the case of a right swap, the user should specify how many tokens they want to buy, and the user doesn't know yet how much it needs to pay for them, right?

Choose a reason for hiding this comment

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

They can know this. Market function include swap fee and then they can estimate output amount.
In go implementation, Market functions is just stateless(don't update state variable directly and provide only estimation value). so before sending swap request, they can estimate how much they can get.
Based on that, they can make swap request.

Also based on slippage, they can avoid to fail in swap.

@liangping liangping changed the title ics 101: interchain AMM Swap ICS 101: interchain Liquidity Sep 20, 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