feat: add initial integration (POC) of the axelar-encoding crate #22

eloylp · 2024-06-10T08:30:54Z

Description

Closes https://github.com/eigerco/solana-axelar-internal/issues/317

This implements the axelar-encoding crate in the multisig-prover wasm contract. The current PR is just a POC to share with the team and solve all the possible doubts. Actually it compiles, but there could be some misinterpretations regarding data, specially when we deal with raw bytes.

Some comments were left in order to get early feedback and start discussion.

Note : CI is failing because we are trying to reach the axelar-encoding crate from a private repo. Whenever we are comfortable with the integration/crate public API, we should update the solana-axelar repo with the latest changes from solana-axelar-internal (which is the private fork we are working on because of privacy reasons) that includes the axelar encoding crate. Then change the cargo import to point to the public repo.

Todos

Unit tests
Documentation
Connect epics/issues

Steps to Test

$ cargo test

contracts/multisig-prover/src/encoding/mod.rs

contracts/multisig-prover/src/encoding/sol/mod.rs

eloylp · 2024-06-10T08:47:59Z

contracts/multisig-prover/src/encoding/sol/mod.rs

+        enc_cc_id,
+        msg.source_address.to_string(),
+        msg.destination_chain.to_string(),
+        msg.destination_address.to_string(),


I think we would need to deeper check if this strings representations are really the expected ones in the other end.

So all this fields, and it's related types, can be built again from a String representation. So obviously we are doing good here, as we are being agnostic. The decoding part would need know beforehand how to represent the decoded String on respective types. Where is that decoding part ? I am missing something.

The decoding part would need know beforehand how to represent the decoded String on respective types. Where is that decoding part ?

Maybe this is a fait point in that the axelar-rkyv-encoding crate should operate on the original router_api::Message types rather than Strings?

I am looking at how the flow goes:

The relayer gets the raw bytes from amplifier API and tries to decode it using a hardcoded decoder [link to code] - in the linked example we're using bcs decoding (will be replaced with rkyv soon).

The messages get bcs decoded (this is our code btw). Looking at the implementation, most of the parameters are just left as bytes and we don't really interact with them. The only parameter that gets interpreted in any way is the destination_address.

