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

feat: better XCM fee management #528

Merged
merged 13 commits into from
Jun 15, 2023
Merged

feat: better XCM fee management #528

merged 13 commits into from
Jun 15, 2023

Conversation

ntn-x2
Copy link
Member

@ntn-x2 ntn-x2 commented Jun 2, 2023

Fixes https://github.com/KILTprotocol/ticket/issues/2566.

This PR changes the way XCM fees are handled for DIP. Before, the provider parachain sovereign account on the consumer chain was paying for the XCM fees, while now it is the sovereign account of the user that triggered the operation on the provider chain, to also pay for the fees on the receiver chain in whatever asset specified in the extrinsic. With XCM v3, it's not possible for a parachain to charge an interior location (e.g., with a DescendOrigin instruction), hence a new barrier and a new origin converter have been introduced, which are supposed to be used together to implement the feeing model DIP has envisioned.

Relevant changes

Reworked XcmRouterIdentityDispatcher

The XcmRouterIdentityDispatcher type used to be part of the pallet-dip-provider pallet. Because it is really specific to KILT, it has been moved into the kilt-dip-support crate.
Additionally, the router now packages the DIP XCM program with the following instructions:

  1. ExpectOrigin with the provider parachain origin, making sure the same XCM message cannot be sent by regular users via the XCM pallet.
  2. DescendOrigin with a new AccountId32 representing the dispatcher account ID on the provider chain.
  3. WithdrawAsset with the MultiAsset specified in the commit_identity extrinsic in the pallet-dip-provider pallet
  4. BuyExecution with the max Weight specified in the commit_identity extrinsic in the pallet-dip-provider pallet
  5. Transact with OriginKind::Native and the same weight limit as above
  6. RefundSurplus
  7. DepositAsset with any remaining assets in the holding registry back to the dispatcher's account on the consumer chain

In the demo runtimes and tests, the Account32Hash location converter has been used to convert the account ID information coming from the X2 junction containing the sibling provider parachain and the disptacher account.

Configurable origin for the pallet_dip_provider::commit_identity extrinsic

The CommitOrigin origin was already configurable, but now its Success result is required to implement the SubmitterInfo trait, which returns the AccountId of the tx submitter, so that the information can be used to compose the XCM message that will be sent to destination (specifically it is included in the DescendOrigin and DepositAsset instructions).
The trait has been implemented for the AccountId32 type, which is returned by the EnsureSigned origin check (which could be used for tests), and for the DidRawOrigin type, which is returned by the EnsureDidOrigin origin check (which has been used in the demo runtimes).

The AllowParachainProviderAsSubaccount barrier

For DIP to implement the feeing model devised here, where the user pays for the fees on both chain, a new barrier + origin converter must be integrated into the XCM configuration.
The AllowParachainProviderAsSubaccount barrier verifies that 1. the origin of the XCM is a parachain with an ID that is returned by the ProviderParaId generic type, and 2. the order of the instructions in the XCM program matches exactly what is expected in the DIP flow. If either the origin or the set of instructions is different, the barrier will return an error.
To extend existing barrier, there are two additional types: OkOrElseCheckForParachainProvider which tries to match the incoming XCM message against the new AllowParachainProviderAsSubaccount barrier if none of the previous barriers have succeeded (explicit approval), and ErrOrElseCheckForParachainProvider which tries to match the XCM message against the new barrier only if none of the previous barriers have failed (explicit rejection).

The AccountIdJunctionAsParachain origin converter

Since there is no way to convert an X2 junction of parachain + sub-account, the AccountIdJunctionAsParachain does just that. It takes the X2 junction of a sibling parachain + sub-account, if the parachain matches the ID given by the ProviderParaId type, and converts it into a ParachainOrigin.

The origin converter and the barrier are meant to be used together, otherwise configuring only one or the other opens the way to security issues on the consumer chain!

