feat: add support for solana message id's #32

eloylp · 2024-07-08T13:42:22Z

Description

At Eiger we are developing the Solana integration. During tests, we realised the current Base58TxDigestAndEventIndex does not support the specific Solana transaction ids (a.k.a as Signatures), which have a length of 88 characters on it's base58 encoded form.

This PR aims to add support for the Solana message id type's by adding a new enum variant on the existing codebase.

Todos

Unit tests
Manual tests
Documentation
Connect epics/issues

Steps to Test

Expected Behaviour

Other Notes

We are unsure about the non direct implications of this change nor it's impact into Axelar's future plans. We appreciate your feedback and suggestions.

* feat(minor-ampd): register public key takes in key type * tofnd client parse ed25519 * cargo sort

…rst tick (axelarnetwork#480) * fix: directly pass interval to queued broadcaster and pass the first unwanted tick * refactor: address PR comments * refactor: simplify steps to take after interval tick in run function

…work#478) * feat: check caller authorization status for multisig * refactor: rename query function name to caller_authorized

…larnetwork#484)

…twork#476) * feat(minor-voting-verifier): consistent gas cost for voting

…#479)

eloylp · 2024-07-08T13:54:18Z

contracts/voting-verifier/src/contract.rs

+                Base58SolanaTxDigestAndEventIndex::new_from_b58_encoded_signature_and_index(
+                    id, index,
+                )
+                .unwrap()


I don't like to unwrap, but functions signature does not allow returning errors.

same, but looks like the other implementations also use unwraps freely here.

eloylp · 2024-07-08T13:58:19Z

contracts/voting-verifier/src/events.rs

@@ -137,6 +138,12 @@ fn parse_message_id(

            Ok((id.tx_hash_as_hex(), id.event_index))
        }
+        MessageIdFormat::Base58SolanaTxDigestAndEventIndex => {
+            let id = Base58SolanaTxDigestAndEventIndex::from_str(&message_id)
+                .map_err(|_| ContractError::InvalidMessageID(message_id.into()))?;


I wonder about providing more error info. But maybe it doesn't worth it, as we wouldn't get much more information anyways.

I agree, would be nice, but lets keep it as is, because it fits with the rest of the code.

eloylp · 2024-07-08T13:59:56Z

packages/axelar-wasm-std/src/msg_id/base_58_solana_event_index.rs

+        }
+    }
+
+    pub fn new_from_b58_encoded_signature_and_index(


Added this extra constructor for convenience, and for not spreading the decoding code everywhere.

eloylp · 2024-07-08T14:00:21Z

packages/axelar-wasm-std/src/msg_id/base_58_solana_event_index.rs

+    }
+}
+
+fn decode_b58_signature(signature: &str) -> Result<RawSignature, Report<Error>> {


Added this helper func for not repeating code.

eloylp · 2024-07-08T14:02:03Z

packages/axelar-wasm-std/src/msg_id/base_58_solana_event_index.rs

+
+    use super::*;
+
+    fn random_bytes() -> [u8; 64] {


Increased this from [u8;32]

You can use the type alias that you defined at the top of the file RawSignature

fixed at 3ce8e17

eloylp · 2024-07-08T14:04:50Z

packages/axelar-wasm-std/src/msg_id/base_58_solana_event_index.rs

+    #[test]
+    fn should_parse_msg_id() {
+        let res = Base58SolanaTxDigestAndEventIndex::from_str(
+            "4hHzKKdpXH2QMB5Jm11YR48cLqUJb9Cwq2YL3tveVTPeFkZaLP8cdcH5UphVPJ7kYwCUCRLnywd3xkUhb4ZYWtf5-0",


Adapted to a solana signature here

eloylp · 2024-07-08T14:05:30Z

packages/axelar-wasm-std/src/msg_id/base_58_solana_event_index.rs

+        let event_index = random_event_index();
+
+        // too long
+        let msg_id = format!("{}{}-{}", tx_digest, tx_digest, event_index);


Basically the test doubles the length here.

eloylp · 2024-07-08T14:05:42Z

packages/axelar-wasm-std/src/msg_id/base_58_solana_event_index.rs

+        assert!(res.is_err());
+
+        // too short
+        let msg_id = format!("{}-{}", &tx_digest[0..30], event_index);


Here we keep the same lower bound

eloylp · 2024-07-08T14:10:47Z

packages/axelar-wasm-std/src/msg_id/base_58_solana_event_index.rs

+            ));
+            assert!(res.is_err());
+        }
+    }


I think here, as in other places, another option would be to test the REGEX directly with something like rstest.

Generally speaking, I would opt for using proptest and do some property-based testing that generates the values for us. But let's not overcomplicate this PR. I'm just mentioning this as something cool that can be expanded upon.

eloylp · 2024-07-08T14:13:55Z

