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

support using KSM as fee(cases: transfer back asset to Statemine) #698

Closed
zqhxuyuan opened this issue Feb 19, 2022 · 9 comments
Closed

support using KSM as fee(cases: transfer back asset to Statemine) #698

zqhxuyuan opened this issue Feb 19, 2022 · 9 comments

Comments

@zqhxuyuan
Copy link
Contributor

zqhxuyuan commented Feb 19, 2022

current, we have multi assets support, and use fee_item asset as reserve

let (transfer_kind, dest, reserve, recipient) = Self::transfer_kind(&fee, &dest)?;

i.e. if currencies=vec![(CurrencyId::R, 100), (CurrencyId::B, 450)], and fee_item=0, reserve=Parent, because fee asset is R, then xcm sent to Relaychain likes:

origin: MultiLocation { parents: 0, interior: X1(Parachain(1)) }, 
message: Xcm([
    WithdrawAsset(MultiAssets([
        MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: Here }), fun: Fungible(100) }, 
        MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: X2(Parachain(2), GeneralKey([66])) }), fun: Fungible(450) }])), // 🔥
    ClearOrigin, 
    BuyExecution { fees: MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: Here }), fun: Fungible(50) }, weight_limit: Limited(40) }, 
    DepositReserveAsset { 
        assets: Wild(All), max_assets: 2, 
        dest: MultiLocation { parents: 0, interior: X1(Parachain(2)) }, 
        xcm: Xcm([
            BuyExecution { fees: MultiAsset { id: Concrete(MultiLocation { parents: 1, interior: Here }), fun: Fungible(50) }, weight_limit: Limited(40) }, 
            DepositAsset { assets: Wild(All), max_assets: 2, beneficiary: MultiLocation { parents: 0, interior: X1(AccountId32 { network: Any, id: [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1] }) } }]) }
]), weight_limit: 18446744073709551615, weight_credit: 0

this will error when execute second WithdrawAsset because relaychain doesn't have (0, Parachain(2), GeneralKey(B)) asset.


if currencies=vec![(CurrencyId::R, 100), (CurrencyId::B, 450)], and fee_item=1, reserve=(1,Parachain(2)), because fee asset is B, then xcm send to Sibling Parachain(2) likes:

origin: MultiLocation { parents: 0, interior: X1(Parachain(1)) }, 
message: Xcm([
    WithdrawAsset(MultiAssets([
        MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: X1(GeneralKey([66])) }), fun: Fungible(450) }, // sibling_a_account should have enougn B token
        MultiAsset { id: Concrete(MultiLocation { parents: 1, interior: Here }), fun: Fungible(100) }])), // sibling_a_account also should have enough R token
    ClearOrigin, 
    BuyExecution { 
        fees: MultiAsset { 
            id: Concrete(MultiLocation { parents: 0, interior: X1(GeneralKey([66])) }), fun: Fungible(450)  // ⬅️
        }, weight_limit: Limited(40) }, 
    DepositAsset { 
        assets: Wild(All), max_assets: 2, 
        beneficiary: MultiLocation { parents: 0, interior: X1(AccountId32 { network: Any, id: [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1] }) } 
	}
]), weight_limit: 6148914691236517205, weight_credit: 0

this can sucess if we open gate here(if not, there'll be DistinctReserveForAssetAndFee error):

ensure!(
fee.reserve() == asset.reserve(),
Error::<T>::DistinctReserveForAssetAndFee
);

and the xcm above use (0, GeneralKey(B)) as fee, although this is what the fee_item exactly wanted.


but in some cases we want use sibling parachain as reserve chain, and also use relaychain asset as fee.
i.e. transfer RMRK from Karura to Statemine, we use KSM as fee. the xcm on recipient parachain should be like:

message: Xcm([
    WithdrawAsset(MultiAssets([
        MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: X1(GeneralKey([66])) }), fun: Fungible(450) }, // sibling_a_account should have enougn B token
        MultiAsset { id: Concrete(MultiLocation { parents: 1, interior: Here }), fun: Fungible(100) }])), // sibling_a_account also should have enough R token
    ClearOrigin, 
    BuyExecution { 
        fees: MultiAsset { 
            id: Concrete(MultiLocation { parents: 1, interior: Here }), fun: Fungible(50)  // ⬅️
        }, weight_limit: Limited(40) }, 
    DepositAsset { 
        assets: Wild(All), max_assets: 2, 
        beneficiary: MultiLocation { parents: 0, interior: X1(AccountId32 { network: Any, id: [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1] }) } 
	}
])

PS: in the case of transfer RMRK from Karura to Statemine, if we transfer only RMRK, in the Statemine side, as trader not matched, xcm executed in Statemine will failed, and throw TooExpsensive error. so we must use KSM as fee, because Statemine only support KSM as fee for now.

https://github.com/paritytech/cumulus/blob/86f76c5619c64d1300315612695ad4b4fcd0f562/polkadot-parachains/statemine/src/lib.rs#L557

