Fixing tests and more comments

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);
 }

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 <code><a href="aggregator_v2.md#0x1_aggregator_v2_try_add">try_add</a>(X, 1)</code> for aggregator <code>X</code>, and the second is
+doing <code><a href="aggregator_v2.md#0x1_aggregator_v2_try_sub">try_sub</a>(X,3)</code>, they can be executed in parallel avoiding a read-modify-write
+dependency.
+However, reading the aggregator value (i.e. calling <code><a href="aggregator_v2.md#0x1_aggregator_v2_read">read</a>(X)</code>) 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.
 
 
 <pre><code><b>struct</b> <a href="aggregator_v2.md#0x1_aggregator_v2_AggregatorSnapshot">AggregatorSnapshot</a>&lt;Element&gt; <b>has</b> drop, store
@@ -119,6 +132,17 @@ The value of aggregator underflows (goes below zero). Raised by uncoditional sub
 
 
 
+<a name="0x1_aggregator_v2_EAGGREGATOR_FUNCTION_NOT_YET_SUPPORTED"></a>
+
+The native aggregator function, that is in the move file, is not yet supported.
+and any calls will raise this error.
+
+
+<pre><code><b>const</b> <a href="aggregator_v2.md#0x1_aggregator_v2_EAGGREGATOR_FUNCTION_NOT_YET_SUPPORTED">EAGGREGATOR_FUNCTION_NOT_YET_SUPPORTED</a>: u64 = 9;
+</code></pre>
+
+
+
 <a name="0x1_aggregator_v2_EAGGREGATOR_SNAPSHOTS_NOT_ENABLED"></a>
 
 The aggregator snapshots feature flag is not enabled.
@@ -328,6 +352,7 @@ If subtraction would result in a negative value, <code><b>false</b></code> is re
 ## Function `read`
 
 Returns a value stored in this aggregator.
+Note: This operation prevents parallelism of the transaction that calls it.
 
 
 <pre><code><b>public</b> <b>fun</b> <a href="aggregator_v2.md#0x1_aggregator_v2_read">read</a>&lt;IntElement&gt;(<a href="aggregator.md#0x1_aggregator">aggregator</a>: &<a href="aggregator_v2.md#0x1_aggregator_v2_Aggregator">aggregator_v2::Aggregator</a>&lt;IntElement&gt;): 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.
 
 
 <pre><code><b>public</b> <b>fun</b> <a href="aggregator_v2.md#0x1_aggregator_v2_snapshot">snapshot</a>&lt;IntElement&gt;(<a href="aggregator.md#0x1_aggregator">aggregator</a>: &<a href="aggregator_v2.md#0x1_aggregator_v2_Aggregator">aggregator_v2::Aggregator</a>&lt;IntElement&gt;): <a href="aggregator_v2.md#0x1_aggregator_v2_AggregatorSnapshot">aggregator_v2::AggregatorSnapshot</a>&lt;IntElement&gt;
@@ -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.
 
 
 <pre><code><b>public</b> <b>fun</b> <a href="aggregator_v2.md#0x1_aggregator_v2_create_snapshot">create_snapshot</a>&lt;Element: <b>copy</b>, drop&gt;(value: Element): <a href="aggregator_v2.md#0x1_aggregator_v2_AggregatorSnapshot">aggregator_v2::AggregatorSnapshot</a>&lt;Element&gt;
@@ -394,6 +423,7 @@ Returns a value stored in this aggregator.
 
 ## Function `copy_snapshot`
 
+NOT YET IMPLEMENTED, always raises EAGGREGATOR_FUNCTION_NOT_YET_SUPPORTED.
 
 
 <pre><code><b>public</b> <b>fun</b> <a href="aggregator_v2.md#0x1_aggregator_v2_copy_snapshot">copy_snapshot</a>&lt;Element: <b>copy</b>, drop&gt;(snapshot: &<a href="aggregator_v2.md#0x1_aggregator_v2_AggregatorSnapshot">aggregator_v2::AggregatorSnapshot</a>&lt;Element&gt;): <a href="aggregator_v2.md#0x1_aggregator_v2_AggregatorSnapshot">aggregator_v2::AggregatorSnapshot</a>&lt;Element&gt;
@@ -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.
 
 
 <pre><code><b>public</b> <b>fun</b> <a href="aggregator_v2.md#0x1_aggregator_v2_read_snapshot">read_snapshot</a>&lt;Element&gt;(snapshot: &<a href="aggregator_v2.md#0x1_aggregator_v2_AggregatorSnapshot">aggregator_v2::AggregatorSnapshot</a>&lt;Element&gt;): Element

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<Element> 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<IntElement>(aggregator: &Aggregator<IntElement>): IntElement;
 
+    /// Returns a wrapper of a current value of an aggregator
+    /// Unlike read(), this enables parallel execution of transactions.
     public native fun snapshot<IntElement>(aggregator: &Aggregator<IntElement>): AggregatorSnapshot<IntElement>;
 
+    /// 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<Element: copy + drop>(value: Element): AggregatorSnapshot<Element>;
 
+    /// NOT YET IMPLEMENTED, always raises EAGGREGATOR_FUNCTION_NOT_YET_SUPPORTED.
     public native fun copy_snapshot<Element: copy + drop>(snapshot: &AggregatorSnapshot<Element>): AggregatorSnapshot<Element>;
 
+    /// Returns a value stored in this snapshot.
+    /// Note: This operation prevents parallelism of the transaction that calls it.
     public native fun read_snapshot<Element>(snapshot: &AggregatorSnapshot<Element>): 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,19 +148,26 @@ 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();
         features::change_feature_flags(fx, vector[feature], vector[]);
 
         let snapshot = create_snapshot<String>(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);
     }
 

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,