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

Remove dispatch_result field #1660

Merged
merged 5 commits into from
Nov 22, 2022
Merged

Remove dispatch_result field #1660

merged 5 commits into from
Nov 22, 2022

Conversation

svyatonik
Copy link
Contributor

@svyatonik svyatonik added P-Runtime PR-audit-needed A PR has to be audited before going live. PR-breaksruntime A PR that is going to break runtime bridge compatibility. We need to be careful with upgrade. labels Nov 22, 2022
/// the target chain to the message submitter at the source chain. If you're using immediate
/// call dispatcher, then it'll be result of the dispatch - `true` if dispatch has succeeded
/// and `false` otherwise.
pub dispatch_result: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

@svyatonik
so this is now kind of "fire and forget"?
so, we dont want to add some information if dispatch was ok/failed to the Event::MessagesDelivered ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. In our previous API, the messaging pallet was the entrypoint for all messages, so we were building applications around that palet (e.g. atomic token swap - https://github.com/paritytech/parity-bridges-common/tree/c7c6f0ce5e88b3ba9cdcd817f4286e76221b8222/modules/token-swap). There we were actually using this (one-bit) dispatch_result to see if our call has succeeded or not.

Now the XCM infra is the central point, so applications are built around XCM. We are just transport, so no extra logic is required. E.g. XCM has its own mechanism to send responses back to the caller (see ReportHolding, ReportTransactStatus, ReportError XCM instructions). Hence we don't need our own mechanism anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, got it, thank you :)

Copy link
Contributor

@bkontur bkontur left a comment

Choose a reason for hiding this comment

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

lgtm

@svyatonik svyatonik enabled auto-merge (squash) November 22, 2022 13:23
@svyatonik svyatonik merged commit cf81034 into master Nov 22, 2022
@svyatonik svyatonik deleted the remove-dispatch-result branch November 22, 2022 13:56
serban300 added a commit to serban300/parity-bridges-common that referenced this pull request Apr 7, 2023
serban300 added a commit that referenced this pull request Apr 10, 2023
* Use an actual Result inside MessageDispatchResult

We need this in order to distinguish between Ok and Err

* Revert #1660

* Fixes + simplifications

* Implement review suggestions
jiguantong pushed a commit to darwinia-network/darwinia-messages-substrate that referenced this pull request Apr 19, 2023
serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Mar 27, 2024
* remove dispatch_result field

* fix benchmarks
serban300 added a commit to serban300/parity-bridges-common that referenced this pull request Mar 27, 2024
* Use an actual Result inside MessageDispatchResult

We need this in order to distinguish between Ok and Err

* Revert paritytech#1660

* Fixes + simplifications

* Implement review suggestions
serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Apr 8, 2024
* remove dispatch_result field

* fix benchmarks
serban300 added a commit to serban300/parity-bridges-common that referenced this pull request Apr 8, 2024
* Use an actual Result inside MessageDispatchResult

We need this in order to distinguish between Ok and Err

* Revert paritytech#1660

* Fixes + simplifications

* Implement review suggestions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-Runtime PR-audit-needed A PR has to be audited before going live. PR-breaksruntime A PR that is going to break runtime bridge compatibility. We need to be careful with upgrade.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants