[FRAME] MQ processor should be transactional #5198
Conversation
|
The CI pipeline was cancelled due to failure one of the required jobs. |
…' into dami/mq-transactional-processing
| @@ -164,6 +164,7 @@ impl ProcessMessage for RecordingMessageProcessor { | |||
| meter: &mut WeightMeter, | |||
| _id: &mut [u8; 32], | |||
| ) -> Result<bool, ProcessMessageError> { | |||
| sp_io::storage::set(b"transactional_storage", &vec![1, 2, 3]); | |||
There was a problem hiding this comment.
The mocked process supports callbacks - no need to hard code it here.
Check out the tests that use it:
Callback::set(Box::new(|_, _| {
// whatever youwant
}));
prdoc/pr_5198.prdoc
Outdated
|
|
||
| crates: | ||
| - name: pallet-message-queue | ||
| bump: patch |
There was a problem hiding this comment.
| bump: patch | |
| bump: major |
Its a logical breaking change, better safe than sorry for these things.
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
|
|
||
| let transaction = match transaction { | ||
| Ok(result) => result, | ||
| _ => unreachable!(), |
There was a problem hiding this comment.
With current storage::with_transaction implementation, this is indeed unreachable, but if the internals of that function change, this will crash the runtime.
I know, highly unlikely, but still I would prefer to:
debug_assert!()to have it crash in tests if the invariant is ever broken,- return some error
MessageExecutionStatusin prod without crashing.
There was a problem hiding this comment.
Yea it should be defensive and then just return an error or so. Having panics in the runtime is never good.
| #[test] | ||
| fn process_message_ok_false_keeps_storage_changes() { | ||
| // FAIL-CI TODO | ||
| } | ||
|
|
||
| #[test] | ||
| fn process_message_ok_true_keeps_storage_changes() { | ||
| // FAIL-CI TODO | ||
| } |
There was a problem hiding this comment.
Looks like these are the only missing pieces to get this in?
dharjeezy
force-pushed
the
dami/mq-transactional-processing
branch
from
30ed233 to
d8201b1
Compare
prdoc/pr_5198.prdoc
Outdated
| title: "MQ processor should be transactional" | ||
|
|
||
| doc: | ||
| - audience: Runtime User |
There was a problem hiding this comment.
| - audience: Runtime User | |
| - audience: [Runtime User, Runtime Dev] |
prdoc/pr_5198.prdoc
Outdated
| doc: | ||
| - audience: Runtime User | ||
| description: | | ||
| Enforce transactional processing on pallet Message Queue Processor |
There was a problem hiding this comment.
| Enforce transactional processing on pallet Message Queue Processor | |
| Enforce transactional processing on pallet Message Queue Processor. | |
| Storage changes that were done while processing a message will now be rolled back | |
| when the processing returns an error. `Ok(false)` will not revert, only `Err(_)`. |
|
|
||
| let transaction = match transaction { | ||
| Ok(result) => result, | ||
| _ => return MessageExecutionStatus::Unprocessable { permanent: false }, |
There was a problem hiding this comment.
| _ => return MessageExecutionStatus::Unprocessable { permanent: false }, | |
| _ => return MessageExecutionStatus::Unprocessable { permanent: true }, |
This should also use the defensive! macro to log an error since it is very much unexpected.
|
/tip small |
|
/tip small |
|
@bkchr A referendum for a small (20 DOT) tip was successfully submitted for @dharjeezy (12GyGD3QhT4i2JJpNzvMf96sxxBLWymz4RdGCxRH5Rj5agKW on polkadot). Referendum number: 1134. |
|
The referendum has appeared on Polkassembly. |
f6eeca9
closes #2441 Polkadot address: 12GyGD3QhT4i2JJpNzvMf96sxxBLWymz4RdGCxRH5Rj5agKW --------- Signed-off-by: Oliver Tale-Yazdi <[email protected]> Co-authored-by: Oliver Tale-Yazdi <[email protected]> Co-authored-by: Bastian Köcher <[email protected]>
closes #2441
Polkadot address: 12GyGD3QhT4i2JJpNzvMf96sxxBLWymz4RdGCxRH5Rj5agKW