@@ -74,7 +75,7 @@ impl xcm_executor::Config for XcmConfig {
type IsTeleporter = ();
type MaxAssetsIntoHolding = ConstU32<64>;
type MessageExporter = ();
type OriginConverter = SiblingParachainAsNative<cumulus_pallet_xcm::Origin, RuntimeOrigin>;
type OriginConverter = AccountIdJunctionAsParachain<ConstU32<2_000>, cumulus_pallet_xcm::Origin, RuntimeOrigin>;
Copy link

@enddynayn enddynayn Jun 2, 2023

Choose a reason for hiding this comment

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

type OriginConverter = SovereignSignedViaLocation<(ForeignChainAliasAccount), RuntimeOrigin>;

What are the cons of using this type of configuration above?
Since DescentOrigin is modifying and extending the interior with the account. Can ForeignChainAliasAccount be used to convert the origin to a signed origin?

The consumer can filter out any calls and only allow the call function to be executed via XCM.

Copy link
Member Author

@ntn-x2 ntn-x2 Jun 7, 2023

Choose a reason for hiding this comment

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

The pallet expects a parachain origin, hence using SovereignSignedViaLocation won't work, unfrotunately.

pub UnitWeightCost: Weight = Weight::from_ref_time(1_000);
pub UniversalLocation: InteriorMultiLocation = Parachain(ParachainInfo::parachain_id().into()).into();
}

pub type Barrier = AllowTopLevelPaidExecutionFrom<Everything>;
pub type AssetTransactorLocationConverter = SiblingParachainConvertsVia<Sibling, AccountId>;
pub type Barrier = OkOrElseCheckForParachainProvider<AllowTopLevelPaidExecutionFrom<Nothing>, ConstU32<2_000>>;

Choose a reason for hiding this comment

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

Can this be accomplished with this?

type Barrier = WithComputedOrigin<(AllowTopLevelPaidExecutionFrom<DerivativeCustomerLocations>);

Copy link
Member Author

Choose a reason for hiding this comment

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

As we discussed, the design at the time of this comment was flawed. I have now added a ExpectOrigin instruction early on (d683a3f). For now, this is a sufficiently good approach, which means that the WithComputedOrigin barrier would not let the call through, which is not what we want. Also the AllowTopLevelPaidExecutionFrom would fail. There is no way around using the OkOrElseCheckForParachainProvider barrier provided here. If some barrier must explicitly fail, the ErrOrElseCheckForParachainProvider should be used.

@ntn-x2 ntn-x2 marked this pull request as draft June 5, 2023 09:55
@ntn-x2 ntn-x2 marked this pull request as ready for review June 8, 2023 07:29
@ntn-x2 ntn-x2 changed the title feat: proper XCM fee management feat: better XCM fee management Jun 8, 2023
ntn-x2 added a commit to KILTprotocol/kilt-did-utilities that referenced this pull request Jun 15, 2023
@ntn-x2 ntn-x2 merged commit b92f628 into aa/dip Jun 15, 2023
@ntn-x2 ntn-x2 deleted the aa/fee-management branch June 15, 2023 08:12
ntn-x2 added a commit that referenced this pull request Dec 14, 2023
Feature branch for everything DIP. It will collect other PRs until we
are happy with the features, and will add the DIP to some of our
runtimes and merge this into `develop`.

## WIP Checklist for the open tasks for v1

- [x] Basic structure ->
#489
- [x] Merkleization of DID Documents ->
#492
- [x] `RuntimeCall` verification logic ->
#502
- [x] DID signature verification ->
#516
- [x] Add support for linked accounts and web3name ->
#525
- [x] Configurable origin for `commit_identity` ->
#526
- [x] Proper fee management ->
#528
- [x] Update to Polkadot 0.9.43 ->
c18a6ce
- [x] Replace XCM with state proofs ->
#543
- [x] Add support for relaychain consumer ->
#553 (part of
#543)
- [x] Proper error handling ->
#572
- [x] Add support for versioning ->
#573
- [x] Take deposits for identity commitments ->
#574
- [x] Expose common definitions usable by consumers ->
#577
- [x] Change ensure_signed! to configurable origin also for the
`dispatch_as` function ->
#577
- [x] Proper benchmarking and weights ->
#585
- [x] Comments and docs ->
#584
- [x] Revert Dockerfile changes in
#587
- [x] [OPTIONAL] Add support for Zombienet ->
#587
- [x] [OPTIONAL] Add chain spec loading from file for template runtimes
-> #587
- [x] Big, final review ->
#494 (review)
- [x] Improvements n.1 PR ->
#591
- [x] Improvements n.2 PR ->
#592
- [x] Add to Peregrine runtime ->
#594
- [ ] Deploy on Peregrine
- [ ] Unit tests
- [ ] Add to Spiritnet runtime
- [ ] Deploy on Spiritnet
- [ ] [OPTIONAL] Move DIP-related stuff into its own repo

