feat(lazer/sui): better parsing and error handling (#3003)

* feat(lazer/sui): better parsing and error handling

* nit: comments

* remove invalid channel enum variant

lazer/contracts/sui/sources/channel.move

@@ -1,17 +1,14 @@
 module pyth_lazer::channel;
 
+// Error codes for channel parsing
+const EInvalidChannel: u64 = 1;
+
 public enum Channel has copy, drop {
-    Invalid,
     RealTime,
     FixedRate50ms,
     FixedRate200ms,
 }
 
-/// Create a new Invalid channel
-public fun new_invalid(): Channel {
-    Channel::Invalid
-}
-
 /// Create a new RealTime channel
 public fun new_real_time(): Channel {
     Channel::RealTime
@@ -27,11 +24,16 @@ public fun new_fixed_rate_200ms(): Channel {
     Channel::FixedRate200ms
 }
 
-/// Check if the channel is Invalid
-public fun is_invalid(channel: &Channel): bool {
-    match (channel) {
-        Channel::Invalid => true,
-        _ => false,
+/// Parse channel from a channel value byte
+public fun from_u8(channel_value: u8): Channel {
+    if (channel_value == 1) {
+        new_real_time()
+    } else if (channel_value == 2) {
+        new_fixed_rate_50ms()
+    } else if (channel_value == 3) {
+        new_fixed_rate_200ms()
+    } else {
+        abort EInvalidChannel
     }
 }
 

lazer/contracts/sui/sources/feed.move

@@ -1,7 +1,11 @@
 module pyth_lazer::feed;
 
-use pyth_lazer::i16::I16;
-use pyth_lazer::i64::I64;
+use pyth_lazer::i16::{Self, I16};
+use pyth_lazer::i64::{Self, I64};
+use sui::bcs;
+
+// Error codes for feed parsing
+const EInvalidProperty: u64 = 2;
 
 /// The feed struct is based on the Lazer rust protocol definition defined here:
 /// https://github.com/pyth-network/pyth-crosschain/blob/main/lazer/sdk/rust/protocol/src/payload.rs
@@ -56,7 +60,7 @@ public(package) fun new(
         confidence,
         funding_rate,
         funding_timestamp,
-        funding_rate_interval
+        funding_rate_interval,
     }
 }
 
@@ -156,6 +160,115 @@ public(package) fun set_funding_timestamp(feed: &mut Feed, funding_timestamp: Op
 }
 
 /// Set the funding rate interval
-public(package) fun set_funding_rate_interval(feed: &mut Feed, funding_rate_interval: Option<Option<u64>>) {
+public(package) fun set_funding_rate_interval(
+    feed: &mut Feed,
+    funding_rate_interval: Option<Option<u64>>,
+) {
     feed.funding_rate_interval = funding_rate_interval;
 }
+
+/// Parse a feed from a BCS cursor
+public(package) fun parse_from_cursor(cursor: &mut bcs::BCS): Feed {
+    let feed_id = cursor.peel_u32();
+    let mut feed = new(
+        feed_id,
+        option::none(),
+        option::none(),
+        option::none(),
+        option::none(),
+        option::none(),
+        option::none(),
+        option::none(),
+        option::none(),
+        option::none(),
+    );
+
+    let properties_count = cursor.peel_u8();
+    let mut properties_i = 0;
+
+    while (properties_i < properties_count) {
+        let property_id = cursor.peel_u8();
+
+        if (property_id == 0) {
+            // Price property
+            let price = cursor.peel_u64();
+            if (price != 0) {
+                feed.set_price(option::some(option::some(i64::from_u64(price))));
+            } else {
+                feed.set_price(option::some(option::none()));
+            }
+        } else if (property_id == 1) {
+            // Best bid price property
+            let best_bid_price = cursor.peel_u64();
+            if (best_bid_price != 0) {
+                feed.set_best_bid_price(
+                    option::some(option::some(i64::from_u64(best_bid_price))),
+                );
+            } else {
+                feed.set_best_bid_price(option::some(option::none()));
+            }
+        } else if (property_id == 2) {
+            // Best ask price property
+            let best_ask_price = cursor.peel_u64();
+            if (best_ask_price != 0) {
+                feed.set_best_ask_price(
+                    option::some(option::some(i64::from_u64(best_ask_price))),
+                );
+            } else {
+                feed.set_best_ask_price(option::some(option::none()));
+            }
+        } else if (property_id == 3) {
+            // Publisher count property
+            let publisher_count = cursor.peel_u16();
+            feed.set_publisher_count(option::some(publisher_count));
+        } else if (property_id == 4) {
+            // Exponent property
+            let exponent = cursor.peel_u16();
+            feed.set_exponent(option::some(i16::from_u16(exponent)));
+        } else if (property_id == 5) {
+            // Confidence property
+            let confidence = cursor.peel_u64();
+            if (confidence != 0) {
+                feed.set_confidence(option::some(option::some(i64::from_u64(confidence))));
+            } else {
+                feed.set_confidence(option::some(option::none()));
+            }
+        } else if (property_id == 6) {
+            // Funding rate property
+            let exists = cursor.peel_bool();
+            if (exists) {
+                let funding_rate = cursor.peel_u64();
+                feed.set_funding_rate(option::some(option::some(i64::from_u64(funding_rate))));
+            } else {
+                feed.set_funding_rate(option::some(option::none()));
+            }
+        } else if (property_id == 7) {
+            // Funding timestamp property
+            let exists = cursor.peel_bool();
+            if (exists) {
+                let funding_timestamp = cursor.peel_u64();
+                feed.set_funding_timestamp(option::some(option::some(funding_timestamp)));
+            } else {
+                feed.set_funding_timestamp(option::some(option::none()));
+            }
+        } else if (property_id == 8) {
+            // Funding rate interval property
+            let exists = cursor.peel_bool();
+            if (exists) {
+                let funding_rate_interval = cursor.peel_u64();
+                feed.set_funding_rate_interval(
+                    option::some(option::some(funding_rate_interval)),
+                );
+            } else {
+                feed.set_funding_rate_interval(option::some(option::none()));
+            }
+        } else {
+            // Unknown property - we cannot safely skip it without knowing its length
+            abort EInvalidProperty
+        };
+
+        properties_i = properties_i + 1;
+    };
+
+    feed
+}

lazer/contracts/sui/sources/pyth_lazer.move

@@ -1,10 +1,6 @@
 module pyth_lazer::pyth_lazer;
 
-use pyth_lazer::channel;
-use pyth_lazer::feed::{Self, Feed};
 use pyth_lazer::state::{Self, State};
-use pyth_lazer::i64::{Self};
-use pyth_lazer::i16::{Self};
 use pyth_lazer::update::{Self, Update};
 use sui::bcs;
 use sui::clock::Clock;
@@ -15,12 +11,10 @@ const UPDATE_MESSAGE_MAGIC: u32 = 1296547300;
 const PAYLOAD_MAGIC: u32 = 2479346549;
 
 // Error codes
-const EInvalidUpdate: u64 = 1;
 const ESignerNotTrusted: u64 = 2;
 const ESignerExpired: u64 = 3;
-
-// TODO:
-// error handling
+const EInvalidMagic: u64 = 4;
+const EInvalidPayloadLength: u64 = 6;
 
 /// The `PYTH_LAZER` resource serves as the one-time witness.
 /// It has the `drop` ability, allowing it to be consumed immediately after use.
@@ -83,161 +77,39 @@ public(package) fun verify_le_ecdsa_message(
 /// * `update` - The LeEcdsa formatted Lazer update
 ///
 /// # Errors
-/// * `EInvalidUpdate` - Failed to parse the update according to the protocol definition
+/// * `EInvalidMagic` - Invalid magic number in update or payload
+/// * `EInvalidPayloadLength` - Payload length doesn't match actual data
 /// * `ESignerNotTrusted` - The recovered public key is not in the trusted signers list
+/// * `ESignerExpired` - The signer's certificate has expired
 public fun parse_and_verify_le_ecdsa_update(s: &State, clock: &Clock, update: vector<u8>): Update {
     let mut cursor = bcs::new(update);
 
-    // TODO: introduce helper functions to check data len before peeling. allows us to return more
-    // granular error messages.
-
+    // Parse and validate message magic
     let magic = cursor.peel_u32();
-    assert!(magic == UPDATE_MESSAGE_MAGIC, 0);
+    assert!(magic == UPDATE_MESSAGE_MAGIC, EInvalidMagic);
 
+    // Parse signature
     let mut signature = vector::empty<u8>();
-
     let mut sig_i = 0;
     while (sig_i < SECP256K1_SIG_LEN) {
         signature.push_back(cursor.peel_u8());
         sig_i = sig_i + 1;
     };
 
+    // Parse expected payload length and get remaining bytes as payload
     let payload_len = cursor.peel_u16();
-
     let payload = cursor.into_remainder_bytes();
 
-    assert!((payload_len as u64) == payload.length(), 0);
-
-    let mut cursor = bcs::new(payload);
-    let payload_magic = cursor.peel_u32();
-    assert!(payload_magic == PAYLOAD_MAGIC, 0);
+    // Validate expectedpayload length
+    assert!((payload_len as u64) == payload.length(), EInvalidPayloadLength);
 
-    let timestamp = cursor.peel_u64();
+    // Parse payload
+    let mut payload_cursor = bcs::new(payload);
+    let payload_magic = payload_cursor.peel_u32();
+    assert!(payload_magic == PAYLOAD_MAGIC, EInvalidMagic);
 
     // Verify the signature against trusted signers
     verify_le_ecdsa_message(s, clock, &signature, &payload);
 
-    let channel_value = cursor.peel_u8();
-    let channel = if (channel_value == 0) {
-        channel::new_invalid()
-    } else if (channel_value == 1) {
-        channel::new_real_time()
-    } else if (channel_value == 2) {
-        channel::new_fixed_rate_50ms()
-    } else if (channel_value == 3) {
-        channel::new_fixed_rate_200ms()
-    } else {
-        channel::new_invalid() // Default to Invalid for unknown values
-    };
-
-    let mut feeds = vector::empty<Feed>();
-    let mut feed_i = 0;
-
-    let feed_count = cursor.peel_u8();
-
-    while (feed_i < feed_count) {
-        let feed_id = cursor.peel_u32();
-        let mut feed = feed::new(
-            feed_id,
-            option::none(),
-            option::none(),
-            option::none(),
-            option::none(),
-            option::none(),
-            option::none(),
-            option::none(),
-            option::none(),
-            option::none(),
-        );
-
-        let properties_count = cursor.peel_u8();
-        let mut properties_i = 0;
-
-        while (properties_i < properties_count) {
-            let property_id = cursor.peel_u8();
-
-            if (property_id == 0) {
-                let price = cursor.peel_u64();
-                if (price != 0) {
-                    feed.set_price(option::some(option::some(i64::from_u64(price))));
-                } else {
-                    feed.set_price(option::some(option::none()));
-                }
-            } else if (property_id == 1) {
-                let best_bid_price = cursor.peel_u64();
-                if (best_bid_price != 0) {
-                    feed.set_best_bid_price(
-                        option::some(option::some(i64::from_u64(best_bid_price))),
-                    );
-                } else {
-                    feed.set_best_bid_price(option::some(option::none()));
-                }
-            } else if (property_id == 2) {
-                let best_ask_price = cursor.peel_u64();
-                if (best_ask_price != 0) {
-                    feed.set_best_ask_price(
-                        option::some(option::some(i64::from_u64(best_ask_price))),
-                    );
-                } else {
-                    feed.set_best_ask_price(option::some(option::none()));
-                }
-            } else if (property_id == 3) {
-                let publisher_count = cursor.peel_u16();
-                feed.set_publisher_count(option::some(publisher_count));
-            } else if (property_id == 4) {
-                let exponent = cursor.peel_u16();
-                feed.set_exponent(option::some(i16::from_u16(exponent)));
-            } else if (property_id == 5) {
-                let confidence = cursor.peel_u64();
-                if (confidence != 0) {
-                    feed.set_confidence(option::some(option::some(i64::from_u64(confidence))));
-                } else {
-                    feed.set_confidence(option::some(option::none()));
-                }
-            } else if (property_id == 6) {
-                let exists = cursor.peel_u8();
-                if (exists == 1) {
-                    let funding_rate = cursor.peel_u64();
-                    feed.set_funding_rate(option::some(option::some(i64::from_u64(funding_rate))));
-                } else {
-                    feed.set_funding_rate(option::some(option::none()));
-                }
-            } else if (property_id == 7) {
-                let exists = cursor.peel_u8();
-
-                if (exists == 1) {
-                    let funding_timestamp = cursor.peel_u64();
-                    feed.set_funding_timestamp(option::some(option::some(funding_timestamp)));
-                } else {
-                    feed.set_funding_timestamp(option::some(option::none()));
-                }
-            } else if (property_id == 8) {
-                let exists = cursor.peel_u8();
-
-                if (exists == 1) {
-                    let funding_rate_interval = cursor.peel_u64();
-                    feed.set_funding_rate_interval(
-                        option::some(option::some(funding_rate_interval)),
-                    );
-                } else {
-                    feed.set_funding_rate_interval(option::some(option::none()));
-                }
-            } else {
-                // When we have an unknown property, we do not know its length, and therefore
-                // we cannot ignore it and parse the next properties.
-                abort EInvalidUpdate // FIXME: return more granular error messages
-            };
-
-            properties_i = properties_i + 1;
-        };
-
-        vector::push_back(&mut feeds, feed);
-
-        feed_i = feed_i + 1;
-    };
-
-    let remaining_bytes = cursor.into_remainder_bytes();
-    assert!(remaining_bytes.length() == 0, 0);
-
-    update::new(timestamp, channel, feeds)
+    update::parse_from_cursor(payload_cursor)
 }

lazer/contracts/sui/sources/update.move

@@ -1,7 +1,11 @@
 module pyth_lazer::update;
 
-use pyth_lazer::channel::Channel;
-use pyth_lazer::feed::Feed;
+use pyth_lazer::channel::{Self, Channel};
+use pyth_lazer::feed::{Self, Feed};
+use sui::bcs;
+
+// Error codes for update parsing
+const EInvalidPayload: u64 = 3;
 
 public struct Update has copy, drop {
     timestamp: u64,
@@ -9,11 +13,7 @@ public struct Update has copy, drop {
     feeds: vector<Feed>,
 }
 
-public(package) fun new(
-    timestamp: u64,
-    channel: Channel,
-    feeds: vector<Feed>
-): Update {
+public(package) fun new(timestamp: u64, channel: Channel, feeds: vector<Feed>): Update {
     Update { timestamp, channel, feeds }
 }
 
@@ -31,3 +31,31 @@ public fun channel(update: &Update): Channel {
 public fun feeds(update: &Update): vector<Feed> {
     update.feeds
 }
+
+/// Parse the update from a BCS cursor containing the payload data
+/// This assumes the payload magic has already been validated and consumed
+public(package) fun parse_from_cursor(mut cursor: bcs::BCS): Update {
+    // Parse timestamp
+    let timestamp = cursor.peel_u64();
+    
+    // Parse channel
+    let channel_value = cursor.peel_u8();
+    let channel = channel::from_u8(channel_value);
+    
+    // Parse feeds
+    let feed_count = cursor.peel_u8();
+    let mut feeds = vector::empty<Feed>();
+    let mut feed_i = 0;
+    
+    while (feed_i < feed_count) {
+        let feed = feed::parse_from_cursor(&mut cursor);
+        vector::push_back(&mut feeds, feed);
+        feed_i = feed_i + 1;
+    };
+    
+    // Verify no remaining bytes
+    let remaining_bytes = cursor.into_remainder_bytes();
+    assert!(remaining_bytes.length() == 0, EInvalidPayload);
+    
+    Update { timestamp, channel, feeds }
+}

lazer/contracts/sui/tests/pyth_lazer_tests.move

@@ -6,7 +6,7 @@ use pyth_lazer::admin;
 use pyth_lazer::channel::new_fixed_rate_200ms;
 use pyth_lazer::i16;
 use pyth_lazer::i64;
-use pyth_lazer::pyth_lazer::{parse_and_verify_le_ecdsa_update, verify_le_ecdsa_message, ESignerNotTrusted, ESignerExpired};
+use pyth_lazer::pyth_lazer::{parse_and_verify_le_ecdsa_update, verify_le_ecdsa_message, ESignerNotTrusted, ESignerExpired, EInvalidMagic, EInvalidPayloadLength};
 use pyth_lazer::state;
 use sui::clock;
 
@@ -227,3 +227,112 @@ public fun test_verify_le_ecdsa_message_multiple_signers() {
     admin::destroy_for_test(admin_cap);
     clock::destroy_for_testing(clock);
 }
+
+// === NEGATIVE PARSING TESTS ===
+
+#[test, expected_failure(abort_code = EInvalidMagic)]
+public fun test_parse_invalid_update_magic() {
+    let mut ctx = tx_context::dummy();
+    let mut s = state::new_for_test(&mut ctx);
+    let admin_cap = admin::mint_for_test(&mut ctx);
+    let clock = clock::create_for_testing(&mut ctx);
+
+    // Add the trusted signer
+    let trusted_pubkey = TEST_TRUSTED_SIGNER_PUBKEY;
+    let expiry_time = 2000000000000000; // Far in the future
+    state::update_trusted_signer(&admin_cap, &mut s, trusted_pubkey, expiry_time);
+
+    // Create update with invalid magic (first 4 bytes corrupted)
+    let mut invalid_update = TEST_LAZER_UPDATE;
+    *vector::borrow_mut(&mut invalid_update, 0) = 0xFF; // Corrupt the magic
+
+    // This should fail with EInvalidMagic
+    parse_and_verify_le_ecdsa_update(&s, &clock, invalid_update);
+
+    // Clean up
+    state::destroy_for_test(s);
+    admin::destroy_for_test(admin_cap);
+    clock::destroy_for_testing(clock);
+}
+
+#[test, expected_failure(abort_code = EInvalidMagic)]
+public fun test_parse_invalid_payload_magic() {
+    let mut ctx = tx_context::dummy();
+    let mut s = state::new_for_test(&mut ctx);
+    let admin_cap = admin::mint_for_test(&mut ctx);
+    let clock = clock::create_for_testing(&mut ctx);
+
+    // Add the trusted signer
+    let trusted_pubkey = TEST_TRUSTED_SIGNER_PUBKEY;
+    let expiry_time = 2000000000000000; // Far in the future
+    state::update_trusted_signer(&admin_cap, &mut s, trusted_pubkey, expiry_time);
+
+    // Create update with invalid payload magic
+    // The payload magic starts at byte 69 (4 bytes magic + 65 bytes signature + 2 payload length)
+    let mut invalid_update = TEST_LAZER_UPDATE;
+    *vector::borrow_mut(&mut invalid_update, 71) = 0xFF; // Corrupt the payload magic
+
+    // This corrupts the payload magic, so expect EInvalidMagic
+    parse_and_verify_le_ecdsa_update(&s, &clock, invalid_update);
+
+    // Clean up
+    state::destroy_for_test(s);
+    admin::destroy_for_test(admin_cap);
+    clock::destroy_for_testing(clock);
+}
+
+#[test, expected_failure(abort_code = EInvalidPayloadLength)]
+public fun test_parse_invalid_payload_length() {
+    let mut ctx = tx_context::dummy();
+    let mut s = state::new_for_test(&mut ctx);
+    let admin_cap = admin::mint_for_test(&mut ctx);
+    let clock = clock::create_for_testing(&mut ctx);
+
+    // Add the trusted signer
+    let trusted_pubkey = TEST_TRUSTED_SIGNER_PUBKEY;
+    let expiry_time = 2000000000000000; // Far in the future
+    state::update_trusted_signer(&admin_cap, &mut s, trusted_pubkey, expiry_time);
+
+    // Create update with wrong payload length 
+    // Layout: magic(4) + signature(65) + payload_len(2) + payload...
+    // So payload length is at bytes 69-70
+    let mut invalid_update = TEST_LAZER_UPDATE;
+    *vector::borrow_mut(&mut invalid_update, 69) = 0xFF; // Set payload length too high
+
+    // This should fail with EInvalidPayloadLength because payload length validation happens before signature verification
+    parse_and_verify_le_ecdsa_update(&s, &clock, invalid_update);
+
+    // Clean up
+    state::destroy_for_test(s);
+    admin::destroy_for_test(admin_cap);
+    clock::destroy_for_testing(clock);
+}
+
+#[test, expected_failure(abort_code = 0, location = sui::bcs)]
+public fun test_parse_truncated_data() {
+    let mut ctx = tx_context::dummy();
+    let mut s = state::new_for_test(&mut ctx);
+    let admin_cap = admin::mint_for_test(&mut ctx);
+    let clock = clock::create_for_testing(&mut ctx);
+
+    // Add the trusted signer
+    let trusted_pubkey = TEST_TRUSTED_SIGNER_PUBKEY;
+    let expiry_time = 2000000000000000; // Far in the future
+    state::update_trusted_signer(&admin_cap, &mut s, trusted_pubkey, expiry_time);
+
+    // Create truncated update (only first 50 bytes)
+    let mut truncated_update = vector::empty<u8>();
+    let mut i = 0;
+    while (i < 50) {
+        vector::push_back(&mut truncated_update, *vector::borrow(&TEST_LAZER_UPDATE, i));
+        i = i + 1;
+    };
+
+    // This should fail with BCS EOutOfRange error when trying to read beyond available data
+    parse_and_verify_le_ecdsa_update(&s, &clock, truncated_update);
+
+    // Clean up
+    state::destroy_for_test(s);
+    admin::destroy_for_test(admin_cap);
+    clock::destroy_for_testing(clock);
+}