feat: starknet GMP (INTERNAL REVIEW) #57

ctoyan · 2025-01-10T09:17:04Z

No description provided.

roberts-pumpurs · 2025-01-10T09:23:47Z

packages/starknet-types/src/events/signers_rotated.rs

+        // while let Some(result) = futures.next().await {
+            // if !result {
+                // panic!("expected conversion to fail for malformed event");
+            // }
+        // }


Suggested change

// while let Some(result) = futures.next().await {

// if !result {

// panic!("expected conversion to fail for malformed event");

// }

// }

roberts-pumpurs · 2025-01-10T09:30:05Z

ampd/src/starknet/json_rpc.rs

+
+        let event: Option<(Felt, ContractCallEvent)> = match receipt_with_block_info.receipt {
+            TransactionReceipt::Invoke(tx) => {
+                // NOTE: There should be only one ContractCall event per gateway tx


What is meant by gateway tx? Because there can be many ContractCalls in a single tx

not sure about your question, but there would be more ContractCalls only in scenario where within single transaction frame we call multiple times Gateway::ContractCall, no? otherwise it should be one event per transaction.

Same question as Marcin. I believe we expect a single ContractCall per tx.

Is multicall what you're referring to and are you familiar with it's use case and if we should actually implement it?
I remember that Squid might need it, but I have no details on the matter.

but there would be more ContractCalls only in scenario where within single transaction frame we call multiple times Gateway::ContractCall

Exactly, and based on the //NOTE: comment I assume that this is not covered 😄

Multicall is one example.
Another example would be ITS, which allows to call arbitrary contracts upon token bridiging with a custom payload (all within the same tx), these contracts can be user-made contracts, or even the Multicall contract that calls the Gateway eg 3x times. What if I want to send 10 memo messages from starknet to EVM by only having to sign & send a single tx on Starknet 😄

summary: I don't think any of the implementations (evm, Solana, multiversx) assume that there would be only a single ContractCall event per on-chain tx, and I dont think you should assume that too.

roberts-pumpurs · 2025-01-10T09:38:43Z

ampd/src/handlers/starknet_verify_verifier_set.rs

+    ///
+    /// # Type Parameters
+    /// * `C` - A Starknet client type that implements the [`StarknetClient`] trait
+    #[allow(dead_code)]


Suggested change

#[allow(dead_code)]

puhtaytow

LGTM, left some comments

puhtaytow · 2025-01-10T09:59:23Z

ampd/src/starknet/json_rpc.rs

+
+        let event: Option<(Felt, ContractCallEvent)> = match receipt_with_block_info.receipt {
+            TransactionReceipt::Invoke(tx) => {
+                // NOTE: There should be only one ContractCall event per gateway tx


not sure about your question, but there would be more ContractCalls only in scenario where within single transaction frame we call multiple times Gateway::ContractCall, no? otherwise it should be one event per transaction.

puhtaytow · 2025-01-10T10:04:21Z

ampd/src/starknet/verifier.rs

+use crate::handlers::starknet_verify_msg::Message;
+use crate::handlers::starknet_verify_verifier_set::VerifierSetConfirmation;
+
+/// Attempts to fetch the tx provided in `axl_msg.tx_id`.


I would add full Docs body

puhtaytow · 2025-01-10T10:04:30Z

ampd/src/starknet/verifier.rs

+    }
+}
+
+pub fn verify_verifier_set(


I would add full Docs body here as well

puhtaytow · 2025-01-10T10:08:05Z

packages/starknet-types/src/events/signers_rotated.rs

+    /// Error returned when a required signers hash is missing from a
+    /// transaction.
+    #[error("missing signers hash for transaction")]
+    MissingSignersHash,
+
+    /// Error returned when payload data cannot be parsed correctly.
+    #[error("failed to parse payload data, error: {0}")]
+    FailedToParsePayloadData(String),
+
+    /// Error returned when the payload data is missing.
+    #[error("missing payload data for transaction")]
+    MissingPayloadData,
+
+    /// Error returned when the epoch number in a transaction is invalid or
+    /// unexpected.
+    #[error("incorrect epoch for transaction")]
+    IncorrectEpoch,
+
+    /// Error returned when the first key doesn't correspod to the
+    /// SignersRotated event.
+    #[error("not a SignersRotated event")]
+    InvalidEvent,
+
+    /// Error returned when the threshold in a transaction is invalid or
+    /// unexpected.
+    #[error("incorrect threshold for transaction")]
+    IncorrectThreshold,
+
+    /// Error returned when the nonce in a transaction is missing.
+    #[error("missing nonce for transaction")]
+    MissingNonce,
+
+    /// Error returned when the keys in a transaction are missing.
+    #[error("missing keys for transaction")]
+    MissingKeys,
+}


the empty lines between arent needed

Suggested change

/// Error returned when a required signers hash is missing from a

/// transaction.

#[error("missing signers hash for transaction")]

MissingSignersHash,

/// Error returned when payload data cannot be parsed correctly.

#[error("failed to parse payload data, error: {0}")]

FailedToParsePayloadData(String),

/// Error returned when the payload data is missing.

#[error("missing payload data for transaction")]

MissingPayloadData,

/// Error returned when the epoch number in a transaction is invalid or

/// unexpected.

#[error("incorrect epoch for transaction")]

IncorrectEpoch,

/// Error returned when the first key doesn't correspod to the

/// SignersRotated event.

#[error("not a SignersRotated event")]

InvalidEvent,

/// Error returned when the threshold in a transaction is invalid or

/// unexpected.

#[error("incorrect threshold for transaction")]

IncorrectThreshold,

/// Error returned when the nonce in a transaction is missing.

#[error("missing nonce for transaction")]

MissingNonce,

/// Error returned when the keys in a transaction are missing.

#[error("missing keys for transaction")]

MissingKeys,

}

/// Error returned when a required signers hash is missing from a

/// transaction.

#[error("missing signers hash for transaction")]

MissingSignersHash,

/// Error returned when payload data cannot be parsed correctly.

#[error("failed to parse payload data, error: {0}")]

FailedToParsePayloadData(String),

/// Error returned when the payload data is missing.

#[error("missing payload data for transaction")]

MissingPayloadData,

/// Error returned when the epoch number in a transaction is invalid or

/// unexpected.

#[error("incorrect epoch for transaction")]

IncorrectEpoch,

/// Error returned when the first key doesn't correspod to the

/// SignersRotated event.

#[error("not a SignersRotated event")]

InvalidEvent,

/// Error returned when the threshold in a transaction is invalid or

/// unexpected.

#[error("incorrect threshold for transaction")]

IncorrectThreshold,

/// Error returned when the nonce in a transaction is missing.

#[error("missing nonce for transaction")]

MissingNonce,

/// Error returned when the keys in a transaction are missing.

#[error("missing keys for transaction")]

MissingKeys,

}

