Minor improvements to the compression crate #2254
Conversation
| fn read_registry(&self, key: &RegistryKey) -> anyhow::Result<T>; | ||
|
|
||
| /// Reads a value from the registry at its current height. | ||
| fn write_registry(&mut self, key: RegistryKey, value: &T) -> anyhow::Result<()>; | ||
| fn write_registry(&mut self, key: &RegistryKey, value: &T) -> anyhow::Result<()>; |
There was a problem hiding this comment.
I don't understand why RegistryKey should be passed by reference. It's small enough to fit into a register so performance is likely not the reason here.
There was a problem hiding this comment.
Just to avoid *key and &key in the code=) You can see that now everything just key
| impl<D, T> TemporalRegistry<T> for &mut D | ||
| where | ||
| D: TemporalRegistry<T>, |
There was a problem hiding this comment.
It allows to use &mut Database as an input to CompressCtx instead of Database
| /// currently backed by a `DaCompressionTemporalRegistryEvictor*` | ||
| /// columns in the offchain database. |
There was a problem hiding this comment.
Not sure why this is removed? It's quite helpful in finding the backing storage. I understand that it breaks the illusion of not depending on the other crate, but in reality they are tightly coupled.
There was a problem hiding this comment.
The trait should be defined without implementation details. The name of the table that is used under the hood inside of the fuel-core an be different in the future, or implementation can be different=) We don't need this information here.
We need to describe how this trait could be used, but not how it is used.
Co-authored-by: Hannes Karppila <[email protected]>
| tables!( | ||
| address: Address, | ||
| asset_id: AssetId, | ||
| contract_id: ContractId, | ||
| script_code: ScriptCode, | ||
| predicate_code: PredicateCode | ||
| ); |
There was a problem hiding this comment.
I removed this so the macros are easier to work with. Do you think it makes sense to introduce the snake_case names just to get rid of the #[allow(non_snake_case)]? Or is there another reason?
There was a problem hiding this comment.
In the tests you used type name as a field name to access it. Plus explicit address: Address means that somewhere there are fields inside of some types. For me it is easier to read and understand=)
There was a problem hiding this comment.
My issue with this is that the field names do not and should not matter. They are only used because Rust generics are not powerful enough. Even though they are implemented as field names, they alway refer to the types.
Minor improvements to the #1609 PR=) Requires decision from @Dentosal