packages/axelar-wasm-std/src/msg_id/base_58_solana_event_index.rs

+            let decoded = bs58::decode(&b58[trim..]).into_vec().unwrap();
+            assert_ne!(bytes.to_vec(), decoded);
+        }
+    }


I think this kind of tests are more "exploratory" ones of the b58 library. Tests that provide insights about a library expected behaviour. If i am right, i would not put them here, but in a dedicated integration test suite, so not related with any specific part of the code.

roberts-pumpurs

overall lgtm, left some adjustments

roberts-pumpurs · 2024-07-09T07:03:51Z

contracts/voting-verifier/src/contract.rs

+                Base58SolanaTxDigestAndEventIndex::new_from_b58_encoded_signature_and_index(
+                    id, index,
+                )
+                .unwrap()


same, but looks like the other implementations also use unwraps freely here.

roberts-pumpurs · 2024-07-09T07:04:42Z

contracts/voting-verifier/src/events.rs

@@ -137,6 +138,12 @@ fn parse_message_id(

            Ok((id.tx_hash_as_hex(), id.event_index))
        }
+        MessageIdFormat::Base58SolanaTxDigestAndEventIndex => {
+            let id = Base58SolanaTxDigestAndEventIndex::from_str(&message_id)
+                .map_err(|_| ContractError::InvalidMessageID(message_id.into()))?;


I agree, would be nice, but lets keep it as is, because it fits with the rest of the code.

roberts-pumpurs · 2024-07-09T07:05:36Z

packages/axelar-wasm-std/src/hash.rs

@@ -1 +1 @@
-pub type Hash = [u8; 32];
+pub type Hash = [u8; 32];


looks like the newline character got deleted on this file, and this diff is unnecessary. You can probably git reset this file

fixed at 5e6e475

roberts-pumpurs · 2024-07-09T07:16:45Z

packages/axelar-wasm-std/src/msg_id/base_58_solana_event_index.rs

+        .map_err(|e: TryFromSliceError| {
+            Error::InvalidTxDigest(format!(
+                "Signature - {}: {}",
+                signature,
+                e
+            ))


Let's change the error message so it would be used the same way as in the other msg_id files (only populate it with the message id - signature in our case), without the extra format! call:

.map_err(|e: TryFromSliceError| { Error::InvalidTxDigest( signature.to_owned() ))

Fixed at c370f4d

roberts-pumpurs · 2024-07-09T07:56:59Z

packages/axelar-wasm-std/src/msg_id/base_58_solana_event_index.rs

+
+    #[test]
+    fn should_not_parse_msg_id_with_correct_length_base58_but_wrong_length_hex() {
+        // this is 88 chars and valid base58, but will encode to 33 bytes


This commend is invalid, right?

but will encode to 33 bytes

Nice catch ! fixed at c8d6344

Also updated this lower bound check to 64 99bd04d

roberts-pumpurs · 2024-07-09T07:58:12Z

packages/axelar-wasm-std/src/msg_id/base_58_solana_event_index.rs

+        })?)
+}
+
+const PATTERN: &str = "^([1-9A-HJ-NP-Za-km-z]{32,88})-(0|[1-9][0-9]*)$";


Based on my reading, the Solana signatures are 86-88 chars long.
But a zeroized byte array [0, 0, ..n] (n=64) will result in base58 string that's 64 chars long:

import base58 # Create 64 bytes all containing 0 data = bytes([0] * 64) # Encode the data using Base58 encoded = base58.b58encode(data) # Print the encoded string print(f"Base58 Encoded String: {encoded}") print(f"Length of Encoded String: {len(encoded)}") # Base58 Encoded String: b'1111111111111111111111111111111111111111111111111111111111111111' # Length of Encoded String: 64

Im not sure if there are other edge cases like this 🤔 , but I think its safe to adjust the regex pattern to ..{64,88}..

Based on the signature's code , i think it's suggesting at least we are going to fill the whole byte slice [u8; 64] , thus the minimum of 64 you are commmenting makes sense and seems safe to adopt.

fixed at 002caa6

roberts-pumpurs · 2024-07-09T07:59:18Z

packages/axelar-wasm-std/src/msg_id/base_58_solana_event_index.rs

+
+    use super::*;
+
+    fn random_bytes() -> [u8; 64] {


You can use the type alias that you defined at the top of the file RawSignature

roberts-pumpurs · 2024-07-09T08:01:18Z

packages/axelar-wasm-std/src/msg_id/base_58_solana_event_index.rs

+        let res = Base58SolanaTxDigestAndEventIndex::from_str(
+            "4hHzKKdpXH2QMB5Jm11YR48cLqUJb9Cwq2YL3tveVTPeFkZaLP8cdcH5UphVPJ7kYwCUCRLnywd3xkUhb4ZYWtf5-0",
+        );
+        res.unwrap();


it would be more idiomatic to assert the Result type rather than unwrap and not use the result:
assert!(res.is_ok());

Fixed at edb54bd . I changed it previously to debug a test. IMO, this is a bad sympton of fragile tests. As the negative can come from different underlying reasons. but let's keep aligned with current test structure.

roberts-pumpurs · 2024-07-09T08:03:13Z

packages/axelar-wasm-std/src/msg_id/base_58_solana_event_index.rs

+        // this is 44 chars and valid base 58, but will encode to 33 bytes
+        // (z is the largest base58 digit, and so this will overflow 2^256)
+        let tx_digest = "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz";
+        assert_eq!(tx_digest.len(), 44);


This should be adjusted to test the upper 88 char limit rather than 44

Also fixed at c8d6344

roberts-pumpurs · 2024-07-09T08:06:08Z

packages/axelar-wasm-std/src/msg_id/base_58_solana_event_index.rs

+            ));
+            assert!(res.is_err());
+        }
+    }