---------

Co-authored-by: Adel Golghalyani <48685760+Ad96el@users.noreply.github.com>
Co-authored-by: Chris Chinchilla <chris@chrischinchilla.com>
Co-authored-by: Albrecht <albrecht@kilt.io>
webguru9178 pushed a commit to webguru9178/kilt-node that referenced this pull request Jan 8, 2024
Feature branch for everything DIP. It will collect other PRs until we
are happy with the features, and will add the DIP to some of our
runtimes and merge this into `develop`.

## WIP Checklist for the open tasks for v1

- [x] Basic structure ->
KILTprotocol/kilt-node#489
- [x] Merkleization of DID Documents ->
KILTprotocol/kilt-node#492
- [x] `RuntimeCall` verification logic ->
KILTprotocol/kilt-node#502
- [x] DID signature verification ->
KILTprotocol/kilt-node#516
- [x] Add support for linked accounts and web3name ->
KILTprotocol/kilt-node#525
- [x] Configurable origin for `commit_identity` ->
KILTprotocol/kilt-node#526
- [x] Proper fee management ->
KILTprotocol/kilt-node#528
- [x] Update to Polkadot 0.9.43 ->
KILTprotocol/kilt-node@c18a6ce
- [x] Replace XCM with state proofs ->
KILTprotocol/kilt-node#543
- [x] Add support for relaychain consumer ->
KILTprotocol/kilt-node#553 (part of
KILTprotocol/kilt-node#543)
- [x] Proper error handling ->
KILTprotocol/kilt-node#572
- [x] Add support for versioning ->
KILTprotocol/kilt-node#573
- [x] Take deposits for identity commitments ->
KILTprotocol/kilt-node#574
- [x] Expose common definitions usable by consumers ->
KILTprotocol/kilt-node#577
- [x] Change ensure_signed! to configurable origin also for the
`dispatch_as` function ->
KILTprotocol/kilt-node#577
- [x] Proper benchmarking and weights ->
KILTprotocol/kilt-node#585
- [x] Comments and docs ->
KILTprotocol/kilt-node#584
- [x] Revert Dockerfile changes in
KILTprotocol/kilt-node#587
- [x] [OPTIONAL] Add support for Zombienet ->
KILTprotocol/kilt-node#587
- [x] [OPTIONAL] Add chain spec loading from file for template runtimes
-> KILTprotocol/kilt-node#587
- [x] Big, final review ->
KILTprotocol/kilt-node#494 (review)
- [x] Improvements n.1 PR ->
KILTprotocol/kilt-node#591
- [x] Improvements n.2 PR ->
KILTprotocol/kilt-node#592
- [x] Add to Peregrine runtime ->
KILTprotocol/kilt-node#594
- [ ] Deploy on Peregrine
- [ ] Unit tests
- [ ] Add to Spiritnet runtime
- [ ] Deploy on Spiritnet
- [ ] [OPTIONAL] Move DIP-related stuff into its own repo

---------

Co-authored-by: Adel Golghalyani <48685760+Ad96el@users.noreply.github.com>
Co-authored-by: Chris Chinchilla <chris@chrischinchilla.com>
Co-authored-by: Albrecht <albrecht@kilt.io>
Ad96el added a commit that referenced this pull request Feb 7, 2024
Feature branch for everything DIP. It will collect other PRs until we
are happy with the features, and will add the DIP to some of our
runtimes and merge this into `develop`.

