Skip to content

Commit f686afa

Browse files
tnowackitnowacki
authored
[move-values] Clean up vector pack (#22071)
## Description - Remove the need for a `Type` in `Vector::pack` by adding a new `VectorSpecialization` enum - Removed `Value::vector_for_testing_only` ## Test plan - Updated tests --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] gRPC: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK:
1 parent 7ddb5f3 commit f686afa

File tree

8 files changed

+115
-55
lines changed

8 files changed

+115
-55
lines changed

‎external-crates/move/crates/move-stdlib-natives/src/vector.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,10 @@ pub fn native_empty(
4343

4444
native_charge_gas_early_exit!(context, gas_params.base);
4545

46-
NativeResult::map_partial_vm_result_one(context.gas_used(), Vector::empty(&ty_args[0]))
46+
NativeResult::map_partial_vm_result_one(
47+
context.gas_used(),
48+
(&ty_args[0]).try_into().and_then(Vector::empty),
49+
)
4750
}
4851

4952
pub fn make_native_empty(gas_params: EmptyGasParameters) -> NativeFunction {

‎external-crates/move/crates/move-vm-runtime/src/interpreter.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ use move_vm_types::{
2727
loaded_data::runtime_types::Type,
2828
values::{
2929
self, IntegerValue, Locals, Reference, Struct, StructRef, VMValueCast, Value, Variant,
30-
VariantRef, Vector, VectorRef,
30+
VariantRef, Vector, VectorRef, VectorSpecialization,
3131
},
3232
views::TypeView,
3333
};
@@ -1271,7 +1271,8 @@ impl Frame {
12711271
interpreter.operand_stack.last_n(*num as usize)?,
12721272
)?;
12731273
let elements = interpreter.operand_stack.popn(*num as u16)?;
1274-
let value = Vector::pack(&ty, elements)?;
1274+
let specialization: VectorSpecialization = (&ty).try_into()?;
1275+
let value = Vector::pack(specialization, elements)?;
12751276
interpreter.operand_stack.push(value)?;
12761277
}
12771278
Bytecode::VecLen(si) => {

‎external-crates/move/crates/move-vm-types/src/values/value_tests.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -194,8 +194,8 @@ fn legacy_val_abstract_memory_size_consistency() -> PartialVMResult<()> {
194194
Value::vector_u256([1, 2, 3, 4].iter().map(|q| U256::from(*q as u64))),
195195
Value::struct_(Struct::pack([])),
196196
Value::struct_(Struct::pack([Value::u8(0), Value::bool(false)])),
197-
Value::vector_for_testing_only([]),
198-
Value::vector_for_testing_only([Value::u8(0), Value::u8(1)]),
197+
Vector::pack(VectorSpecialization::Container, [])?,
198+
Vector::pack(VectorSpecialization::U8, [Value::u8(0), Value::u8(1)])?,
199199
];
200200

201201
let mut locals = Locals::new(vals.len());

‎external-crates/move/crates/move-vm-types/src/values/values_impl.rs

Lines changed: 62 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1372,7 +1372,6 @@ impl Value {
13721372
))))
13731373
}
13741374

1375-
// TODO: consider whether we want to replace these with fn vector(v: Vec<Value>).
13761375
pub fn vector_u8(it: impl IntoIterator<Item = u8>) -> Self {
13771376
Self(ValueImpl::Container(Container::VecU8(Rc::new(
13781377
RefCell::new(it.into_iter().collect()),
@@ -1420,13 +1419,6 @@ impl Value {
14201419
RefCell::new(it.into_iter().collect()),
14211420
))))
14221421
}
1423-
1424-
// REVIEW: This API can break
1425-
pub fn vector_for_testing_only(it: impl IntoIterator<Item = Value>) -> Self {
1426-
Self(ValueImpl::Container(Container::Vec(Rc::new(RefCell::new(
1427-
it.into_iter().map(|v| v.0).collect(),
1428-
)))))
1429-
}
14301422
}
14311423

