Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Companion for polkadot#7234 (XCM: Tools for uniquely referencing messages) #2601

Merged
merged 33 commits into from
May 25, 2023

Conversation

gavofyork
Copy link
Member

@gavofyork gavofyork commented May 19, 2023

Companion to fix for new API. Also moves DenyThenTry and DenyReserveTransferToRelayChain into Polkadot.

This revamps some of the *MPQ events to correctly report both the message hash and ID. It also moves the *MPQ code over to use ExecuteXcm::prepare_and_execute rather than the older APIs, which required simplifying some tests since the new API is at a slightly lower level.

  • Changes of message hash/ID should be respected in any events deposited
    • DMP arrival/dispatch (Cumulus)
    • XCMP arrival/dispatch (Cumulus)
    • Consider adding the ID into existing send events
  • Integrate into parachains (Cumulus)

@gavofyork gavofyork marked this pull request as draft May 19, 2023 12:12
@paritytech-ci paritytech-ci requested review from a team May 19, 2023 12:12
@gavofyork gavofyork added A3-in_progress Pull request is in progress. No review needed at this stage. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited T1-runtime This PR/Issue is related to the topic “runtime”. labels May 19, 2023
@gavofyork gavofyork marked this pull request as ready for review May 19, 2023 15:20
@gavofyork gavofyork added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels May 19, 2023
@bkontur
Copy link
Contributor

bkontur commented May 23, 2023

@gavofyork
there is one older TODO here

    // We aren't able to track the XCM that initiated the fee deposit, so we create a
    // fake message hash here
    &XcmContext::with_message_hash([0; 32]),

could this be fixed now with this new message_id/topic stuff?
my guess is maybe to change trait for trader here (extends with context: &XcmContext),

wdyt?

@gavofyork
Copy link
Member Author

could this be fixed now with this new message_id/topic stuff?

These changes don't much help this.

In fact, the functions inside WeightTrader and TakeRevenue and perhaps some other hook-traits used by XcmExecutor should all take XcmContext. This is quite a substantial change in APIs so should get its own issue and PR. Feel free to create the issue.

pallets/dmp-queue/src/lib.rs Outdated Show resolved Hide resolved
@paritytech-ci paritytech-ci requested a review from a team May 25, 2023 11:07
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

approval stamp to unblock the companion check

@gavofyork gavofyork merged commit 10afc2f into master May 25, 2023
@gavofyork gavofyork deleted the gav-unique-topics branch May 25, 2023 15:52
ordian added a commit that referenced this pull request May 25, 2023
* master:
  Companion for polkadot#7234 (XCM: Tools for uniquely referencing messages) (#2601)
  bump substrate version (#2640)
  bump zombienet version (#2637)
@redzsina redzsina added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited. and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Aug 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants