Allow negative prices in aggregation (#344)

* make it build

* allow negative prices

* clean up comments

* doc

* fixme

* cleanup

* fix aggregation tests

* fix ema stuff too

* delet

Author: jayantk

README.md

@@ -145,4 +145,3 @@ root@pyth-dev# usermod -u 1002 -g 1004 -s /bin/bash pyth
 ```
 
 Finally, in docker extension inside VS Code click right and choose "Attach VS Code". If you're using a remote host in VS Code make sure to let this connection be open.
-

program/c/src/oracle/upd_aggregate.h

@@ -102,19 +102,12 @@ static void upd_ema(
     // compute numer/denom and new value from decay factor
     pd_load( numer, ptr->numer_ );
     pd_load( denom, ptr->denom_ );
-    if ( numer->v_ < 0 || denom->v_ < 0 ) {
-      // temporary reset twap on negative value
-      pd_set( numer, val );
-      pd_set( denom, one );
-    }
-    else {
-      pd_mul( numer, numer, decay );
-      pd_mul( wval, val, cwgt );
-      pd_add( numer, numer, wval, qs->fact_ );
-      pd_mul( denom, denom, decay );
-      pd_add( denom, denom, cwgt, qs->fact_ );
-      pd_div( val, numer, denom );
-    }
+    pd_mul( numer, numer, decay );
+    pd_mul( wval, val, cwgt );
+    pd_add( numer, numer, wval, qs->fact_ );
+    pd_mul( denom, denom, decay );
+    pd_add( denom, denom, cwgt, qs->fact_ );
+    pd_div( val, numer, denom );
   }
 
   // adjust and store results
@@ -180,12 +173,14 @@ static inline bool upd_aggregate( pc_price_t *ptr, uint64_t slot, int64_t timest
       int64_t price     = iptr->agg_.price_;
       int64_t conf      = ( int64_t )( iptr->agg_.conf_ );
       if ( iptr->agg_.status_ == PC_STATUS_TRADING &&
-           (int64_t)0 < conf && conf < price && conf <= (INT64_MAX-price) && // No overflow for INT64_MAX-price as price>0
+           // No overflow for INT64_MIN+conf or INT64_MAX-conf as 0 < conf < INT64_MAX
+           // These checks ensure that price - conf and price + conf do not overflow.
+           (int64_t)0 < conf && (INT64_MIN + conf) <= price && price <= (INT64_MAX-conf) &&
            slot_diff >= 0 && slot_diff <= PC_MAX_SEND_LATENCY ) {
         numv += 1;
-        prcs[ nprcs++ ] = price - conf; // No overflow as 0 < conf < price
+        prcs[ nprcs++ ] = price - conf;
         prcs[ nprcs++ ] = price;
-        prcs[ nprcs++ ] = price + conf; // No overflow as 0 < conf <= INT64_MAX-price
+        prcs[ nprcs++ ] = price + conf;
       }
     }
 

program/rust/src/tests/test_ema.rs

@@ -1,4 +1,5 @@
 extern crate test_generator;
+
 use {
     crate::{
         accounts::PriceAccount,
@@ -20,6 +21,41 @@ use {
 
 #[test_resources("program/rust/test_data/ema/*.csv")]
 fn test_ema(input_path_raw: &str) {
+    let (inputs, expected_outputs) = read_test_data(input_path_raw);
+    run_ema_test(&inputs, &expected_outputs);
+}
+
+// This test is identical to the one above, except that it negates all of the prices.
+#[test_resources("program/rust/test_data/ema/*.csv")]
+fn test_ema_negated(input_path_raw: &str) {
+    let (inputs, expected_outputs) = read_test_data(input_path_raw);
+
+    let modified_inputs: Vec<InputRecord> = inputs
+        .iter()
+        .map(|x| InputRecord {
+            price:  -x.price,
+            conf:   x.conf,
+            expo:   x.expo,
+            nslots: x.nslots,
+        })
+        .collect();
+
+    let modified_outputs: Vec<OutputRecord> = expected_outputs
+        .iter()
+        .map(|x| OutputRecord {
+            price:  -x.price,
+            conf:   x.conf,
+            expo:   x.expo,
+            nslots: x.nslots,
+            twap:   -x.twap,
+            twac:   x.twac,
+        })
+        .collect();
+
+    run_ema_test(&modified_inputs, &modified_outputs);
+}
+
+fn read_test_data(input_path_raw: &str) -> (Vec<InputRecord>, Vec<OutputRecord>) {
     // For some reason these tests have a different working directory than the macro.
     let input_path = input_path_raw.replace("program/rust/", "");
     let result_path = input_path.replace(".csv", ".result");
@@ -48,6 +84,10 @@ fn test_ema(input_path_raw: &str) {
         "Mismatched CSV file lengths"
     );
 
+    (inputs, expected_outputs)
+}
+
+fn run_ema_test(inputs: &[InputRecord], expected_outputs: &[OutputRecord]) {
     let current_slot = 1000;
     let current_timestamp = 1234;
     let mut price_account: PriceAccount = PriceAccount::zeroed();
@@ -70,6 +110,7 @@ fn test_ema(input_path_raw: &str) {
     }
 }
 
+
 // TODO: put these functions somewhere more accessible
 pub fn upd_aggregate(
     price_account: &mut PriceAccount,

program/rust/src/tests/test_upd_price.rs

@@ -277,6 +277,60 @@ fn test_upd_price() {
         assert_eq!(price_data.agg_.price_, 81);
         assert_eq!(price_data.agg_.status_, PC_STATUS_UNKNOWN);
     }
+
+    // Negative prices are accepted
+    populate_instruction(&mut instruction_data, -100, 1, 7);
+    update_clock_slot(&mut clock_account, 8);
+
+    assert!(process_instruction(
+        &program_id,
+        &[
+            funding_account.clone(),
+            price_account.clone(),
+            clock_account.clone()
+        ],
+        &instruction_data
+    )
+    .is_ok());
+
+    {
+        let price_data = load_checked::<PriceAccount>(&price_account, PC_VERSION).unwrap();
+        assert_eq!(price_data.comp_[0].latest_.price_, -100);
+        assert_eq!(price_data.comp_[0].latest_.conf_, 1);
+        assert_eq!(price_data.comp_[0].latest_.pub_slot_, 7);
+        assert_eq!(price_data.comp_[0].latest_.status_, PC_STATUS_TRADING);
+        assert_eq!(price_data.valid_slot_, 7);
+        assert_eq!(price_data.agg_.pub_slot_, 8);
+        assert_eq!(price_data.agg_.price_, 81);
+        assert_eq!(price_data.agg_.status_, PC_STATUS_UNKNOWN);
+    }
+
+    // Crank again for aggregate
+    populate_instruction(&mut instruction_data, -100, 1, 8);
+    update_clock_slot(&mut clock_account, 9);
+
+    assert!(process_instruction(
+        &program_id,
+        &[
+            funding_account.clone(),
+            price_account.clone(),
+            clock_account.clone()
+        ],
+        &instruction_data
+    )
+    .is_ok());
+
+    {
+        let price_data = load_checked::<PriceAccount>(&price_account, PC_VERSION).unwrap();
+        assert_eq!(price_data.comp_[0].latest_.price_, -100);
+        assert_eq!(price_data.comp_[0].latest_.conf_, 1);
+        assert_eq!(price_data.comp_[0].latest_.pub_slot_, 8);
+        assert_eq!(price_data.comp_[0].latest_.status_, PC_STATUS_TRADING);
+        assert_eq!(price_data.valid_slot_, 8);
+        assert_eq!(price_data.agg_.pub_slot_, 9);
+        assert_eq!(price_data.agg_.price_, -100);
+        assert_eq!(price_data.agg_.status_, PC_STATUS_TRADING);
+    }
 }
 
 // Create an upd_price instruction with the provided parameters

program/rust/test_data/aggregation/2.result

@@ -1 +1 @@
-{"exponent":-3,"price":0,"conf":0,"status":"unknown"}
+{"exponent":-3,"price":-10000,"conf":1000,"status":"trading"}

program/rust/test_data/aggregation/3.result

@@ -1 +1 @@
-{"exponent":-3,"price":0,"conf":0,"status":"unknown"}
+{"exponent":-3,"price":0,"conf":1000,"status":"trading"}

program/rust/test_data/aggregation/31.result

@@ -1 +1 @@
-{"exponent":-8,"price":4329187000000,"conf":3187000000,"status":"trading"}
+{"exponent":-8,"price":4328634500000,"conf":2914500000,"status":"trading"}

program/rust/test_data/aggregation/32.result

@@ -1 +1 @@
-{"exponent":-8,"price":4329402176469,"conf":2206823531,"status":"trading"}
+{"exponent":-8,"price":4329187000000,"conf":3187000000,"status":"trading"}