puhtaytow · 2025-01-10T10:08:32Z

packages/starknet-types/src/events/signers_rotated.rs

+
+#[cfg(test)]
+mod tests {
+    // use futures::stream::{FuturesUnordered, StreamExt};


could be removed

Suggested change

// use futures::stream::{FuturesUnordered, StreamExt};

puhtaytow · 2025-01-10T10:16:42Z

packages/starknet-types/src/events/contract_call.rs

+            .map_err(|err| ContractCallError::TryFromConversion(err.to_string()))?
+            .into();
+
+        // It's + 3, because we need to offset the 0th element, pending_word and


Suggested change

// It's + 3, because we need to offset the 0th element, pending_word and

// it's + 3, because we need to offset the 0th element, pending_word and

puhtaytow · 2025-01-10T10:17:26Z

packages/starknet-types/src/types/array_span.rs

+    type Error = ArraySpanError;
+
+    fn try_from(data: Vec<Felt>) -> Result<Self, Self::Error> {
+        // First element is always the array length, which is a felt (so u8 is enough)


Suggested change

// First element is always the array length, which is a felt (so u8 is enough)

// first element is always the array length, which is a felt (so u8 is enough)

puhtaytow · 2025-01-10T10:18:17Z

packages/starknet-types/src/types/starknet_message.rs

+
+        let result = StarknetMessage::from_token(starknet_msg_token);
+
+        // Tested like this, because InvalidOutputType doesn't implement PartialEq


Suggested change

// Tested like this, because InvalidOutputType doesn't implement PartialEq

// tested like this, because InvalidOutputType doesn't implement PartialEq

puhtaytow · 2025-01-10T10:18:26Z

packages/starknet-types/src/types/starknet_message.rs

+
+        let result = StarknetMessage::from_token(starknet_msg_token);
+
+        // Tested like this, because InvalidOutputType doesn't implement PartialEq


Suggested change

// Tested like this, because InvalidOutputType doesn't implement PartialEq

// tested like this, because InvalidOutputType doesn't implement PartialEq

puhtaytow · 2025-01-10T10:18:39Z

packages/starknet-types/src/types/starknet_message.rs

+
+        let result = StarknetMessage::from_token(starknet_msg_token);
+
+        // Tested like this, because InvalidOutputType doesn't implement PartialEq


Suggested change

// Tested like this, because InvalidOutputType doesn't implement PartialEq

// tested like this, because InvalidOutputType doesn't implement PartialEq

puhtaytow

LGTM2 left suggestions

puhtaytow · 2025-01-10T10:22:14Z

packages/starknet-types/src/events/signers_rotated.rs

+    /// Error returned when a required signers hash is missing from a
+    /// transaction.
+    #[error("missing signers hash for transaction")]
+    MissingSignersHash,
+
+    /// Error returned when payload data cannot be parsed correctly.
+    #[error("failed to parse payload data, error: {0}")]
+    FailedToParsePayloadData(String),
+
+    /// Error returned when the payload data is missing.
+    #[error("missing payload data for transaction")]
+    MissingPayloadData,
+
+    /// Error returned when the epoch number in a transaction is invalid or
+    /// unexpected.
+    #[error("incorrect epoch for transaction")]
+    IncorrectEpoch,
+
+    /// Error returned when the first key doesn't correspod to the
+    /// SignersRotated event.
+    #[error("not a SignersRotated event")]
+    InvalidEvent,
+
+    /// Error returned when the threshold in a transaction is invalid or
+    /// unexpected.
+    #[error("incorrect threshold for transaction")]
+    IncorrectThreshold,
+
+    /// Error returned when the nonce in a transaction is missing.
+    #[error("missing nonce for transaction")]
+    MissingNonce,
+
+    /// Error returned when the keys in a transaction are missing.
+    #[error("missing keys for transaction")]
+    MissingKeys,
+}


Suggested change

/// Error returned when a required signers hash is missing from a

/// transaction.

#[error("missing signers hash for transaction")]

MissingSignersHash,

/// Error returned when payload data cannot be parsed correctly.

#[error("failed to parse payload data, error: {0}")]

FailedToParsePayloadData(String),

/// Error returned when the payload data is missing.

#[error("missing payload data for transaction")]

MissingPayloadData,

/// Error returned when the epoch number in a transaction is invalid or

/// unexpected.

#[error("incorrect epoch for transaction")]

IncorrectEpoch,

/// Error returned when the first key doesn't correspod to the

/// SignersRotated event.

#[error("not a SignersRotated event")]

InvalidEvent,

/// Error returned when the threshold in a transaction is invalid or

/// unexpected.

#[error("incorrect threshold for transaction")]

IncorrectThreshold,

/// Error returned when the nonce in a transaction is missing.

#[error("missing nonce for transaction")]

MissingNonce,

/// Error returned when the keys in a transaction are missing.

#[error("missing keys for transaction")]

MissingKeys,

}

