Suggestions to simplify the V1 storage requirements and initialization #2483
Conversation
xgreenx
requested review from
Dentosal,
MitchTurner and
rafal-ch
as code owners
…ure/simplify-v1-initialization # Conflicts: # crates/services/gas_price_service/src/v1/tests.rs # crates/services/gas_price_service/src/v1/uninitialized_task.rs
…y-v1-initialization
| pub trait TransactionableStorage | ||
| where | ||
| Self: 'static, | ||
| Self: Send + Sync, | ||
| Self: GetMetadataStorage + GetDaSequenceNumber, | ||
| { | ||
| type Transaction<'a>: AsUnrecordedBlocks | ||
| + SetMetadataStorage | ||
| + GetMetadataStorage | ||
| + SetDaSequenceNumber | ||
| + GetDaSequenceNumber |
There was a problem hiding this comment.
Generally I think pushing many constraints into a single trait is an anti-pattern. I prefer to keep all the constraints as close to place that needs those constraints met. Namely, the GasPriceService needs these things to be transactionable together in order to construct the storage tx.
You can make the argument that the domain requirement is that our storage is transactionable for these things. Which I would agree with, but then we need to include the UnrecordedBlocks as a requirement as well.
There was a problem hiding this comment.
I feel like I'd prefer these constraints on the RunableTask impl for GasPriceService than on some obfuscated trait, but if we can name this something informative, not just Transactionable then I might be okay with it. e.g. GasPriceServiceAtomicStorage or something like that :)
There was a problem hiding this comment.
Naming is fully up to you, I just tried to show you an exampel fo how it can looks like=)
| pub trait AsUnrecordedBlocks { | ||
| type Wrapper<'a>: UnrecordedBlocks | ||
| where | ||
| Self: 'a; | ||
|
|
||
| fn as_unrecorded_blocks(&mut self) -> Self::Wrapper<'_>; | ||
| } | ||
|
|
||
| impl<S> AsUnrecordedBlocks for S | ||
| where | ||
| S: StorageMutate<UnrecordedBlocksTable, Error = StorageError>, | ||
| { | ||
| type Wrapper<'a> = FuelStorageUnrecordedBlocks<&'a mut Self> | ||
| where | ||
| Self: 'a; | ||
|
|
||
| fn as_unrecorded_blocks(&mut self) -> Self::Wrapper<'_> { | ||
| FuelStorageUnrecordedBlocks::new(self) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
What does this trait give us that we didn't get with the newtype wrapper? I feel like the newtype pattern is the idiomatic way to deal with the orphan rule problem in Rust, but maybe this solution freed up some constraints that I can't see.
There was a problem hiding this comment.
Without this helpers trait, you need to mock StorageMutate<UnrecordedBlocksTable, Error = StorageError> in tests, instead of just mocking AsUnrecordedBlocks. You can remove it and see the problem=)
MitchTurner
merged commit 2eba245
into
chore/add-unrecorded-blocks-trait
Suggestions for the #2468 PR.