@girazoki
Copy link
Contributor

girazoki commented Feb 23, 2022

I have a question regarding this, which seems completely valid: will you allow KSM to be transferred directly from Statemine? Because if so I think you would need to map it to a different token than the one you have, and its kinda hard to do with the current executor layout (we would need to distinguish based on origin, because the multiasset is identical).

If not, in order for this approach to work your sovereign in Statemine needs KSM funds. WIll you manually (or the user) fund it?

@zqhxuyuan
Copy link
Contributor Author

zqhxuyuan commented Feb 23, 2022

@girazoki @xlc I think previous we've some discussion in this issue: #663

in order to transfer RMRK back to Statemine, I think we could allow fund KSM to parachain sovereign account on Statemine. but of course the weakness is If someone in Karura(sender parachain) set too much KSM as fee, the parachain sovereign account will soon be exhausted so that other user can't transfer RMRK back to Statemine. I'm not sure is this a potential problem?

A solution maybe: we don't allow user set the KSM fee, instead provide a fixed KSM fee(this could be done on Dapp frontend). and this fee is just enough for BuyExecution on Statemine. in this way, the user provide just enough KSM fee, and there're no additional KSM lefted for the user on Statemine.

but consider there is a issue reported: paritytech/cumulus#892 which allow Statemine transfer RMRK and use KRM as fee to Karura. and if that issue is fixed, that'll be problem too as you mentioned in case one(allow transfer KSM from Statemine). that should discuss more sufficient and need audit.

@girazoki
Copy link
Contributor

Yeah I remember having discussed this issue before. Seems fair to just allow the dapp to provide the fixed KSM fee, but if you have the polkadotXCM pallet enabled I think a user could craft the request in which they exhaust thes tatemine sovereign of KSM. I am unsure if this is a problem, as a user might potentially have to spend quite a bunch of KSM just to pause sending back RMRK to Statemine for a short period of time (until it is funded again)-

To be honest, I might be overthinking this problem, but it seems to me we need better support for common good parachains in general in the XCM ecosystem. We have this issue in which paritytech/cumulus#925 we communicate our concerns to parity, but not sure this will get improved.

Anyway, thanks for your response @zqhxuyuan! Really appreciated your clarification

@zqhxuyuan zqhxuyuan changed the title support using KSM as fee support using KSM as fee(cases: transfer back asset to Statemine) Feb 23, 2022
@girazoki
Copy link
Contributor

Just a heads up that https://github.com/paritytech/polkadot/pull/4756/files might solve our issues. If the transactor receives an origin parameter, we can simply mint a different token if the transfer came from Statemine, which would simplify all the issues we discussed above

@zqhxuyuan
Copy link
Contributor Author

zqhxuyuan commented Feb 23, 2022

if you have the polkadotXCM pallet enabled I think a user could craft the request

even though we can disable polkadotXCM by call filter as Acala current did, but as xTokens is still exposed with the new PR, I'm afraid we can't stop user/bot setting too much KSM fee when transfer asset back to Statemine.

unless we set fixed KSM fee(which relay on trader config on Statemine) in the code. I guess that's why bifrost @yrong previous did in bifrost-finance/bifrost#420 instead of specify fee amount to prevent above potential attack happend.

@yrong
Copy link

yrong commented Feb 23, 2022

Yes, remeber some discussion with @girazoki before in bifrost-finance/bifrost#420, ksm from statemine should not be mapped to the same token as from kusama. Our workround now is to deduct extra fee and then manually fund our sovereign account in statemine as compensation. I do think there should be some better solution.

@girazoki
Copy link
Contributor

girazoki commented Feb 23, 2022

I see, thanks for the clarification guys! that makes total sense.

I think there will be a way after xcm v3, since assetTransactors will receive the context of the call,which includes the origin. Therefore we can map the parent token to a different token depending if the origin is Statemine or the Relay.

@zqhxuyuan
Copy link
Contributor Author

zqhxuyuan commented Feb 25, 2022

After talk to @shaunxw and @xlc, we got two solution for this case before v3 released.

  1. one xcm direct send to Statemine, which need pre-fund relaychain asset to parachain account on Statemine, also let all unused fee to parachain account instead of recipient. but this still has the problem that parachain account will exhausted and need fund again. and second bad thing is the relaychain asset is unbalanced(sender parachain consume relaychain asset but parachain sovereign account on relaychain not changed).
  2. two xcm: first one send to relaychain and route to Statemine(A-[R]-B NonReserve case), transfer user's fee to parachain account on Statemine. second one like first solution(A-[R,B]-B, ToReserve case). but in this second solution, we don't need to pre-fund relaychain asset to parachain account on Statemine.

we choose second solution for now(detail see the PR), the bad thing for user is need more fee, the good thing for parachain is they would't need pre-fund anymore and balance between parachains and relaychain is balanced.

@xlc
Copy link
Member

xlc commented Mar 25, 2022

closed by #700

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

No branches or pull requests

4 participants