Skip to content

Remove unneeded error debug strings #2017

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 4, 2023
Merged

Remove unneeded error debug strings #2017

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
406cbd7
Remove unneeded error debug strings
serban300 Apr 3, 2023
ff33e6c
Remove DispatchBlobError from XcmBlobMessageDispatchResult::NotDispat…
serban300 Apr 4, 2023
b4a8115
Revert "Remove DispatchBlobError from XcmBlobMessageDispatchResult::N…
serban300 Apr 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 2 additions & 14 deletions bin/runtime-common/src/messages_xcm_extension.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ pub type XcmAsPlainPayload = sp_std::prelude::Vec<u8>;
pub enum XcmBlobMessageDispatchResult {
InvalidPayload,
Dispatched,
NotDispatched(#[codec(skip)] &'static str),
NotDispatched(#[codec(skip)] Option<DispatchBlobError>),
Copy link
Contributor

Choose a reason for hiding this comment

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

@serban300
so, can we get rid of skip and Option here now? if so, lets clean it
or do we use NotDispatched(None) somewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I double checked. The events are stored in an encoded form, so if we do #[codec(skip)], the DispatchBlobError won't be saved anyway. And it doesn't seem to be used elsewhere. I removed it. PTAL !

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant to leave it as:
NotDispatched(Option<DispatchBlobError>)
or
NotDispatched(DispatchBlobError)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, sorry, I misunderstood. NotDispatched(Option<DispatchBlobError>) and NotDispatched(DispatchBlobError) don't work. Because DispatchBlobError doesn't implement Encode/Decode. The only thing we could do here is to derive Encode, Decode for DispatchBlobError in the Polkadot repo. I can create a PR for that and see if it is accepted. But until then, would it be ok to use NotDispatched(#[codec(skip)] Option<DispatchBlobError>) ? Functionally, it's the same as now, but this way we can remove the error strings.

Copy link
Contributor

Choose a reason for hiding this comment

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

oki, lets remove &str stuff, and why do we need Option?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. Done. Reverted the 2nd commit. PTAL.

and why do we need Option

Deriving Decode needs a type that implements Default. Because for example when we derives Decode for NotDispatched(#[codec(skip)] Option<DispatchBlobError>), since Option<DispatchBlobError> is skipped at encoding time, it won't be present at decoding time and we wouldn't know what kind of DispatchBlobError to create. So we creates a NotDispatched(Default::default()). and DispatchBlobError doesn't implement Default either.

Copy link
Contributor

Choose a reason for hiding this comment

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

aha, ok, got it, thank you

}

/// [`XcmBlobMessageDispatch`] is responsible for dispatching received messages
Expand Down Expand Up @@ -106,24 +106,12 @@ impl<
XcmBlobMessageDispatchResult::Dispatched
},
Err(e) => {
let e = match e {
DispatchBlobError::Unbridgable => "DispatchBlobError::Unbridgable",
DispatchBlobError::InvalidEncoding => "DispatchBlobError::InvalidEncoding",
DispatchBlobError::UnsupportedLocationVersion =>
"DispatchBlobError::UnsupportedLocationVersion",
DispatchBlobError::UnsupportedXcmVersion =>
"DispatchBlobError::UnsupportedXcmVersion",
DispatchBlobError::RoutingError => "DispatchBlobError::RoutingError",
DispatchBlobError::NonUniversalDestination =>
"DispatchBlobError::NonUniversalDestination",
DispatchBlobError::WrongGlobal => "DispatchBlobError::WrongGlobal",
};
log::error!(
target: crate::LOG_TARGET_BRIDGE_DISPATCH,
"[XcmBlobMessageDispatch] DispatchBlob::dispatch_blob failed, error: {:?} - message_nonce: {:?}",
e, message.key.nonce
);
XcmBlobMessageDispatchResult::NotDispatched(e)
XcmBlobMessageDispatchResult::NotDispatched(Some(e))
},
};
MessageDispatchResult { unspent_weight: Weight::zero(), dispatch_level_result }
Expand Down