## WIP Checklist for the open tasks for v1

- [x] Basic structure ->
#489
- [x] Merkleization of DID Documents ->
#492
- [x] `RuntimeCall` verification logic ->
#502
- [x] DID signature verification ->
#516
- [x] Add support for linked accounts and web3name ->
#525
- [x] Configurable origin for `commit_identity` ->
#526
- [x] Proper fee management ->
#528
- [x] Update to Polkadot 0.9.43 ->
c18a6ce
- [x] Replace XCM with state proofs ->
#543
- [x] Add support for relaychain consumer ->
#553 (part of
#543)
- [x] Proper error handling ->
#572
- [x] Add support for versioning ->
#573
- [x] Take deposits for identity commitments ->
#574
- [x] Expose common definitions usable by consumers ->
#577
- [x] Change ensure_signed! to configurable origin also for the
`dispatch_as` function ->
#577
- [x] Proper benchmarking and weights ->
#585
- [x] Comments and docs ->
#584
- [x] Revert Dockerfile changes in
#587
- [x] [OPTIONAL] Add support for Zombienet ->
#587
- [x] [OPTIONAL] Add chain spec loading from file for template runtimes
-> #587
- [x] Big, final review ->
#494 (review)
- [x] Improvements n.1 PR ->
#591
- [x] Improvements n.2 PR ->
#592
- [x] Add to Peregrine runtime ->
#594
- [ ] Deploy on Peregrine
- [ ] Unit tests
- [ ] Add to Spiritnet runtime
- [ ] Deploy on Spiritnet
- [ ] [OPTIONAL] Move DIP-related stuff into its own repo

---------

Co-authored-by: Adel Golghalyani <48685760+Ad96el@users.noreply.github.com>
Co-authored-by: Chris Chinchilla <chris@chrischinchilla.com>
Co-authored-by: Albrecht <albrecht@kilt.io>
Ad96el added a commit that referenced this pull request Apr 2, 2024
Feature branch for everything DIP. It will collect other PRs until we
are happy with the features, and will add the DIP to some of our
runtimes and merge this into `develop`.

## WIP Checklist for the open tasks for v1

- [x] Basic structure ->
#489
- [x] Merkleization of DID Documents ->
#492
- [x] `RuntimeCall` verification logic ->
#502
- [x] DID signature verification ->
#516
- [x] Add support for linked accounts and web3name ->
#525
- [x] Configurable origin for `commit_identity` ->
#526
- [x] Proper fee management ->
#528
- [x] Update to Polkadot 0.9.43 ->
c18a6ce
- [x] Replace XCM with state proofs ->
#543
- [x] Add support for relaychain consumer ->
#553 (part of
#543)
- [x] Proper error handling ->
#572
- [x] Add support for versioning ->
#573
- [x] Take deposits for identity commitments ->
#574
- [x] Expose common definitions usable by consumers ->
#577
- [x] Change ensure_signed! to configurable origin also for the
`dispatch_as` function ->
#577
- [x] Proper benchmarking and weights ->
#585
- [x] Comments and docs ->
#584
- [x] Revert Dockerfile changes in
#587
- [x] [OPTIONAL] Add support for Zombienet ->
#587
- [x] [OPTIONAL] Add chain spec loading from file for template runtimes
-> #587
- [x] Big, final review ->
#494 (review)
- [x] Improvements n.1 PR ->
#591
- [x] Improvements n.2 PR ->
#592
- [x] Add to Peregrine runtime ->
#594
- [ ] Deploy on Peregrine
- [ ] Unit tests
- [ ] Add to Spiritnet runtime
- [ ] Deploy on Spiritnet
- [ ] [OPTIONAL] Move DIP-related stuff into its own repo

---------

Co-authored-by: Adel Golghalyani <48685760+Ad96el@users.noreply.github.com>
Co-authored-by: Chris Chinchilla <chris@chrischinchilla.com>
Co-authored-by: Albrecht <albrecht@kilt.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants