RET manfiest classification update #373
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #373 +/- ##
==========================================
+ Coverage 92.68% 92.77% +0.08%
==========================================
Files 1190 1190
Lines 26861 26907 +46
Branches 85 85
==========================================
+ Hits 24896 24962 +66
+ Misses 1954 1934 -20
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| impl TryFrom<(ScryptoManifestResourceAddress, NetworkID)> for ResourceAddress { | ||
| type Error = CommonError; | ||
| fn try_from( | ||
| value: (ScryptoManifestResourceAddress, NetworkID), |
There was a problem hiding this comment.
an alternative spelling is: (address, network): (ScryptoManifestResourceAddress, NetworkID) which I've started to use
| fn from(value: (RetNewEntities, NetworkID)) -> Self { | ||
| let (ret, network_id) = value; | ||
| impl | ||
| From<( |
There was a problem hiding this comment.
oh wow, hehe, I think maybe best do this as a named ctor on NewEntities, this is quite hard to read
| return Ok(ManifestEncounteredComponentAddress::$variant(address)); | ||
| } | ||
| )* | ||
|
|
||
| Err(CommonError::FailedToCreateAddressFromGlobalAddressAndNetworkID { |
There was a problem hiding this comment.
Maybe rename the error to FailedToCreateAddressFromManifestAddressAndNetworkID { address: address.to_string() } or to_hex
| @@ -370,6 +370,7 @@ iso8601-timestamp = { version = "0.2.16", default-features = false, features = [ | |||
| ] } | |||
|
|
|||
| itertools = { version = "0.12.0", default-features = true } | |||
| either = { version = "1.13.0", default-features = false } | |||
There was a problem hiding this comment.
Im really not a fan of Either as ::Left gives zero glue as to what it is. Can we create a custom enum instead?
There was a problem hiding this comment.
I am using Either as it is part of iter partition_map API, might need to revisit then that implementation. Either is good for temporary types, similar to tuples.
| pub(crate) fn filter_try_to_hashmap_network_aware_key<K, V, L, U>( | ||
| values: impl IntoIterator<Item = (K, V)>, | ||
| network_id: NetworkID, | ||
| ) -> HashMap<L, U> |
There was a problem hiding this comment.
should we return IndexMap instead for stable sorting helpful when debugging?
There was a problem hiding this comment.
yeah right, may be a good time to use IndexMap instead of HashMap where possible
| @@ -79,7 +96,7 @@ pub struct ExecutionSummary { | |||
|
|
|||
| /// The set of instructions encountered in the manifest that are reserved | |||
| /// and can only be included in the manifest by the wallet itself. | |||
| pub reserved_instructions: Vec<ReservedInstruction>, | |||
| pub reserved_instructions: IndexSet<ReservedInstruction>, | |||
There was a problem hiding this comment.
👍 ah nice with IndexSet! post break out UniFFI allows us to do this, excellent!
| .or_default() | ||
| .insert(resource_address, update); | ||
| }; | ||
| acc |
There was a problem hiding this comment.
if any is err. we effectively filter it out without error? should be log at least?
There was a problem hiding this comment.
This can fail only if there are named manifest addresses, which now are simply ignored.
| let resource = | ||
| ResourceOrNonFungible::try_from((manifest_resource, n)) | ||
| .ok()?; | ||
| Some(((account_address, resource), op)) |
Update to use new RET manifest summary models.
For now just mapped new models to current Sargon structure with the intent to reduce the number of changes as much as possible.
A followup PR will add additional fields/models to Sargon's ManifestSummary/ExecutionSummary to make use of new RET summary surfaced information.