relay account and tests by zqhxuyuan · Pull Request #651 · open-web3-stack/open-runtime-module-library

zqhxuyuan · 2021-11-10T09:23:02Z

add RelaychainAccountId32Aliases to xcm-support
add AllowRelayedPaidExecutionFromParent to xcm-support
add unit testcase
add integration test in acala: relay account transact AcalaNetwork/Acala#1614

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 · 2021-11-10T09:50:05Z

Codecov Report

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

@@            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.

shaunxw

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

xlc · 2021-11-10T23:23:18Z

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

apopiak · 2021-11-12T15:17:20Z

xcm-support/src/lib.rs

+		Ok(AccountId32 {
+			id: who.into(),
+			network: Network::get(),
+		}
+		.into())


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?

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

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.

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

xtokens/Cargo.toml

xcm-support/src/lib.rs

xtokens/src/mock/tests.rs

xtokens/src/tests.rs

xcm-support/src/lib.rs

zqhxuyuan · 2021-11-16T08:44:43Z

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

xlc · 2021-11-16T22:47:12Z

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 · 2021-11-18T02:47:45Z

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.

ghzlatarev · 2022-06-13T17:50:44Z

xcm-support/src/lib.rs

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


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.

relay account and tests

7421f97

zqhxuyuan requested review from shaunxw and xlc November 10, 2021 09:23

zqhxuyuan mentioned this pull request Nov 10, 2021

RelayChainAccountId32Aliases #636

Open

shaunxw requested changes Nov 10, 2021

View reviewed changes

zqhxuyuan added 3 commits November 11, 2021 08:20

convert origin unit test

514566b

add AllowDescendOrigin and integration test

1f7d92a

sibling parachain test

d132663

KiChjang reviewed Nov 11, 2021

View reviewed changes

xcm-support/src/lib.rs Outdated Show resolved Hide resolved

KiChjang reviewed Nov 11, 2021

View reviewed changes

xcm-support/src/lib.rs Outdated Show resolved Hide resolved

zqhxuyuan added 3 commits November 12, 2021 08:15

add doc

f4162e3

add transfer test and change to AllowEquivalentAccountsFrom

ced4d67

clippy

cbb4944

apopiak reviewed Nov 12, 2021

View reviewed changes

fix test

c3b4d7d

KiChjang reviewed Nov 13, 2021

View reviewed changes

xcm-support/src/lib.rs Outdated Show resolved Hide resolved

allow network any

61d40f5

xlc reviewed Nov 16, 2021

View reviewed changes

xcm-support/src/lib.rs Outdated Show resolved Hide resolved

add IsParent

1cb1abf

shaunxw reviewed Nov 16, 2021

View reviewed changes

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

zqhxuyuan added 2 commits November 16, 2021 13:38

clean test

23e1a9f

clippy

e1467ba

KiChjang reviewed Nov 16, 2021

View reviewed changes

xcm-support/src/lib.rs Outdated Show resolved Hide resolved

zqhxuyuan mentioned this pull request Nov 16, 2021

LocationToAccountId reverse AccountId to Location can't distinguish paritytech/polkadot#4296

Closed

zqhxuyuan added 2 commits November 16, 2021 14:52

rename

9cffcc5

fmt

631659b

zqhxuyuan mentioned this pull request Nov 16, 2021

relay account transact AcalaNetwork/Acala#1614

Draft

xlc reviewed Nov 16, 2021

View reviewed changes

xcm-support/src/tests.rs Outdated Show resolved Hide resolved

zqhxuyuan added 2 commits November 18, 2021 17:55

support withdraw and deposit after buy_execution

b1a2a90

rm withdraw and more tests

4177cd6

0x8f701 mentioned this pull request Apr 11, 2022

TransactFilter for allowing users to call parachain call on relaychain paritytech/polkadot-sdk#846

Open

zqhxuyuan mentioned this pull request Jun 13, 2022

Transfer with Transact Design Discussion #766

Open

ghzlatarev reviewed Jun 13, 2022

View reviewed changes

Conversation

zqhxuyuan commented Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

shaunxw left a comment

Choose a reason for hiding this comment

Uh oh!

xlc commented Nov 10, 2021

Uh oh!

Uh oh!

Uh oh!

apopiak Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

zqhxuyuan Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

zqhxuyuan Nov 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zqhxuyuan Nov 16, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zqhxuyuan commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

xlc commented Nov 16, 2021

Uh oh!

zqhxuyuan commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghzlatarev Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

zqhxuyuan commented Nov 10, 2021 •

edited

Loading

codecov-commenter commented Nov 10, 2021 •

edited

Loading

zqhxuyuan Nov 13, 2021 •

edited

Loading

zqhxuyuan commented Nov 16, 2021 •

edited

Loading

zqhxuyuan commented Nov 18, 2021 •

edited

Loading