rework: Rework the manifest resource interpreter #1931
Conversation
dhedey
force-pushed
the
feature/resource-movements-static-analysis-rework
branch
2 times, most recently
from
d499922 to
1a9c066
Compare
|
Docker tags |
dhedey
force-pushed
the
feature/resource-movements-static-analysis-rework
branch
from
1a9c066 to
269382e
Compare
dhedey
force-pushed
the
feature/resource-movements-static-analysis-rework
branch
from
e66bec1 to
be98983
Compare
Benchmark for 59e955eClick to view benchmark
|
radix-transactions/src/manifest/static_resource_movements/types.rs
Outdated
Show resolved
Hide resolved
radix-transactions/src/manifest/static_resource_movements/effect.rs
Outdated
Show resolved
Hide resolved
* It errors if it can't encode/decode, or can't match the native method/function * Named / reserved addresses work
| ValueKind::Array => Self::ManifestBuckets( | ||
| Vec::<ManifestBucket>::decode_body_with_value_kind(decoder, value_kind)?, | ||
| ), | ||
| ValueKind::Custom(_) => { |
There was a problem hiding this comment.
Probably should be ValueKind::Custom(ManifestCustomValueKind::Expression)
There was a problem hiding this comment.
Could be - I think it's fine either way, this is just a hint to work out how to branch - it'll get verified in the Expression decode method anyway :)
| @@ -364,6 +368,15 @@ impl CheckedAdd<Decimal> for Decimal { | |||
| } | |||
| } | |||
|
|
|||
| impl SaturatingAdd<Decimal> for Decimal { | |||
There was a problem hiding this comment.
I assume that this is no longer needed?
There was a problem hiding this comment.
True, but may as well keep it?
| @@ -366,6 +366,15 @@ macro_rules! op_impl { | |||
| } | |||
| } | |||
|
|
|||
| impl SaturatingAdd for $t | |||
There was a problem hiding this comment.
I assume that this is no longer needed?
There was a problem hiding this comment.
True, but may as well keep it?
| @@ -6,6 +6,14 @@ pub trait CheckedAdd<Rhs = Self> { | |||
| Self: Sized; | |||
| } | |||
|
|
|||
| pub trait SaturatingAdd<Rhs = Self> { | |||
There was a problem hiding this comment.
I assume that this is no longer needed?
There was a problem hiding this comment.
True, but may as well keep it?
| @@ -72,6 +72,10 @@ impl NodeId { | |||
| matches!(self.entity_type(), Some(t) if t.is_global_component()) | |||
| } | |||
|
|
|||
| pub const fn is_global_account(&self) -> bool { | |||
| matches!(self.entity_type(), Some(t) if t.is_global_account()) | |||
There was a problem hiding this comment.
Consistency aside, a nicer way to implement this is:
self.entity_type().is_some_and(|entity_type| entity_type.is_global_account())There was a problem hiding this comment.
Doesn't work because these methods are const, and is_some_and is not const :D
| method_name: &str, | ||
| args: &::radix_common::prelude::ManifestValue | ||
| ) -> Result<Option<Self>, StaticResourceMovementsError> { | ||
| Ok(match *package_address { |
There was a problem hiding this comment.
To reduce nesting in the generated code and improve the readability of the macro code I think we should match over a tuple of package address and blueprint name.
let maybe_self = match (*package_address, blueprint_name) {
$(
$(
($package_address, stringify!($blueprint_name)) => {
let invocation = [< $blueprint_name Method >]::from_invocation(method_name, args)?;
Some(Self::[< $package_name Package >](
[< $package_name Invocations >]::[< $blueprint_name Blueprint >](
[< $blueprint_name BlueprintInvocations >]::Method(invocation)
)
))
}
)*
)*
_ => Err(StaticResourceMovementsError::UnknownNativeBlueprint {
package: *package_address,
blueprint: blueprint_name.to_string(),
}),
};
Ok(maybe_self)There was a problem hiding this comment.
This doesn't work.
We need a different error message for "package address not matched" (the package address is not native - or not supported yet - so we return None for not-matched)" to "package address is native and method is not detected" (this is an error, because our method mapping for each package should be complete)
| pub fn from_method_invocation( | ||
| address: &NodeId, | ||
| module_id: ::radix_engine_interface::prelude::ModuleId, | ||
| pub fn from_main_module_method_invocation( |
There was a problem hiding this comment.
Something about the new constructors isn't quite sitting right with me... Maybe we should combine the CallMethod invocation constructors into a single constructor that takes in a DynamicGlobalAddress?
This new DynamicGlobalAddress would be defined as:
pub enum DynamicGlobalAddress {
Static(GlobalAddress),
Named {
package_address: PackageAddress,
blueprint_name: String
}
}There was a problem hiding this comment.
Possibly - although we shouldn't call it a DynamicGlobalAddress because that clashes with the existing enum.
And internally, we will still need to delegate to two different match statements.
But yes, I'd support adding back a module_id in this API and letting a high level API then delegate to either a blueprint or entity type match. (e.g. a module id of metadata would go to a blueprint match on the metadata module blueprint)
| #[derive(Debug, Clone, PartialEq, Eq, Default)] | ||
| pub struct TrackedResources { | ||
| /// Captures the bounds of explicitly tracked resources. | ||
| /// Some of these may be |
radix-transactions/src/manifest/static_resource_movements/types.rs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| pub fn new_with_possible_balance_of_unspecified_resources( | ||
| change_sources: impl IntoIterator<Item = ChangeSource>, |
There was a problem hiding this comment.
Perhaps it doesn't need to be an iterator and can just be a ChangeSource.
There was a problem hiding this comment.
In principle, you could have multiple sources here - e.g. in a test assertion, if we have a subintent start, then a random call, and then a deposit worktop, then the deposit would have two uncertainty sources.
0xOmarA
merged commit 658b9ff
into
feature/resource-movements-static-analysis
Summary
Reworks some implementation of #1909 to be more powerful.