HB SBP Milestone review #2

[hbulgarini]

Hello! the milestone review 2 have been done for this repository:

You can find the comments under the comments with the structure:

/// <HB SBP Milestone Review II
///
/// Comment.
///
///>

The highlights for this review are:

• Benchmarking: Some calls lacks the benchmarking function and the values in the weights value seems to be copied from other calls.
• The marketplace pallet is storing unbounded vecs, which is a potential risk since it can blow out the PoV when the block is pushed to the relay chain for verification.

[godenzim]

yes those are placeholders

[gitsimon]

@hbulgarini Do you refer to the transaction started at the extrinsic level?

Our design requires the extrinsic submit_message calling process_message -> process_action not to fail on errors (we hide errors after depositing an event at line 378) happening during executing the message, but selectively revert (partial) storage changes done during whatever executing involves.

I expanded the top-level frame_support::pallet macro both with and without the #[transactional] here and only with it I get the process_action's function body wrapped with:

fn process_action(message_bytes: &Vec<u8>) -> Result<(), ProcessMessageResult> {
            use frame_support::storage::{with_transaction, TransactionOutcome};
            with_transaction(|| {
                let r = (|| {
                    {
                        // ...
                        T::ActionExecutor::execute(action) // <- whatever side-effects produced by this should be reverted on later failure
                            .map_err(|_| ProcessMessageResult::ActionFailed(raw_action.clone()))?;

                        Ok(())
                    }
                })();
                if r.is_ok() { TransactionOutcome::Commit(r) } else { TransactionOutcome::Rollback(r) }
            })
        }

[hbulgarini]

indeed but my understanding is that the process_action fn is going to be reverted anyway since it's being called from the extrinsic submit_message making all the storage commits revertible.
But your comment is valid and now i doubt about my statement, maybe we might reproduce it in a test and see how it works?

These two GH links might be useful as well:

paritytech/substrate#11431
paritytech/substrate#10806