/// Error returned when a required signers hash is missing from a

/// transaction.

#[error("missing signers hash for transaction")]

MissingSignersHash,

/// Error returned when payload data cannot be parsed correctly.

#[error("failed to parse payload data, error: {0}")]

FailedToParsePayloadData(String),

/// Error returned when the payload data is missing.

#[error("missing payload data for transaction")]

MissingPayloadData,

/// Error returned when the epoch number in a transaction is invalid or

/// unexpected.

#[error("incorrect epoch for transaction")]

IncorrectEpoch,

/// Error returned when the first key doesn't correspod to the

/// SignersRotated event.

#[error("not a SignersRotated event")]

InvalidEvent,

/// Error returned when the threshold in a transaction is invalid or

/// unexpected.

#[error("incorrect threshold for transaction")]

IncorrectThreshold,

/// Error returned when the nonce in a transaction is missing.

#[error("missing nonce for transaction")]

MissingNonce,

/// Error returned when the keys in a transaction are missing.

#[error("missing keys for transaction")]

MissingKeys,

}

puhtaytow · 2025-01-10T10:22:38Z

packages/starknet-types/src/events/signers_rotated.rs

+
+    #[tokio::test]
+    async fn test_try_from_event_randomly_malformed_data_x1000() {
+        // let mut futures = FuturesUnordered::new();


Suggested change

// let mut futures = FuturesUnordered::new();

puhtaytow · 2025-01-10T10:22:49Z

packages/starknet-types/src/events/signers_rotated.rs

+                    keys: event.keys,
+                });
+                assert!(result.is_err());
+            // });


