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

New currency type #342

Merged
merged 9 commits into from
Nov 15, 2023
Merged

New currency type #342

merged 9 commits into from
Nov 15, 2023

Conversation

gianfra-t
Copy link
Contributor

@gianfra-t gianfra-t commented Nov 9, 2023

Closes #334.

The new type CurrencyId::Token(u64) was added to spacewalk here.

The new currency type is disabled to zenlink conversion functions.

@gianfra-t gianfra-t marked this pull request as ready for review November 9, 2023 20:11
@gianfra-t gianfra-t requested a review from ebma November 9, 2023 20:11
Base automatically changed from 314-add-the-new-spacewalk-pallets to main November 13, 2023 16:07
@ebma ebma requested a review from a team November 14, 2023 11:48
@ebma ebma linked an issue Nov 14, 2023 that may be closed by this pull request
Copy link
Member

@ebma ebma 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 nice description of the PR @gianfra-t.
It seems like we only have a choice between two 'easy' solutions:

  1. we disallow using CurrencyId::Tokens in Zenlink pools completely or
  2. we limit it to the CurrencyId::Tokens that fit into one byte, like you implemented it.

Everything else is probably more complicated because we need to change the ZenlinkLPToken type.

It might be worth discussing these two approaches before we choose one over the other. For now, I would actually vote to go for approach 1) because this way we still have time to think about the 'ideal' approach until we actually need it. If we use approach 2) and a pool is created with this we might limit ourselves in the future if we choose to come up with some fancy custom derivation function of the ID or whatever.

What's your opinion on this @gianfra-t? Maybe also @b-yap, @bogdanS98 and @TorstenStueber

@gianfra-t
Copy link
Contributor Author

gianfra-t commented Nov 14, 2023

If we are not interested in using Zenlink with these new currency type now, then I also think it is better to just not allow it and in the near future think of another way to derive the IDs not just for this new type but for other types that we could add, according to our future needs.
Because even if we change the internal type of CurrencyId::ZenlinkLPToken(u8, u8, u8, u8) to u256, we will still be limited by the 2 byte representation of the asset_index shared between all currency types.

@ebma
Copy link
Member

ebma commented Nov 14, 2023

True. Alright then let's completely disallow using Token for Zenlink pools and postpone thinking of a derivation once needed.

Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

LGTM overall

runtime/common/src/zenlink.rs Outdated Show resolved Hide resolved
runtime/common/src/zenlink.rs Outdated Show resolved Hide resolved
@ebma ebma self-requested a review November 15, 2023 17:01
@gianfra-t gianfra-t merged commit c15f27b into main Nov 15, 2023
2 checks passed
@gianfra-t gianfra-t deleted the new-currency-type branch November 15, 2023 19:03
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.

Use latest Spacewalk CurrencyId in runtimes
2 participants