xcm: fix for DenyThenTry Barrier #7169
Conversation
| @@ -504,3 +506,36 @@ impl ShouldExecute for DenyReserveTransferToRelayChain { | |||
| Ok(()) | |||
| } | |||
| } | |||
|
|
|||
| // See https://github.com/paritytech/polkadot-sdk/pull/6838 | |||
There was a problem hiding this comment.
please, add here the real description
| @@ -504,3 +506,36 @@ impl ShouldExecute for DenyReserveTransferToRelayChain { | |||
| Ok(()) | |||
| } | |||
| } | |||
|
|
|||
| // See https://github.com/paritytech/polkadot-sdk/pull/6838 | |||
| pub struct DenyFirstExportMessageFrom<FromOrigin, ToGlobalConsensus>( | |||
There was a problem hiding this comment.
this is pretty specific barrier for BridgeHubs, so I would move it with tests to the bridge-hub-common
| _max_weight: Weight, | ||
| _properties: &mut Properties, | ||
| ) -> Result<(), ProcessMessageError> { | ||
| message.matcher().match_next_inst_while( |
There was a problem hiding this comment.
I think that match_next_inst_while with ControlFlow::Continue checks all the instructions, so the name DenyFirstExportMessageFrom is not correct, so let's better use DenyExportMessage.
I am just thinking, that my initial advice DenyFirst was wrong, if there are multiple ExportMessage instructions, we should check them all for sure.
| /// Can be amalgamated into a tuple to have multiple trials. If any of the tuple elements returns | ||
| /// `Err(())`, the execution stops. Else, `Ok(_)` is returned if all elements accept the message. | ||
| pub trait ShouldNotExecute { | ||
| /// Returns `Ok(())` if the given `message` may be executed. |
There was a problem hiding this comment.
I think this description is copied from ShouldExecute. Please revise it to something like: "Ok(()) means there is no reason not to execute, while Err(e) indicates there is a reason not to execute."
| fn should_not_execute<RuntimeCall>( | ||
| origin: &Location, | ||
| instructions: &mut [Instruction<RuntimeCall>], | ||
| max_weight: Weight, | ||
| properties: &mut Properties, | ||
| ) -> Result<(), ProcessMessageError>; |
There was a problem hiding this comment.
Maybe fn should_not_execute<RuntimeCall>(..) -> Option<ProcessMessageError> would be a bit more readable. In this case, Option could represent: Some(reason) if there is a reason why it should not be executed, and None if there is no reason to deny execution.
Currently, when we ask Barrier::should_not_execute(message) and the answer is Ok(()), it’s unclear what Ok(()) means. Does it imply it’s okay not to execute, or that it’s okay to execute?
What about renaming trait ShouldNotExecute { fn should_not_execute() -> Result } to something like trait DenyExecution { fn deny_execution() -> Option<ProcessMessageError> }? This would make the intent more explicit?
Maybe it’s fine as it is now, but perhaps it’s just my language barrier or the challenge of double negations... I’d definitely appreciate other suggestions here! :D
Please, do not change anything, let's keep this renaming as a last open point.
There was a problem hiding this comment.
haha, I found similar comment in the very old PR: paritytech/cumulus#1169 (comment)
There was a problem hiding this comment.
Agreed the naming and what is returned might be confusing
|
@yrong overall looks good so far, I was also thinking about another trait for that like this |
…adot-sdk into fix-for-deny-then-try
| fn should_not_execute<RuntimeCall>( | ||
| origin: &Location, | ||
| instructions: &mut [Instruction<RuntimeCall>], | ||
| max_weight: Weight, | ||
| properties: &mut Properties, | ||
| ) -> Result<(), ProcessMessageError>; |
There was a problem hiding this comment.
Agreed the naming and what is returned might be confusing
Co-authored-by: Francisco Aguirre <[email protected]>
Resolves: #7148
This PR changes the behavior of
DenyThenTryfrom the patternDenyIfAllMatchtoDenyIfAnyMatchfor the tuple.I would expect the latter is the right behavior so make the fix in place, but we can also add a dedicated Impl with the legacy one untouched if necessary.