From e854e015cc97f453aaa977bd75ef31bd065e5625 Mon Sep 17 00:00:00 2001 From: Igor Date: Thu, 12 Oct 2023 15:10:07 -0700 Subject: [PATCH] Fixing tests and more comments --- .../e2e-move-tests/src/tests/aggregator_v2.rs | 11 +++-- .../aptos-framework/doc/aggregator_v2.md | 34 +++++++++++++- .../sources/aggregator_v2/aggregator_v2.move | 46 ++++++++++++++++--- .../aggregator_natives/aggregator_v2.rs | 2 +- 4 files changed, 81 insertions(+), 12 deletions(-) diff --git a/aptos-move/e2e-move-tests/src/tests/aggregator_v2.rs b/aptos-move/e2e-move-tests/src/tests/aggregator_v2.rs index 63055a6e28a72f..76b105067bcf91 100644 --- a/aptos-move/e2e-move-tests/src/tests/aggregator_v2.rs +++ b/aptos-move/e2e-move-tests/src/tests/aggregator_v2.rs @@ -6,10 +6,13 @@ use crate::{ initialize, verify_copy_snapshot, verify_copy_string_snapshot, verify_string_concat, verify_string_snapshot_concat, }, - assert_success, + assert_abort, assert_success, tests::common, MoveHarness, }; +use aptos_framework::natives::aggregator_natives::aggregator_v2::{ + EAGGREGATOR_FUNCTION_NOT_YET_SUPPORTED, EUNSUPPORTED_AGGREGATOR_SNAPSHOT_TYPE, +}; use aptos_language_e2e_tests::account::Account; fn setup() -> (MoveHarness, Account) { @@ -20,14 +23,14 @@ fn setup() -> (MoveHarness, Account) { fn test_copy_snapshot() { let (mut h, acc) = setup(); let txn = verify_copy_snapshot(&mut h, &acc); - assert_success!(h.run(txn)); + assert_abort!(h.run(txn), EAGGREGATOR_FUNCTION_NOT_YET_SUPPORTED); } #[test] fn test_copy_string_snapshot() { let (mut h, acc) = setup(); let txn = verify_copy_string_snapshot(&mut h, &acc); - assert_success!(h.run(txn)); + assert_abort!(h.run(txn), EAGGREGATOR_FUNCTION_NOT_YET_SUPPORTED); } #[test] @@ -41,5 +44,5 @@ fn test_string_concat() { fn test_string_snapshot_concat() { let (mut h, acc) = setup(); let txn = verify_string_snapshot_concat(&mut h, &acc); - assert_success!(h.run(txn)); + assert_abort!(h.run(txn), EUNSUPPORTED_AGGREGATOR_SNAPSHOT_TYPE); } diff --git a/aptos-move/framework/aptos-framework/doc/aggregator_v2.md b/aptos-move/framework/aptos-framework/doc/aggregator_v2.md index 8104abdcd3867b..9dfedf155df1f9 100644 --- a/aptos-move/framework/aptos-framework/doc/aggregator_v2.md +++ b/aptos-move/framework/aptos-framework/doc/aggregator_v2.md @@ -3,7 +3,17 @@ # Module `0x1::aggregator_v2` -This module provides an interface for aggregators (version 2). +This module provides an interface for aggregators (version 2). Aggregators are +similar to unsigned integers and support addition and subtraction (aborting on +underflow or on overflowing a custom upper limit). The difference from integers +is that aggregators allow to perform both additions and subtractions in parallel +across multiple transactions, enabling parallel execution. For example, if the +first transaction is doing try_add(X, 1) for aggregator X, and the second is +doing try_sub(X,3), they can be executed in parallel avoiding a read-modify-write +dependency. +However, reading the aggregator value (i.e. calling read(X)) is an expensive +operation and should be avoided as much as possible because it reduces the +parallelism. - [Struct `Aggregator`](#0x1_aggregator_v2_Aggregator) @@ -71,6 +81,9 @@ Currently supported types for Element are u64 and u128. ## Struct `AggregatorSnapshot` +Represents a constant value, that was derived from an aggregator at given instant in time. +Unlike read() and storing the value directly, this enables parallel execution of transactions, +while storing snapshot of aggregator state elsewhere. 
struct AggregatorSnapshot<Element> has drop, store
@@ -119,6 +132,17 @@ The value of aggregator underflows (goes below zero). Raised by uncoditional sub
 
 
 
+
+
+The native aggregator function, that is in the move file, is not yet supported.
+and any calls will raise this error.
+
+
+
const EAGGREGATOR_FUNCTION_NOT_YET_SUPPORTED: u64 = 9;
+

+
+
+
 
 
 The aggregator snapshots feature flag is not enabled.
@@ -328,6 +352,7 @@ If subtraction would result in a negative value, false is re
 ## Function `read`
 
 Returns a value stored in this aggregator.
+Note: This operation prevents parallelism of the transaction that calls it.
 
 
 
public fun read<IntElement>(aggregator: &aggregator_v2::Aggregator<IntElement>): IntElement
@@ -350,6 +375,8 @@ Returns a value stored in this aggregator.
 
 ## Function `snapshot`
 
+Returns a wrapper of a current value of an aggregator
+Unlike read(), this enables parallel execution of transactions.
 
 
 
public fun snapshot<IntElement>(aggregator: &aggregator_v2::Aggregator<IntElement>): aggregator_v2::AggregatorSnapshot<IntElement>
@@ -372,6 +399,8 @@ Returns a value stored in this aggregator.
 
 ## Function `create_snapshot`
 
+Creates a snapshot of a given value.
+Useful for when object is sometimes created via snapshot() or string_concat(), and sometimes directly.
 
 
 
public fun create_snapshot<Element: copy, drop>(value: Element): aggregator_v2::AggregatorSnapshot<Element>
@@ -394,6 +423,7 @@ Returns a value stored in this aggregator.
 
 ## Function `copy_snapshot`
 
+NOT YET IMPLEMENTED, always raises EAGGREGATOR_FUNCTION_NOT_YET_SUPPORTED.
 
 
 
public fun copy_snapshot<Element: copy, drop>(snapshot: &aggregator_v2::AggregatorSnapshot<Element>): aggregator_v2::AggregatorSnapshot<Element>
@@ -416,6 +446,8 @@ Returns a value stored in this aggregator.
 
 ## Function `read_snapshot`
 
+Returns a value stored in this snapshot.
+Note: This operation prevents parallelism of the transaction that calls it.
 
 
 
public fun read_snapshot<Element>(snapshot: &aggregator_v2::AggregatorSnapshot<Element>): Element
diff --git a/aptos-move/framework/aptos-framework/sources/aggregator_v2/aggregator_v2.move b/aptos-move/framework/aptos-framework/sources/aggregator_v2/aggregator_v2.move
index 4ecbfac6e9e33e..ad14573797f1c5 100644
--- a/aptos-move/framework/aptos-framework/sources/aggregator_v2/aggregator_v2.move
+++ b/aptos-move/framework/aptos-framework/sources/aggregator_v2/aggregator_v2.move
@@ -1,4 +1,14 @@
-/// This module provides an interface for aggregators (version 2).
+/// This module provides an interface for aggregators (version 2). Aggregators are
+/// similar to unsigned integers and support addition and subtraction (aborting on
+/// underflow or on overflowing a custom upper limit). The difference from integers
+/// is that aggregators allow to perform both additions and subtractions in parallel
+/// across multiple transactions, enabling parallel execution. For example, if the
+/// first transaction is doing `try_add(X, 1)` for aggregator `X`, and the second is
+/// doing `try_sub(X,3)`, they can be executed in parallel avoiding a read-modify-write
+/// dependency.
+/// However, reading the aggregator value (i.e. calling `read(X)`) is an expensive
+/// operation and should be avoided as much as possible because it reduces the
+/// parallelism.
 module aptos_framework::aggregator_v2 {
     use std::error;
     use std::string::String;
@@ -18,6 +28,10 @@ module aptos_framework::aggregator_v2 {
     /// The generic type supplied to the aggregator is not supported.
     const EUNSUPPORTED_AGGREGATOR_TYPE: u64 = 7;
 
+    /// The native aggregator function, that is in the move file, is not yet supported.
+    /// and any calls will raise this error.
+    const EAGGREGATOR_FUNCTION_NOT_YET_SUPPORTED: u64 = 9;
+
     /// Represents an integer which supports parallel additions and subtractions
     /// across multiple transactions. See the module description for more details.
     ///
@@ -27,6 +41,9 @@ module aptos_framework::aggregator_v2 {
         max_value: IntElement,
     }
 
+    /// Represents a constant value, that was derived from an aggregator at given instant in time.
+    /// Unlike read() and storing the value directly, this enables parallel execution of transactions,
+    /// while storing snapshot of aggregator state elsewhere.
     struct AggregatorSnapshot has store, drop {
         value: Element,
     }
@@ -70,14 +87,22 @@ module aptos_framework::aggregator_v2 {
     }
 
     /// Returns a value stored in this aggregator.
+    /// Note: This operation prevents parallelism of the transaction that calls it.
     public native fun read(aggregator: &Aggregator): IntElement;
 
+    /// Returns a wrapper of a current value of an aggregator
+    /// Unlike read(), this enables parallel execution of transactions.
     public native fun snapshot(aggregator: &Aggregator): AggregatorSnapshot;
 
+    /// Creates a snapshot of a given value.
+    /// Useful for when object is sometimes created via snapshot() or string_concat(), and sometimes directly.
     public native fun create_snapshot(value: Element): AggregatorSnapshot;
 
+    /// NOT YET IMPLEMENTED, always raises EAGGREGATOR_FUNCTION_NOT_YET_SUPPORTED.
     public native fun copy_snapshot(snapshot: &AggregatorSnapshot): AggregatorSnapshot;
 
+    /// Returns a value stored in this snapshot.
+    /// Note: This operation prevents parallelism of the transaction that calls it.
     public native fun read_snapshot(snapshot: &AggregatorSnapshot): Element;
 
     /// Concatenates `before`, `snapshot` and `after` into a single string.
@@ -92,9 +117,10 @@ module aptos_framework::aggregator_v2 {
         features::change_feature_flags(fx, vector[feature], vector[]);
 
         let snapshot = create_snapshot(42);
-        let snapshot2 = copy_snapshot(&snapshot);
+        // copy not yet supported
+        // let snapshot2 = copy_snapshot(&snapshot);
         assert!(read_snapshot(&snapshot) == 42, 0);
-        assert!(read_snapshot(&snapshot2) == 42, 0);
+        // assert!(read_snapshot(&snapshot2) == 42, 0);
     }
 
     #[test(fx = @std)]
@@ -104,9 +130,10 @@ module aptos_framework::aggregator_v2 {
         features::change_feature_flags(fx, vector[feature], vector[]);
 
         let snapshot = create_snapshot(std::string::utf8(b"42"));
-        let snapshot2 = copy_snapshot(&snapshot);
+        // copy not yet supported
+        // let snapshot2 = copy_snapshot(&snapshot);
         assert!(read_snapshot(&snapshot) == std::string::utf8(b"42"), 0);
-        assert!(read_snapshot(&snapshot2) == std::string::utf8(b"42"), 0);
+        // assert!(read_snapshot(&snapshot2) == std::string::utf8(b"42"), 0);
     }
 
     #[test(fx = @std)]
@@ -121,6 +148,7 @@ module aptos_framework::aggregator_v2 {
     }
 
     #[test(fx = @std)]
+    #[expected_failure(abort_code = 0x030005, location = Self)]
     public fun test_string_concat2(fx: &signer) {
         use std::features;
         let feature = features::get_aggregator_snapshots_feature();
@@ -128,12 +156,18 @@ module aptos_framework::aggregator_v2 {
 
         let snapshot = create_snapshot(std::string::utf8(b"42"));
         let snapshot2 = string_concat(std::string::utf8(b"before"), &snapshot, std::string::utf8(b"after"));
-        assert!(read_snapshot(&snapshot2) == std::string::utf8(b"before42after"), 0);
+        read_snapshot(&snapshot2);
+        // String concatenation not supported
+        // assert!(read_snapshot(&snapshot2) == std::string::utf8(b"before42after"), 0);
     }
 
     #[test]
     #[expected_failure(abort_code = 0x030006, location = Self)]
     public fun test_snapshot_feature_not_enabled() {
+        use std::features;
+        let feature = features::get_aggregator_snapshots_feature();
+        features::change_feature_flags(fx, vector[], vector[feature]);
+
         create_snapshot(42);
     }
 
diff --git a/aptos-move/framework/src/natives/aggregator_natives/aggregator_v2.rs b/aptos-move/framework/src/natives/aggregator_natives/aggregator_v2.rs
index 86f499e2aa63cd..03063766d71334 100644
--- a/aptos-move/framework/src/natives/aggregator_natives/aggregator_v2.rs
+++ b/aptos-move/framework/src/natives/aggregator_natives/aggregator_v2.rs
@@ -50,7 +50,7 @@ fn is_string_type(context: &SafeNativeContext, type_arg: &Type) -> SafeNativeRes
     Ok(false)
 }
 
-/// Given the list of native function arguments and a type, returns a tuple of its
+/// Given the native function argument and a type, returns a tuple of its
 /// fields: (`aggregator id`, `limit`).
 pub fn get_aggregator_fields_by_type(
     ty_arg: &Type,