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

generalize asset id in standard amm #46

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

gregdhill
Copy link
Contributor

@gregdhill gregdhill commented Oct 28, 2022

Signed-off-by: Gregory Hill gregorydhill@outlook.com

Allows the consumer to use any AssetId when instantiating the "standard" AMM. In my case I would like to directly use the CurrencyId instead of converting to and from zenlink_protocol::AssetId since we anyway will have XCM disabled (see this comment). The important thing to note is that this PR does not introduce any functional changes since the consumer may still use the concrete AssetId type and PairLpGenerate.

Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
@gregdhill gregdhill changed the title generalize asset id in stable amm generalize asset id in standard amm Oct 28, 2022
@jerrybaoo
Copy link
Collaborator

jerrybaoo commented Nov 1, 2022

  1. It makes sense to keep the same format across all chains for cross-chain transactions. This is the reason why AssetId is not generic in the standard AMM.
  2. Using trait to generate LPs is a meaningful change.
    We will make some improvements based on your PR

@jerrybaoo
Copy link
Collaborator

jerrybaoo commented Nov 1, 2022

In fact, if the chain uses the substrate asset pallet(https://github.com/paritytech/substrate/tree/master/frame/assets), then the conversion is very smooth.

@gregdhill
Copy link
Contributor Author

It makes sense to keep the same format across all chains for cross-chain transactions.

As I understand you anyway intend to separate the XCM functionality so I'm not convinced forcing this format across all chains is meaningful here - especially as the CurrencyId in the "stable" AMM is already generic.

@jerrybaoo jerrybaoo merged commit 26ea50a into zenlinkpro:master Nov 2, 2022
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.

2 participants