refactor: Moves all transaction models into radix-transactions#1871
Conversation
There was a problem hiding this comment.
These were shifted-and-lifted.
| } | ||
| } | ||
|
|
||
| pub struct SubstateBootStore<'a, S: SubstateDatabase> { |
There was a problem hiding this comment.
This was unused, so I deleted it as part of the fixing of compiler warnings from 1.80
| impl<P: Clone> Node<P> { | ||
| /// Creates the [`Internal`](Node::Internal) variant. | ||
| #[cfg(any(test, feature = "fuzzing"))] | ||
| #[cfg(test)] |
There was a problem hiding this comment.
The fuzzing feature mention was a remanent from when we imported the code originally and as of 1.80 the compiler catches that this isn't a supported feature in the crate.
| @@ -0,0 +1,544 @@ | |||
| use crate::internal_prelude::*; | |||
There was a problem hiding this comment.
These were shifted-and-lifted out of the very long instruction file
| @@ -0,0 +1,65 @@ | |||
| use super::*; | |||
There was a problem hiding this comment.
This was shifted-and-lifted from node.
| @@ -0,0 +1,134 @@ | |||
| use super::*; | |||
There was a problem hiding this comment.
This was shifted-and-lifted from node.
dhedey
force-pushed
the
refactor/commonize-transactions
branch
from
05cca54 to
472ece9
Compare
|
Docker tags |
Benchmark for f1ea882Click to view benchmark
|
jakrawcz-rdx
left a comment
jakrawcz-rdx
left a comment
There was a problem hiding this comment.
I only have minor remarks, please address whatever makes sense and 🟢 !
| blobs, | ||
| .. | ||
| }) => (instructions.0, blobs.blobs), | ||
| _ => return Err("Transaction type not currently supported".to_string()), |
There was a problem hiding this comment.
(minor)
| _ => return Err("Transaction type not currently supported".to_string()), | |
| other_type => return Err(format!("Transaction type {:?} not currently supported", other_type)), |
| /// "unchanged" Substate with its new tree leaf. | ||
| pub enum AssociatedSubstateValue<'v> { | ||
| Upserted(&'v DbSubstateValue), | ||
| Upserted(&'v [u8]), |
There was a problem hiding this comment.
I quite like the well-named types instead of arbitrary bytes. Even if it's just an alias, it documents things a bit.
(minor; you can say "but here it literally means arbitrary bytes" and I will be ok)
There was a problem hiding this comment.
Yeah broadly I agree with you! :)
However, here DbSubstateValue = Vec<u8> - I wanted to change it to the more natural &[u8] rather than using &Vec<u8>.
I don't really like the type alias pattern for these sorts of types because they don't protect anything; I prefer new types.
Arguably that's what AssociatedSubstateValue is already :) - however, imo really it should wrap a RawScryptoValue<'a> - that's essentially what this PR tries to do: #1860
|
|
||
| impl DatabaseUpdate { | ||
| pub fn as_change(&self) -> SubstateChange<'_> { | ||
| /// Uses the given [`DatabaseKeyMapper`] to express self using database-level key encoding. |
There was a problem hiding this comment.
(nitpick) you can now drop this rustdoc from impls
| } | ||
|
|
||
| /// A 1:1 counterpart of [`DatabaseUpdate`], but operating on references. | ||
| pub enum DatabaseUpdateRef<'v> { |
There was a problem hiding this comment.
nice; if we are touching this, should we go for:
pub enum DatabaseUpdateWithValueOrRef<V> {
...
}
pub type DatabaseUpdate = DatabaseUpdateWithValueOrRef<DbSubstateValue>;
pub type DatabaseUpdateRef<'v> = DatabaseUpdateWithValueOrRef<&'v [u8]>;
(please disregard if it does not deduplicate any "common" handling code and just makes things harder ;p)
There was a problem hiding this comment.
Once we have #1860 I think it's natural to formulate it as:
pub enum DatabaseUpdate<'v> {
Set(RawScryptoPayload<'v>),
Delete,
}
pub type OwnedDatabaseUpdate = DatabaseUpdate<'static>;(Happy to debate naming) So perhaps we can wait for that?
2 participants
Summary
ScryptoDescribeto the transaction models so that I can assert that theNotarizedTransactionV1schema is fixedManifestSchemaand use that; but this is good enough for now, to protect us from accidentally breaking transaction models! Nearly all types map 1:1 between these schemas. We can look to adjust in future.radix-transactions:Flashtransaction, this required moving theStateUpdatesmodels intoradix-commonfromradix-engine / track- which I was initially a bit hesitant about, but I think actually works quite nicely. (radix-commonis already aware of some of the nuances of nodes and things).radix-clisVersionedTransactionPayloadto be completeTesting
N/A - just a refactor, existing tests cover it.
Update Recommendations
radix_common::prelude::*.