-
Notifications
You must be signed in to change notification settings - Fork 95
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
Asset Hub: Asset Conversion and Swap Weight Trader #218
Asset Hub: Asset Conversion and Swap Weight Trader #218
Conversation
@joepetrowski need your help to set there parameters:
I think we can have |
We decided to set both as Liquidity withdrawal fee can also be |
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.
Some minor nits but looking good. One question, XCM V3 is used throughout, but since SDK v1.7.0 we are on XCM v4. Do these types need to be updated here?
I didn't look too closely at the bridge test cases. Would be good if @acatangiu can check those.
system-parachains/asset-hubs/asset-hub-kusama/src/xcm_config.rs
Outdated
Show resolved
Hide resolved
// TODO: the types in the module copied from the PR: https://github.com/paritytech/polkadot-sdk/pull/3250 | ||
// and should be removed when changes from the PR will get released. |
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.
Link an issue?
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.
I am also collecting similar TODOs here: #186
system-parachains/asset-hubs/asset-hub-polkadot/src/xcm_config.rs
Outdated
Show resolved
Hide resolved
system-parachains/asset-hubs/asset-hub-polkadot/src/xcm_config.rs
Outdated
Show resolved
Hide resolved
integration-tests/emulated/tests/assets/asset-hub-kusama/src/tests/swap.rs
Outdated
Show resolved
Hide resolved
Reviewed tests and all of them look good! Before finishing full review, I had an early question: are we adding the DEX/pools on both Kusama and Polkadot at the same time? I was under the impression Polkadot was to get it one release later than Kusama.. |
This has been on Kusama for like 6 months :) |
Ok, sorry, I posted the early question too early (before even looking at the runtimes code) :)) |
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.
LGTM
integration-tests/emulated/tests/bridges/bridge-hub-kusama/src/tests/asset_transfers.rs
Outdated
Show resolved
Hide resolved
integration-tests/emulated/tests/bridges/bridge-hub-kusama/src/tests/asset_transfers.rs
Outdated
Show resolved
Hide resolved
integration-tests/emulated/tests/bridges/bridge-hub-kusama/src/tests/asset_transfers.rs
Outdated
Show resolved
Hide resolved
integration-tests/emulated/tests/bridges/bridge-hub-kusama/src/tests/asset_transfers.rs
Outdated
Show resolved
Hide resolved
integration-tests/emulated/tests/assets/asset-hub-kusama/src/tests/swap.rs
Show resolved
Hide resolved
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> Co-authored-by: Sergej Sakac <73715684+Szegoo@users.noreply.github.com>
@joepetrowski I have updated the deposits/fees. Plus for |
…ion-and-weight-trader
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.
lgmt, just left a few nits to check
integration-tests/emulated/tests/assets/asset-hub-kusama/src/tests/swap.rs
Outdated
Show resolved
Hide resolved
integration-tests/emulated/tests/assets/asset-hub-kusama/src/tests/swap.rs
Outdated
Show resolved
Hide resolved
@@ -721,6 +425,77 @@ fn limited_reserve_transfer_assets_for_native_asset_to_asset_hub_polkadot_works( | |||
) | |||
} | |||
|
|||
#[test] | |||
fn receive_reserve_asset_deposited_dot_from_asset_hub_polkadot_fees_paid_by_pool_swap_works() { |
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.
👍
/// | ||
/// The `PoolId` is represented as a tuple of `AssetKind`s with `FirstAsset` always positioned | ||
/// as the first element. | ||
pub struct WithFirstAsset<FirstAsset, AccountId, AssetKind, AccountIdConverter>( |
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.
what about the AssetHubKusama? It uses pallet_asset_conversion::WithFirstAsset
, should we change it there also?
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.
only possible with migration, which will be available for the next release
parameter_types! { | ||
pub const AssetConversionPalletId: PalletId = PalletId(*b"py/ascon"); | ||
pub const LiquidityWithdrawalFee: Permill = Permill::from_percent(0); | ||
pub const PoolSetupFee: Balance = system_para_deposit(1, 4); |
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.
what about AssetHubKusama? There is still:
type PoolSetupFee = ConstU128<0>; // Asset class deposit fees are sufficient to prevent spam
shouldn't we change AHK also?
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.
set it as in polkadot
// TODO: the types in the module copied from the PR: https://github.com/paritytech/polkadot-sdk/pull/3250 | ||
// and should be removed when changes from the PR will get released. |
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.
I am also collecting similar TODOs here: #186
// --extrinsic=* | ||
// --wasm-execution=compiled | ||
// --heap-pages=4096 | ||
// --output=./asset-hub-kusama-weights |
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.
when merged, I will regenerate weights as a part of #223
@acatangiu can you please help with review |
integration-tests/emulated/tests/assets/asset-hub-kusama/src/tests/swap.rs
Outdated
Show resolved
Hide resolved
type AssetDeposit = ConstU128<0>; | ||
type AssetAccountDeposit = AssetAccountDeposit; | ||
type MetadataDepositBase = ConstU128<0>; | ||
type MetadataDepositPerByte = ConstU128<0>; |
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.
DQ but why are they zero?
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.
Only the system (the AC pallet) can actually create assets within this instance of Assets, so only the system itself could actually create any assets and add any metadata.
@muharem is this ready to be merged? |
yes |
/merge |
There was a problem running the action.❌😵❌ Please find more information in the logs. |
/merge |
There was a problem running the action.❌😵❌ Please find more information in the logs. |
Asset Conversion instance for Polkadot Asset Hub and XCM Swap Weight Trader for both Asset Hubs.
TODO: