[move-2] Decorated values and resource viewer for enum types #14144
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wrwg
added
CICD:run-e2e-tests
wrwg
requested review from
vineethk,
runtian-zhou,
vgao1996,
fEst1ck,
georgemitenkov,
brmataptos and
ziaptos
wrwg
requested review from
davidiw,
movekevin,
banool,
gregnazario and
0xmaayan
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14144 +/- ##
=========================================
- Coverage 59.1% 59.0% -0.2%
=========================================
Files 827 828 +1
Lines 201015 201584 +569
=========================================
+ Hits 118830 118943 +113
- Misses 82185 82641 +456 ☔ View full report in Codecov by Sentry. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
wrwg
commented
Jul 30, 2024
| abort_code: EARGS_MISMATCH, | ||
| }); | ||
| } | ||
| let tag = elems.pop().unwrap().value_as::<u32>()? as usize; |
There was a problem hiding this comment.
Good catch, fixed. This is no handled as a general helper in values_impl.
| /// A decorated representation with human-readable field names | ||
| WithFields(Vec<(Identifier, MoveValue)>), | ||
| /// An even more decorated representation with both types and human-readable field names | ||
| WithTypes { | ||
| type_: StructTag, | ||
| fields: Vec<(Identifier, MoveValue)>, | ||
| }, | ||
| /// A decorated representation of a variant, with the variant name, tag value, and field values. | ||
| WithVariant(Identifier, u16, Vec<(Identifier, MoveValue)>), |
| #[derive(Debug, Clone, Hash, Serialize, Deserialize, PartialEq, Eq)] | ||
| #[cfg_attr(any(test, feature = "fuzzing"), derive(arbitrary::Arbitrary))] | ||
| pub enum MoveStructLayout { | ||
| /// The representation used by the MoveVM for plain structs | ||
| Runtime(Vec<MoveTypeLayout>), | ||
| /// The representation used by the MoveVM for struct variants. | ||
| /// The representation used by the MoveVM for plaint struct variants. |
| @@ -40,6 +40,7 @@ mod limit; | |||
| pub struct AnnotatedMoveStruct { | |||
| pub abilities: AbilitySet, | |||
| pub ty_tag: StructTag, | |||
| pub variant_tag: Option<(u16, Identifier)>, | |||
| .collect(), | ||
| MoveStruct::WithFields(fields) | MoveStruct::WithTypes { fields, .. } => fields, | ||
| MoveStruct::Runtime(values) => { | ||
| let (_, field_names) = self.get_field_information(&ty, None)?; |
There was a problem hiding this comment.
I added a debug assert, but this is clearly satisfied invariant since if you pass in None to this function, the returned tag will always be None. Does not depend on external input.
| }, | ||
| _ => bail!("inconsistent layout information"), |
There was a problem hiding this comment.
Agree, but not boiling the ocean. All those things should be asserts but we can't have crashes. We really should have an internal_bail! which creates a bug report with stack trace or something.
| if let Some((_, name)) = &self.variant_tag { | ||
| let mut s = serializer.serialize_map(Some(self.value.len() + 1))?; | ||
| s.serialize_entry("$variant", name)?; | ||
| for (f, v) in &self.value { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
georgemitenkov
approved these changes
Jul 30, 2024
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This adapts more functionality around enums/struct variants for move value representation: - `MoveStructLayout::WithVariants` is a new decorated representation for variant types, and `MoveStruct::WithVariant` is the matching decorated value representation. Code has been adapted to deal with those decorated versions - Unfortunately, we do not only have serializers/deserializers in `value.rs` (which where already implemented for variants) but also `value_impl.rs`. It is unclear what of this code is used where, so implemented now the serialization logic a 2nd time in `value_impl.rs` with support of custom serializers. - Adapted yet another redundant type representation to support variants, `FatType` in resource viewer. Extended `AnnotatedMoveValue` to carry an optional field for variant names. This led to some upstream changes in api. - Changed serialization logic and runtime representation. Serialization did not really work as we require `deserialize_seq` for variants and henceforth also for serialization. Since values are serialized type free, they need to be distinguished whether variant or not. This in turn makes it more logical to call out the variant tag explicitly in MoveValue. - Implement stackless bytecode generator for enums to make Aptos CLI working. (Required by extended checks.) This is tested by existing tests. Also a new roundtrip test is added. Its unclear how resource viewer and related functionality are tested at all besides e2e tests, but did some manual tests via Aptos CLI.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This adapts more functionality around enums/struct variants for move value representation:
MoveStructLayout::WithVariantsis a new decorated representation for variant types, andMoveStruct::WithVariantis the matching decorated value representation. Code has been adapted to deal with those decorated versionsvalue.rs(which where already implemented for variants) but alsovalue_impl.rs. It is unclear what of this code is used where, so implemented now the serialization logic a 2nd time invalue_impl.rswith support of custom serializers.FatTypein resource viewer. ExtendedAnnotatedMoveValueto carry an optional field for variant names. This led to some upstream changes in api.deserialize_seqfor variants and henceforth also for serialization. Since values are serialized type free, they need to be distinguished whether variant or not. This in turn makes it more logical to call out the variant tag explicitly in MoveValue.Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
This is tested by existing tests. Also a new roundtrip test is added. Its unclear how resource viewer and related functionality are tested at all besides e2e tests, but did some manual tests via Aptos CLI.