When I was working on the "e2e" tests between evm and solana, I was relying on the Pubkey::to_string() implementation when being invoked on the source chain (here's the decoding). iirc Pukey::to_string() is a base58 encoded string.

Where is the issue: We force the origin chain to produce strings of "destination_address" that must be properly parsed by the destination chain / destination relayer.

Solution: nothing we can really do here? Because theoretically its up to the caller on the source chain to properly encode destination_address so that we can parse it. Anyone can call a source gateway and pass destination addresses as hex encoded, binary encoded strings, base58 encoded strings, etc. And we cannot realistically handle this.

And if we want this encoding be generic, then we cannot force it to use base58 encoded strings for ed25519 addresses either. This means that the parsing of the string gets propagated to the relayer.

Unless we step back from this encoding being Solana-agnostic: this encoding is now Solana-specific and all incoming destination_address params must be (base58|hex|something else) encoded strings. Then we can add this enforcement in the encoding library and make this restriction explicit.

I think we would need to deeper check if this strings representations are really the expected ones in the other end.
[...] The decoding part would need know beforehand how to represent the decoded String on respective types. Where is that decoding part ? I am missing something.

What you are doing here is just transforming the outer types, but the actual work will be performed by rkyv, so we don't need to worry about any manual serialization or deserialization. It's just like serde, where the type system will do all the heavy lifting for us.

Maybe this is a fait point in that the axelar-rkyv-encoding crate should operate on the original router_api::Message types rather than Strings?

The original Axelar/CosmWasm types are all Strings in the end, so I'm not worried about data loss — as long as we don't forget any data point when transforming the types into the ones in axelar-rkyv-encoding.

This means that the parsing of the string gets propagated to the relayer.

True.

Unless we step back from this encoding being Solana-agnostic: this encoding is now Solana-specific and all incoming destination_address params must be (base58|hex|something else) encoded strings.

Well, since we are in control of the final types, we can force the destination_address to be [u8; 32], just like solana_program::pubkey::Pubkey. The fit will be even tighter.

Are the different parts of a same flow (chain scoped, so integration scoped) able to know (by agreement) how to properly handle the data at each stage right ? If yes, i would probably leave the encoding crate chain agnostic.

So far, the axelar-rkyv-encoding is independent of Solana, and I'll try my best to ensure it stays that way. Importing any Solana dependencies would mean bringing in the whole circus of Solana crates as Amplifier dependencies, and I want to avoid that.

Well, since we are in control of the final types, we can force the destination_address to be [u8; 32], just like solana_program::pubkey::Pubkey. The fit will be even tighter.

The benefits of this:

we can directly map the bytes into a public key (this only affects the relayer)

there's no evm or cosmos chain compatibility that we'd have to worry about (they use 20 bytes for addresses)

On the other hand the interface for the EVM Gateway expects that the address is already provided as a string rather than raw bytes.

iirc I tried doing exactly this (trying to send raw bytes, then converting them to a String inside Solidity, then sending it to the EVM gateway). But I had major issues with paring the events - the data didn't make sense. I think this is because ethers library generates the interfaces and types for events, and it expects the string parameter to be a valid UTF-8 string - but Solidity does not enforce strings to be utf8 hence the issue in parsing.

I think that we should rather leave the parameter as a String and keep the parsing logic on the relayer - with the assumption that the origin chain always sends base58 encoded to parse string as base58 for Solana. I think its easy to reason with this assumptoin. Even now, every gmp packet is expected to uphold some arbitrary encoding enforced by the destination relayer.

contracts/multisig-prover/src/encoding/sol/mod.rs

contracts/multisig-prover/src/error.rs

contracts/multisig-prover/src/payload.rs

Cargo.toml

eloylp · 2024-06-10T10:06:32Z

contracts/multisig-prover/src/encoding/sol/mod.rs

+        // Following 2: We are just moving the bytes around, hoping this conversions match. Not sure if `HexBinary`
+        // representation will match here with the decoding part.
+        multisig::key::Signature::EcdsaRecoverable(r) => {
+            Signature::EcdsaRecoverable(r.as_ref().try_into().unwrap())


I need to get rid of unwraps on this code

this was fixed at 12c37e1 , although i still think the error handling should ideally provide more details. Propagating errors up in the call stack without references can be confusing when their are shown on an log line.

roberts-pumpurs

Great work, I added a few comments, mostly questions rather than suggestions.

Could you also add some encoding unittests to ensure that everything works?

contracts/multisig-prover/src/encoding/mod.rs

contracts/multisig-prover/src/encoding/sol/mod.rs

contracts/multisig-prover/src/payload.rs

Cargo.toml

contracts/multisig-prover/src/encoding/sol/mod.rs

contracts/multisig-prover/src/payload.rs

eloylp · 2024-06-11T11:55:39Z

Great work, I added a few comments, mostly questions rather than suggestions.

Could you also add some encoding unittests to ensure that everything works?

Thank you @roberts-pumpurs ! Of course, let's add that unit tests 👍

- Use the new crate renaming `axlelar-rkyv-encoding` - Import the create directly from the development branch - Use now public constants - Change to little endian everywhere

…-encode

eloylp · 2024-07-02T15:16:57Z

contracts/multisig-prover/src/encoding/rkyv/mod.rs

+            let enc_weight =
+                axelar_rkyv_encoding::types::U256::from_le(to_u256_le(signer.weight.u128()));
+
+            signers.insert(enc_pubkey, enc_weight);


@roberts-pumpurs , @tilacog , please let's ensure this is correct. I am directly using signer.pub_key here, and avoiding a conversion from a String (the keys of the map), assuming they are the same.