14321424
/***************************************************************************************
@@ -1474,6 +1466,12 @@ macro_rules! impl_vm_value_cast_boxed {
14741466
};
14751467
}
14761468

1469+
impl VMValueCast<Value> for Value {
1470+
fn cast(self) -> PartialVMResult<Value> {
1471+
Ok(self)
1472+
}
1473+
}
1474+
14771475
impl_vm_value_cast!(u8, U8);
14781476
impl_vm_value_cast!(u16, U16);
14791477
impl_vm_value_cast!(u32, U32);
@@ -2452,77 +2450,109 @@ impl VectorRef {
24522450
}
24532451
}
24542452

2453+
pub enum VectorSpecialization {
2454+
U8,
2455+
U16,
2456+
U32,
2457+
U64,
2458+
U128,
2459+
U256,
2460+
Bool,
2461+
Address,
2462+
Container,
2463+
}
2464+
2465+
impl TryFrom<&Type> for VectorSpecialization {
2466+
type Error = PartialVMError;
2467+
2468+
fn try_from(ty: &Type) -> Result<Self, Self::Error> {
2469+
Ok(match ty {
2470+
Type::U8 => VectorSpecialization::U8,
2471+
Type::U16 => VectorSpecialization::U16,
2472+
Type::U32 => VectorSpecialization::U32,
2473+
Type::U64 => VectorSpecialization::U64,
2474+
Type::U128 => VectorSpecialization::U128,
2475+
Type::U256 => VectorSpecialization::U256,
2476+
Type::Bool => VectorSpecialization::Bool,
2477+
Type::Address => VectorSpecialization::Address,
2478+
Type::Vector(_) | Type::Signer | Type::Datatype(_) | Type::DatatypeInstantiation(_) => {
2479+
VectorSpecialization::Container
2480+
}
2481+
Type::Reference(_) | Type::MutableReference(_) | Type::TyParam(_) => {
2482+
return Err(
2483+
PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR)
2484+
.with_message(format!("invalid type param for vector: {:?}", ty)),
2485+
);
2486+
}
2487+
})
2488+
}
2489+
}
2490+
24552491
impl Vector {
2456-
pub fn pack(type_param: &Type, elements: Vec<Value>) -> PartialVMResult<Value> {
2457-
let container = match type_param {
2458-
Type::U8 => Value::vector_u8(
2492+
pub fn pack(
2493+
specialization: VectorSpecialization,
2494+
elements: impl IntoIterator<Item = Value>,
2495+
) -> PartialVMResult<Value> {
2496+
let container = match specialization {
2497+
VectorSpecialization::U8 => Value::vector_u8(
24592498
elements
24602499
.into_iter()
24612500
.map(|v| v.value_as())
24622501
.collect::<PartialVMResult<Vec<_>>>()?,
24632502
),
2464-
Type::U16 => Value::vector_u16(
2503+
VectorSpecialization::U16 => Value::vector_u16(
24652504
elements
24662505
.into_iter()
24672506
.map(|v| v.value_as())
24682507
.collect::<PartialVMResult<Vec<_>>>()?,
24692508
),
2470-
Type::U32 => Value::vector_u32(
2509+
VectorSpecialization::U32 => Value::vector_u32(
24712510
elements
24722511
.into_iter()
24732512
.map(|v| v.value_as())
24742513
.collect::<PartialVMResult<Vec<_>>>()?,
24752514
),
2476-
Type::U64 => Value::vector_u64(
2515+
VectorSpecialization::U64 => Value::vector_u64(
24772516
elements
24782517
.into_iter()
24792518
.map(|v| v.value_as())
24802519
.collect::<PartialVMResult<Vec<_>>>()?,
24812520
),
2482-
Type::U128 => Value::vector_u128(
2521+
VectorSpecialization::U128 => Value::vector_u128(
24832522
elements
24842523
.into_iter()
24852524
.map(|v| v.value_as())
24862525
.collect::<PartialVMResult<Vec<_>>>()?,
24872526
),
2488-
Type::U256 => Value::vector_u256(
2527+
VectorSpecialization::U256 => Value::vector_u256(
24892528
elements
24902529
.into_iter()
24912530
.map(|v| v.value_as())
24922531
.collect::<PartialVMResult<Vec<_>>>()?,
24932532
),
2494-
Type::Bool => Value::vector_bool(
2533+
VectorSpecialization::Bool => Value::vector_bool(
24952534
elements
24962535
.into_iter()
24972536
.map(|v| v.value_as())
24982537
.collect::<PartialVMResult<Vec<_>>>()?,
24992538
),
2500-
Type::Address => Value::vector_address(
2539+
VectorSpecialization::Address => Value::vector_address(
25012540
elements
25022541
.into_iter()
25032542
.map(|v| v.value_as())
25042543
.collect::<PartialVMResult<Vec<_>>>()?,
25052544
),
25062545

2507-
Type::Signer | Type::Vector(_) | Type::Datatype(_) | Type::DatatypeInstantiation(_) => {
2508-
Value(ValueImpl::Container(Container::Vec(Rc::new(RefCell::new(
2509-
elements.into_iter().map(|v| v.0).collect(),
2510-
)))))
2511-
}
2512-
2513-
Type::Reference(_) | Type::MutableReference(_) | Type::TyParam(_) => {
2514-
return Err(
2515-
PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR)
2516-
.with_message(format!("invalid type param for vector: {:?}", type_param)),
2517-
);
2518-
}
2546+
VectorSpecialization::Container => Value(ValueImpl::Container(Container::Vec(
2547+
Rc::new(RefCell::new(elements.into_iter().map(|v| v.0).collect())),
2548+
))),
25192549
};
25202550

25212551
Ok(container)
25222552
}
25232553

2524-
pub fn empty(type_param: &Type) -> PartialVMResult<Value> {
2525-
Self::pack(type_param, vec![])
2554+
pub fn empty(specialization: VectorSpecialization) -> PartialVMResult<Value> {
2555+
Self::pack(specialization, vec![])
25262556
}
25272557

25282558
pub fn unpack(self, type_param: &Type, expected_num: u64) -> PartialVMResult<Vec<Value>> {

‎sui-execution/latest/sui-move-natives/src/config.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,6 @@ fn unpack_option(option: Value, type_param: &Type) -> PartialVMResult<Option<Val
184184

185185
fn option_none(type_param: &Type) -> PartialVMResult<Value> {
186186
Ok(Value::struct_(Struct::pack(vec![Vector::empty(
187-
type_param,
187+
type_param.try_into()?,
188188
)?])))
189189
}

‎sui-execution/latest/sui-move-natives/src/crypto/nitro_attestation.rs

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use move_vm_types::{
88
loaded_data::runtime_types::Type,
99
natives::function::NativeResult,
1010
pop_arg,
11-
values::{Struct, Value, Vector, VectorRef},
11+
values::{Struct, Value, Vector, VectorRef, VectorSpecialization},
1212
};
1313
use std::collections::VecDeque;
1414
use sui_types::nitro_attestation::{parse_nitro_attestation, verify_nitro_attestation};
@@ -119,16 +119,15 @@ pub fn load_nitro_attestation_internal(
119119

120120
// Build an Option<vector<u8>> value
121121
fn to_option_vector_u8(value: Option<Vec<u8>>) -> PartialVMResult<Value> {
122-
let vector_u8_type = Type::Vector(Box::new(Type::U8));
123122
match value {
124123
// Some(<vector<u8>>) = { vector[ <vector<u8>> ] }
125124
Some(vec) => Ok(Value::struct_(Struct::pack(vec![Vector::pack(
126-
&vector_u8_type,
125+
VectorSpecialization::Container,
127126
vec![Value::vector_u8(vec)],
128127
)?]))),
129128
// None = { vector[ ] }
130129
None => Ok(Value::struct_(Struct::pack(vec![Vector::empty(
131-
&vector_u8_type,
130+
VectorSpecialization::Container,
132131
)?]))),
133132
}
134133
}
@@ -148,5 +147,5 @@ fn to_indexed_struct(pcrs: Vec<Vec<u8>>) -> PartialVMResult<Value> {
148147
Value::vector_u8(pcr.to_vec()),
149148
])));
150149
}
151-
Ok(Value::vector_for_testing_only(indexed_struct))
150+
Vector::pack(VectorSpecialization::Container, indexed_struct)
152151
}

‎sui-execution/latest/sui-move-natives/src/event.rs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,9 @@ use move_binary_format::errors::{PartialVMError, PartialVMResult};
66
use move_core_types::{gas_algebra::InternalGas, language_storage::TypeTag, vm_status::StatusCode};
77
use move_vm_runtime::{native_charge_gas_early_exit, native_functions::NativeContext};
88
use move_vm_types::{
9-
loaded_data::runtime_types::Type, natives::function::NativeResult, values::Value,
9+
loaded_data::runtime_types::Type,
10+
natives::function::NativeResult,
11+
values::{Value, VectorSpecialization},
1012
};
1113
use smallvec::smallvec;
1214
use std::collections::VecDeque;
@@ -142,6 +144,7 @@ pub fn get_events_by_type(
142144
) -> PartialVMResult<NativeResult> {
143145
assert_eq!(ty_args.len(), 1);
144146
let specified_ty = ty_args.pop().unwrap();
147+
let specialization: VectorSpecialization = (&specified_ty).try_into()?;
145148
assert!(args.is_empty());
146149
let object_runtime_ref: &ObjectRuntime = context.extensions().get()?;
147150
let matched_events = object_runtime_ref
@@ -158,6 +161,9 @@ pub fn get_events_by_type(
158161
.collect::<Vec<_>>();
159162
Ok(NativeResult::ok(
160163
legacy_test_cost(),
161-
smallvec![Value::vector_for_testing_only(matched_events)],
164+
smallvec![move_vm_types::values::Vector::pack(
165+
specialization,
166+
matched_events
167+
)?],
162168
))
163169
}

‎sui-execution/latest/sui-move-natives/src/test_scenario.rs

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ use move_vm_types::{
2020
loaded_data::runtime_types::Type,
2121
natives::function::NativeResult,
2222
pop_arg,
23-
values::{self, StructRef, Value},
23+
values::{self, StructRef, Value, Vector, VectorSpecialization},
2424
};
2525
use smallvec::smallvec;
2626
use std::{
@@ -420,7 +420,7 @@ pub fn ids_for_address(
420420
.and_then(|inv| inv.get(&specified_ty))
421421
.map(|s| s.iter().map(|id| pack_id(*id)).collect::<Vec<Value>>())
422422
.unwrap_or_default();
423-
let ids_vector = Value::vector_for_testing_only(ids);
423+
let ids_vector = Vector::pack(VectorSpecialization::Container, ids).unwrap();
424424
Ok(NativeResult::ok(legacy_test_cost(), smallvec![ids_vector]))
425425
}
426426

@@ -436,7 +436,7 @@ pub fn most_recent_id_for_address(
436436
let object_runtime: &mut ObjectRuntime = context.extensions_mut().get_mut()?;
437437
let inventories = &mut object_runtime.test_inventories;
438438
let most_recent_id = match inventories.address_inventories.get(&account) {
439-
None => pack_option(None),
439+
None => pack_option(vector_specialization(&specified_ty), None),
440440
Some(inv) => most_recent_at_ty(&inventories.taken, inv, specified_ty),
441441
};
442442
Ok(NativeResult::ok(
@@ -770,12 +770,25 @@ fn take_from_inventory(
770770
Ok(obj.copy_value().unwrap())
771771
}
772772

773+
fn vector_specialization(ty: &Type) -> VectorSpecialization {
774+
match ty.try_into() {
775+
Ok(s) => s,
776+
Err(_) => {
777+
debug_assert!(false, "Invalid vector specialization");
778+
VectorSpecialization::Container
779+
}
780+
}
781+
}
782+
773783
fn most_recent_at_ty(
774784
taken: &BTreeMap<ObjectID, Owner>,
775785
inv: &BTreeMap<Type, Set<ObjectID>>,
776786
ty: Type,
777787
) -> Value {
778-
pack_option(most_recent_at_ty_opt(taken, inv, ty))
788+
pack_option(
789+
vector_specialization(&ty),
790+
most_recent_at_ty_opt(taken, inv, ty),
791+
)
779792
}
780793

781794
fn most_recent_at_ty_opt(
@@ -813,15 +826,21 @@ fn pack_id(a: impl Into<AccountAddress>) -> Value {
813826
}
814827

815828
fn pack_ids(items: impl IntoIterator<Item = impl Into<AccountAddress>>) -> Value {
816-
Value::vector_for_testing_only(items.into_iter().map(pack_id))
829+
Vector::pack(
830+
VectorSpecialization::Container,
831+
items.into_iter().map(pack_id),
832+
)
833+
.unwrap()
817834
}
818835

819836
fn pack_vec_map(items: impl IntoIterator<Item = (Value, Value)>) -> Value {
820-
Value::struct_(values::Struct::pack(vec![Value::vector_for_testing_only(
837+
Value::struct_(values::Struct::pack(vec![Vector::pack(
838+
VectorSpecialization::Container,
821839
items
822840
.into_iter()
823841
.map(|(k, v)| Value::struct_(values::Struct::pack(vec![k, v]))),
824-
)]))
842+
)
843+
.unwrap()]))
825844
}
826845

827846
fn transaction_effects(
@@ -872,14 +891,16 @@ fn transaction_effects(
872891
]))
873892
}
874893

875-
fn pack_option(opt: Option<Value>) -> Value {
894+
fn pack_option(specialization: VectorSpecialization, opt: Option<Value>) -> Value {
876895
let item = match opt {
877896
Some(v) => vec![v],
878897
None => vec![],
879898
};
880-
Value::struct_(values::Struct::pack(vec![Value::vector_for_testing_only(
899+
Value::struct_(values::Struct::pack(vec![Vector::pack(
900+
specialization,
881901
item,
882-
)]))
902+
)
903+
.unwrap()]))
883904
}
884905

885906
fn find_all_wrapped_objects<'a, 'i>(

0 commit comments

Comments
 (0)