-
Notifications
You must be signed in to change notification settings - Fork 286
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
use relaychain asset as fee #700
Conversation
Codecov Report
@@ Coverage Diff @@
## master #700 +/- ##
==========================================
+ Coverage 74.99% 75.39% +0.40%
==========================================
Files 83 83
Lines 7266 7438 +172
==========================================
+ Hits 5449 5608 +159
- Misses 1817 1830 +13
Continue to review full report at Codecov.
|
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 would recommend to update transfer_with_fee
impl instead of adding a new dispatchable call.
The new transfer_using_relaychain_as_fee
is basically the same with do_transfer_multiassets
, but it doesn't require fee and other assets share the same reserve, plus it use non-fee assets as reserve.
Updating do_transfer_multiassets
and make it
- Only ensure non-fee assets share the same reserve location.
- Refer to non-fee assets for reserve location.
should work. As it only extends use cases of it with less limitations, it's not a breaking change for transfer_with_fee
.
That make sense, but I think change open-runtime-module-library/xtokens/src/tests.rs Lines 310 to 315 in f8f6f82
|
My bad, I mean the new updated impl which can specify fee asset type. Updating |
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.
DistinctReserveForAssetAndFee
error variant should be removed, as there is not such limitation any more.
I think |
This reverts commit aac79b3.
xtokens/src/lib.rs
Outdated
// destination chain, which asset is originated from sender account on sender | ||
// chain. This means if user setup too much fee, the fee is not returned to | ||
// user, instead deposit to sibling parachain sovereign account on dest chain. | ||
// Notice: if parachain set `SelfLocation` to (0, Here), it'll be error! |
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.
This should be documented in SelfLocation
definition.
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.
move to README
xtokens/src/lib.rs
Outdated
// Current not ensure xcm order delivery. if second xcm is executed before first | ||
// xcm, then second xcm may failed because of sibling parachain account don't | ||
// have enough fee to withdraw. we can pre-fund some amount to sibling parachain | ||
// sovereign account to fix this issue. when first xcm executed later on, the | ||
// sibling sovereign parachain account is deposit. and next transaction will | ||
// succeed even though second xcm is executed before first xcm if user fee is | ||
// less than parachain sovereign account balance. |
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.
Dispatchable calls docstring or pallet readme should include these usage notes.
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.
move to README
xtokens/src/lib.rs
Outdated
} | ||
|
||
/// Send xcm with given assets and fee to dest or reserve chain. | ||
fn send_xcm( |
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.
need a better naming
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.
rename to execute_and_send_reserve_kind_xcm
@@ -813,3 +920,18 @@ fn half(asset: &MultiAsset) -> MultiAsset { | |||
id: asset.id.clone(), | |||
} | |||
} | |||
|
|||
fn subtract_fee(asset: &MultiAsset, amount: u128) -> MultiAsset { | |||
let final_amount = fungible_amount(asset).checked_sub(amount).expect("fee too low; qed"); |
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.
There is no fee_amount >= min_xcm_fee
check. This could panic no?
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.
we checked before subtract, so it's ok here I think
// min xcm fee should less than user fee
let fee_to_dest: MultiAsset = (fee.id.clone(), min_xcm_fee).into();
ensure!(fee_to_dest < fee, Error::<T>::InvalidAsset);
checkout context infomation on: #698
for not breaking current semantic(don't allow different reserve), I use another disptach call instead mixed indo_transfer_multiassets
. (i.e. may be need another parametherallow_different_reserve
to implemented.)thetransfer_using_relaychain_as_fee
only used for parachain(A) send token to sibling parachain(B), and use relaychain token(R) as fee. thus, the reserve location is decided by sibling asset instead of fee asset.Edit:
for the case of transfer custom asset(RMRK) from Karura to Statemine, we current use two xcm implementation:
we also has the case of transfer RMRK from Karura to Bifrost. this case also need use KSM as fee too.acala statemine tests: AcalaNetwork/Acala#1712