Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
IT: Simplify liquidity_pool test module #1869
IT: Simplify liquidity_pool test module #1869
Changes from 24 commits
ea08cec
0bc96f6
e9174c9
57908e9
1770f11
5b41f5e
cdf8837
8dd05e3
bf338c8
126eeb5
891bfe7
d781110
eeb1dbf
bdee3ed
0c3bb43
5a832ad
de3760a
1b79822
abace1f
a2f7119
d24c686
415a158
cce79b6
7f79140
3991dea
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Tech-debt to create some tests to be able to compute the exact fee charged in the above two cases. Using
FixedRateOfFungible
and usingAssetRegistryTrader
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.
BTW, here I would expect a symmetric behavior, but it isn't.
The implementation only checks the transferability when converting from
location -> currency
ref. Should we add the same filter forcurrency -> location
?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.
Honestly not sure if we want this to be symmetric. AFAICT, we could switch to that based on my below observations.
If we decide on that, it blocks converting any local currency locations into their corresponding currency id. One implication is that those currencies cannot be used for paying XCM transaction fees. On mainnet, CFG is configured as XCM transferable.
It would be interesting to see the failing tests when switching to symmetric behaviour. Depending on the failing tests, this could be a good indicator on whether we require location-to-currency location for local currencies as part of XCM for instance.
AFAICS, the
CurrencyIdConvert
is used to determine and withdraw cross-chain fee amounts in the configured fee currency in three different pallets:orml_xtokens::CurrencyIdConvert
to derive the asset origination locationpallet_xcm_transactor::CurrencyIdLocation
to withdraw the fee amount of the configured fee currencystaging_xcm_executor::Config::AssetTransactor = FungiblesTransactor
asMatcher
AT of ourFungiblesAdaptor
impl to withdraw the XCM fee in the configured fee currenciesCurious about your thoughts @mustermeiszer