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

relay account and tests #651

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

relay account and tests #651

wants to merge 16 commits into from

Conversation

zqhxuyuan
Copy link
Contributor

@zqhxuyuan zqhxuyuan commented Nov 10, 2021

AllowRelayedPaidExecutionFromParent is needed because in the case of relaychain account send Transact to parachain, then DescendOrigin is automatically added as the first instruction sended to parachain. so it's used as Barrier check to allow this situation passing through.

RelaychainAccountId32Aliases is used as convert relaychain account to account controlled on parachain. and it's related to above case when DescendOrigin append origin(which is Parent) with Account which result to (Parent, Account).

closes #636

@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2021

Codecov Report

Merging #651 (4177cd6) into master (d69f226) will decrease coverage by 0.03%.
The diff coverage is 73.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #651      +/-   ##
==========================================
- Coverage   76.02%   75.99%   -0.04%     
==========================================
  Files          78       78              
  Lines        6557     6644      +87     
==========================================
+ Hits         4985     5049      +64     
- Misses       1572     1595      +23     
Impacted Files Coverage Δ
xtokens/src/mock/para.rs 52.77% <ø> (ø)
xtokens/src/tests.rs 100.00% <ø> (ø)
xcm-support/src/lib.rs 58.92% <50.00%> (-26.79%) ⬇️
xcm-support/src/tests.rs 93.97% <93.02%> (-1.03%) ⬇️
xtokens/src/mock/mod.rs 89.58% <100.00%> (+1.21%) ⬆️
benchmarking/src/lib.rs 30.08% <0.00%> (ø)
authority/src/mock.rs 75.51% <0.00%> (+2.04%) ⬆️
xcm/src/lib.rs 80.00% <0.00%> (+5.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d69f226...4177cd6. Read the comment docs.

Copy link
Member

@shaunxw shaunxw left a comment

Choose a reason for hiding this comment

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

Any reason to add this struct in orml-xcm-support? It looks like a runtime config to me.

@xlc
Copy link
Member

xlc commented Nov 10, 2021

it is something to be used by all the runtime and potentially used by other parachains.
Like https://github.com/paritytech/polkadot/blob/54d1cb9178fd41b535d8d908b021926d01d91f00/xcm/xcm-builder/src/location_conversion.rs#L104

xcm-support/src/lib.rs Outdated Show resolved Hide resolved
xcm-support/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 103 to 107
Ok(AccountId32 {
id: who.into(),
network: Network::get(),
}
.into())
Copy link
Contributor

Choose a reason for hiding this comment

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

This will reverse to MultiLocation { parents: 0, interior: X1(AccountId32), }, no?
That means it will not convert back to the input if calling RelaychainAccountId32Aliases.convert(location).reverse() which would be desirable, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a good point, as the reverse path is not currently go through, I may miss here.

Copy link
Contributor Author

@zqhxuyuan zqhxuyuan Nov 13, 2021

Choose a reason for hiding this comment

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

after I change here to return (1, AccountId32).into(). If we config (...,AccountId32Aliases,RelaychainAccountId32Aliases), then reverse on AccountId will always return (parents:0, X1(AccountId32)). But when I change the order to (...,RelaychainAccountId32Aliases, AccountId32Aliases), then reverse on AccountId will always return (parents:1, X1(AccountId32)). this sounds no better solution if those two config has same trait bounds, unless we make them different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've post an issue on polkadot: paritytech/polkadot#4296. would you mind to see if that's a potential problem? @KiChjang @apopiak

xcm-support/src/lib.rs Show resolved Hide resolved
xcm-support/src/lib.rs Outdated Show resolved Hide resolved
xcm-support/src/lib.rs Outdated Show resolved Hide resolved
xcm-support/src/lib.rs Outdated Show resolved Hide resolved
xcm-support/src/lib.rs Outdated Show resolved Hide resolved
xcm-support/src/lib.rs Outdated Show resolved Hide resolved
xtokens/Cargo.toml Outdated Show resolved Hide resolved
xcm-support/src/lib.rs Outdated Show resolved Hide resolved
xcm-support/src/lib.rs Outdated Show resolved Hide resolved
xtokens/src/mock/tests.rs Outdated Show resolved Hide resolved
xtokens/src/tests.rs Outdated Show resolved Hide resolved
xtokens/src/tests.rs Outdated Show resolved Hide resolved
xcm-support/src/lib.rs Outdated Show resolved Hide resolved
@zqhxuyuan
Copy link
Contributor Author

zqhxuyuan commented Nov 16, 2021

current the xcm format is WithdrawAsset | BuyExecution(limited) | Transact, this means the asset of relaychain acount(say 1 KSM) will all be first move to holding, then payment fee will be buyed for execution(say 0.1 KSM), after that unspent asset(0.9 KSM) will be in holding again. and As the xcm has its AssetTrap mechanism, the unspent asset will be lefted in the storage for later claim. Do we need add another DepositAsset to do this for users or let user to self-decide? @xlc

in my testcase the event below shows that there're 0.016 used as fee payment, and 0.98 KSM lefted in AssetsTrapped:

Event::Currencies(Event::Deposited(CurrencyId::Token(TokenSymbol::KSM), ... (5EYCAe5f...), 16128000000))

Event::PolkadotXcm(Event::AssetsTrapped(...., MultiLocation { parents: 1, interior: Here }, 
V1(MultiAssets([MultiAsset { id: Concrete(MultiLocation { parents: 1, interior: Here }),
fun: Fungible(983872000000) }]))))

xcm-support/src/tests.rs Outdated Show resolved Hide resolved
@xlc
Copy link
Member

xlc commented Nov 16, 2021

Yeah we should allow DepositAsset the unused fees back.

Also maybe should support ReserveAssetDeposited so it is possible to send some funds from relay chain for the tx fee. Otherwise user needs to fund the account on parachain first.

@zqhxuyuan
Copy link
Contributor Author

zqhxuyuan commented Nov 18, 2021

Yeah we should allow DepositAsset the unused fees back.

Also maybe should support ReserveAssetDeposited so it is possible to send some funds from relay chain for the tx fee. Otherwise user needs to fund the account on parachain first.

it's easy add ReserveAssetDeposited to match case inside AllowRelayedPaidExecutionFromParent, but construct a xcm in relaychain side currently didn't work.

e.g. relaychain has xcm format TransferReserverAsset{xcm: BuyExecution | Transact | DepositAsset }, then parachain receives: ReserveAssetDeposited | ClearOrigin | BuyExecution | Transact | DepositAsset. As the Transact instruction need origin not null, but previous ClearOrigin set origin to None, this will cause BadOrigin error in parachain.

let alice = X1(Junction::AccountId32 {
    network: NetworkId::Kusama,
    id: ALICE.into(),
});
let call = Call::Balances(pallet_balances::Call::<Runtime>::transfer {
    dest: BOB.into(),
    value: 500,
});
Relay::execute_with(|| {
    let _ = RelayBalances::deposit_creating(&ALICE, 20_000);

    let xcm = vec![
        TransferReserveAsset {
            assets: (Here, 10_000).into(),
            dest: Parachain(1).into(),
            xcm: Xcm::<()>(vec![
                BuyExecution {
                    fees: (Parent, 10_000).into(),
                    weight_limit: Limited(6050 as u64),
                },
                Transact {
                    origin_type: OriginKind::SovereignAccount,
                    require_weight_at_most: 6_000 as u64,
                    call: call.encode().into(),
                },
                DepositAsset {
                    assets: All.into(),
                    max_assets: 1,
                    beneficiary: (0, alice.clone()).into(),
                }
            ]),
        },
    ];
    XcmExecutor::<relay::XcmConfig>::execute_xcm_in_credit(alice, Xcm(xcm), 10, 10);
});

also If change Transact to WithdrawAsset isn't going to work. because WithdrawAsset also needs origin is not null.

let alice = X1(Junction::AccountId32 {
    network: NetworkId::Kusama,
    id: ALICE.into(),
});
let bob = X1(Junction::AccountId32 {
    network: NetworkId::Kusama,
    id: BOB.into(),
});

Relay::execute_with(|| {
    let _ = RelayBalances::deposit_creating(&ALICE, 20_000);
    assert_eq!(21_000, RelayBalances::free_balance(&ALICE));

    let xcm = vec![
        TransferReserveAsset {
            assets: (Here, 10_000).into(),
            dest: Parachain(1).into(),
            xcm: Xcm::<()>(vec![
                BuyExecution {
                    fees: (Parent, 10_000).into(), // use relaychain asset as fee payment
                    weight_limit: Limited(6050 as u64),
                },
                WithdrawAsset((Here, 500).into()), // withdraw Alice on parachain
                // BuyExecution may add here too
                DepositAsset {
                    assets: All.into(),
                    max_assets: 1,
                    beneficiary: (0, bob).into(), // deposit to Bob on parachain
                }
            ]),
        },
    ];
    XcmExecutor::<relay::XcmConfig>::execute_xcm_in_credit(alice, Xcm(xcm), 10, 10);
});

if relaychain xcm format is WithdrawAsset | DepositReserveAsset{ xcm: BuyExecution | Transact | DepositAsset }, in the parachain it also has this problem too.

max_weight: Weight,
_weight_credit: &mut Weight,
) -> Result<(), ()> {
ensure!(origin.contains_parents_only(1), ());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to make this a configurable generic that implements Contains<MultiLocation> ?
We have a use-case for allowing sibling chains to send Transact instructions to us.

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.

RelayChainAccountId32Aliases
7 participants