Messaging general #494
Messaging general #494
Conversation
| type AccountIdOf<T> = <T as frame_system::Config>::AccountId; | ||
| type BlockNumberOf<T> = BlockNumberFor<T>; | ||
| type BalanceOf<T> = <<T as Config>::Deposit as Inspect<AccountIdOf<T>>>::Balance; | ||
| pub type MessageId = u64; | ||
| pub type MessageId = [u8; 32]; |
There was a problem hiding this comment.
Any reason for this change? Seems a little large for the message id.
There was a problem hiding this comment.
So that one can pass a 32 byte hash as an id (Blake2/3, sha256)
|
|
||
| // Calculate deposit and place on hold. | ||
| let deposit = Self::calculate_deposit( | ||
| message.calculate_deposit() + |
There was a problem hiding this comment.
This message.calculate_deposit() is missing in the newest calculation. Is it no longer needed?
There was a problem hiding this comment.
This has been replaced with calculate_message_deposit
|
|
||
| #[benchmark] | ||
| // x: The number of removals required. | ||
| fn remove(x: Linear<1, 255>) { |
There was a problem hiding this comment.
is it possible to use T::MaxRemovals here? (instead of 255)
There was a problem hiding this comment.
No but im tempted to increase the limit, what do you think?
| } | ||
|
|
||
| #[derive(Clone, Debug, Encode, Eq, Decode, MaxEncodedLen, PartialEq, TypeInfo)] | ||
| pub enum MessageStatus { |
There was a problem hiding this comment.
Nice! This feels better and keeps the Message struct a little cleaner. Also makes sense as it's easier to track what the original request was.
This goes over most of the General section of the messaging pallet's tasks.
Losing ones deposit
Previously, in the extrinsics that take a message deposit, there was a possibility that the extrinsic would error before the message is stored and therefore when one tried to removed the message, it did not exist and therefore, the deposit could not be claimed. This has been fixed by moving much of the validation code before taking the deposit and then handling the fallible code after taking deposit more appropriately. (NOTE: this is now irrelevant since transactional is default)
Remove extrinsic
Notably the
removeextrinsic has had a makeover so that it can be more comfortably unit tested.I have moved much of the logic into the impl of
Message, encapsulating the logic like this allowed me to cut down the complexity of the tests and the code.Many tests have been written and cover all bases as far as im concerned.
The extrinsic has also been made transactional this is because we deposit an event at the end of the extrinsic that emits the messages removed, The previous implementation would remove messages without emitting an event which will be a pain for app devs . We could build it so it ignores erroneous messages and emits only the ones that pass but i thought this was a quick fix and i have lots to do thats of higher priority.
MessageStatus
The
MessageStatushas been introduced and ive moved away from combination of message status and message type.Previously a message would be of type
IsmpTimedOutnow it is anIsmpMessagewith the status ofTimedOut.This was done to differentiate between what a message is and what status it is in. I think this will allow more flexibility for messages in the future. The downside is that one must handle all message statuses for a message and the previous version uses Rust's type system to enforce a messages status. This is the change im least confident about.
Storage Deposits
Previously the deposit implementations were scattered around the codebase and there was a trait called
CalculateDeposits. Following this code was more difficult than it had to be. I have madedeposits.rsto contain all the deposit calculation code, and used a blanket implementation instead of the specific impls of theCalculateDeposits. This has improved readability nicely.The
IsmpByteFeeis intended for Offchain storage costs hence it has been replaced withOffchainByteFee.ByteFeehas subsequently been changed toOnChainByteFee.Other
CallbackThas been renamed to CallbackExecutor for better semantics.MessageIdhas been changed to a [u8; 32] from an unsigned integer.Todo:
[sc-3066]