Generally speaking, I would opt for using proptest and do some property-based testing that generates the values for us. But let's not overcomplicate this PR. I'm just mentioning this as something cool that can be expanded upon.

eloylp · 2024-07-09T09:59:13Z

@roberts-pumpurs thanks for your review, addressed all suggestions. I will start crafting a new PR based on this outcome for upstream.

eloylp · 2024-07-09T14:22:21Z

Upstream PR is ready for review: axelarnetwork#494

cc @roberts-pumpurs

roberts-pumpurs · 2024-07-15T08:32:05Z

Closing this PR because it has been merged in upstream

cgorenflo and others added 11 commits June 26, 2024 21:30

docs: update compatibility matrix (axelarnetwork#475)

66a7d91

feat(minor-ampd): support ed25519 keygen and sign (axelarnetwork#465)

92a7dc6

* feat(minor-ampd): register public key takes in key type * tofnd client parse ed25519 * cargo sort

fix(ampd): enable derive for der crate (axelarnetwork#477)

1cbbf8d

refactor: generalize msg access control (axelarnetwork#473)

65c23ee

feat(minor-router): add command to freeze all chains (axelarnetwork#472)

86b0c16

feat(multisig): query to check caller authorization status (axelarnet…

c2a4122

…work#478) * feat: check caller authorization status for multisig * refactor: rename query function name to caller_authorized

fix(ampd): increase number of retries when fetching block events (axe…

a64c6e0

…larnetwork#484)

feat(minor-voting-verifier): consistent gas cost for voting (axelarne…

60cee5d

…twork#476) * feat(minor-voting-verifier): consistent gas cost for voting

feat: improve msg permission control with derive macro (axelarnetwork…

b37114e

…#479)

feat: add support for solana message id's

addfec5

eloylp self-assigned this Jul 8, 2024

eloylp commented Jul 8, 2024

View reviewed changes

eloylp requested a review from roberts-pumpurs July 8, 2024 14:36

roberts-pumpurs reviewed Jul 9, 2024

View reviewed changes

eloylp added 4 commits July 9, 2024 10:27

Preserve new line char

5e6e475

Align error handling to repo standar

c370f4d

Remove non necessary argument

e43bbb3

Fix test lengths (comments/code)

c8d6344

eloylp force-pushed the add-solana-msg-id-support branch from 02fb8f9 to c8d6344 Compare July 9, 2024 09:17

Adjust regex to min 64 chars

002caa6

eloylp added 3 commits July 9, 2024 11:36

Use type alias

3ce8e17

Adjust lower bound checking in test

99bd04d

return back assert

edb54bd

eloylp requested a review from roberts-pumpurs July 9, 2024 09:58

roberts-pumpurs force-pushed the main branch from b37114e to 3a184af Compare July 15, 2024 08:16

roberts-pumpurs closed this Jul 15, 2024

eloylp deleted the add-solana-msg-id-support branch September 4, 2024 09:27

		@@ -1 +1 @@
		pub type Hash = [u8; 32];
		pub type Hash = [u8; 32];

feat: add support for solana message id's #32

feat: add support for solana message id's #32

Conversation

eloylp commented Jul 8, 2024 • edited Loading

Description

Todos

Steps to Test

Expected Behaviour

Other Notes

eloylp Jul 8, 2024 • 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

eloylp Jul 8, 2024 • 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

roberts-pumpurs 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eloylp Jul 9, 2024 • 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

eloylp commented Jul 9, 2024 • edited Loading

eloylp commented Jul 9, 2024

roberts-pumpurs commented Jul 15, 2024

eloylp commented Jul 8, 2024 •

edited

Loading

eloylp Jul 8, 2024 •

edited

Loading

eloylp Jul 8, 2024 •

edited

Loading

eloylp Jul 9, 2024 •

edited

Loading

eloylp commented Jul 9, 2024 •

edited

Loading