Suggested change

// });

eloylp · 2025-01-10T11:36:21Z

packages/starknet-types/src/events/contract_call.rs

+        //
+        // This field, should not exceed 252 bits (a felt's length)
+        let destination_chain = parse_cairo_short_string(&event.keys[1])?;
+


What about using &event.keys.get(1).expect("I expected this key") ?

ctoyan added 7 commits December 17, 2024 17:52

feat(ampd): starknet gmp implementation

559a9ea

fixes and refactors

25f5ed1

remove event type check when parsing events

05791d1

add CheckedFelt type

ec362e7

Merge branch 'main' into feat/starknet-gmp

e525c29

Merge branch 'main' into feat/starknet-gmp

e66a4d4

fixes after merge

35712fe

roberts-pumpurs reviewed Jan 10, 2025

View reviewed changes

puhtaytow approved these changes Jan 10, 2025

View reviewed changes

puhtaytow reviewed Jan 10, 2025

View reviewed changes

eloylp reviewed Jan 10, 2025

View reviewed changes

ctoyan added 9 commits January 10, 2025 14:59

README changes for starknet

451f625

remove crypto bigint

28d0add

add verifier set missing boilerplate

686ec98

add to config_template.toml

c7515a7

remove dead code allowance; remove futures dependency

55b49c0

add verifier set tests; various fixes and refactors

2807dd0

implement multicall, handling multiple msgs in one tx

2fb6ce0

small refactors

c43524e

Merge branch 'main' into feat/starknet-gmp

33ef7b8

puhtaytow approved these changes Jan 14, 2025

View reviewed changes

ctoyan added 3 commits January 17, 2025 14:55

remove unwrap

c811313

fix the event index iteration

8c14dc2

rename starknet-types and move part of the modules to ampd

f46f33a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: starknet GMP (INTERNAL REVIEW) #57

feat: starknet GMP (INTERNAL REVIEW) #57

ctoyan commented Jan 10, 2025

roberts-pumpurs Jan 10, 2025

puhtaytow Jan 10, 2025

roberts-pumpurs Jan 10, 2025

puhtaytow Jan 10, 2025

ctoyan Jan 10, 2025

roberts-pumpurs Jan 10, 2025

roberts-pumpurs Jan 10, 2025

puhtaytow left a comment •

edited

Loading

puhtaytow Jan 10, 2025

puhtaytow Jan 10, 2025

puhtaytow Jan 10, 2025

puhtaytow Jan 10, 2025

puhtaytow Jan 10, 2025

puhtaytow Jan 10, 2025

puhtaytow Jan 10, 2025

puhtaytow Jan 10, 2025

puhtaytow Jan 10, 2025

puhtaytow Jan 10, 2025

puhtaytow Jan 10, 2025

puhtaytow Jan 10, 2025

puhtaytow left a comment

puhtaytow Jan 10, 2025

puhtaytow Jan 10, 2025

puhtaytow Jan 10, 2025

eloylp Jan 10, 2025

	// It's + 3, because we need to offset the 0th element, pending_word and
	// it's + 3, because we need to offset the 0th element, pending_word and

	// First element is always the array length, which is a felt (so u8 is enough)
	// first element is always the array length, which is a felt (so u8 is enough)


		let result = StarknetMessage::from_token(starknet_msg_token);

		// Tested like this, because InvalidOutputType doesn't implement PartialEq

feat: starknet GMP (INTERNAL REVIEW) #57

Are you sure you want to change the base?

feat: starknet GMP (INTERNAL REVIEW) #57

Conversation

ctoyan commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puhtaytow left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puhtaytow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puhtaytow left a comment •

edited

Loading