Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 17 commits into from
Jul 2, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 103 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ serde_json = "1.0.89"
schemars = "0.8.10"
sha3 = { version = "0.10.8", default-features = false, features = [] }
signature-verifier-api = { version = "^0.1.0", path = "packages/signature-verifier-api" }
axelar-encoding = { git = "ssh://[email protected]/eigerco/solana-axelar-internal.git", branch = "main"}
eloylp marked this conversation as resolved.
Show resolved Hide resolved

[workspace.lints.clippy]
arithmetic_side_effects = "deny"
Expand Down
1 change: 1 addition & 0 deletions contracts/multisig-prover/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ service-registry = { workspace = true }
sha3 = { workspace = true }
thiserror = { workspace = true }
voting-verifier = { workspace = true, features = ["library"] }
axelar-encoding = { workspace = true }

[dev-dependencies]
anyhow = "1.0"
Expand Down
2 changes: 2 additions & 0 deletions contracts/multisig-prover/src/encoding/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
pub mod abi;
pub mod sol;

use cosmwasm_schema::cw_serde;

Expand All @@ -7,4 +8,5 @@ use cosmwasm_schema::cw_serde;
pub enum Encoder {
Abi,
Bcs,
Solana
eloylp marked this conversation as resolved.
Show resolved Hide resolved
}
136 changes: 136 additions & 0 deletions contracts/multisig-prover/src/encoding/sol/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
use std::{array::TryFromSliceError, collections::BTreeMap};

use axelar_encoding::types::{
CrossChainId, Message, Payload, PublicKey, Signature, Signer, WeightedSignature, WorkerSet,
U256,
};

use itertools::Itertools;
use multisig::{msg::SignerWithSig, verifier_set::VerifierSet};

use crate::error::ContractError;

const ECDSA_COMPRESSED_PUBKEY_LEN: usize = 33; // this should be probably a public constant in axelar_encoding.
const ED25519_PUBKEY_LEN: usize = 32; // this should be probably a public constant in axelar_encoding.

eloylp marked this conversation as resolved.
Show resolved Hide resolved
type Result<T> = core::result::Result<T, ContractError>;

pub fn to_worker_set(vs: &VerifierSet) -> Result<WorkerSet> {
let mut signers: BTreeMap<String, Signer> = BTreeMap::new();

vs.signers
.iter()
.try_for_each(|(address, signer)| -> Result<()> {
let enc_signer = signer.address.to_string();
let enc_pubkey = to_pub_key(&signer.pub_key)?;

let enc_weight = U256::from_be(to_u256_be(signer.weight.u128()));

signers.insert(
address.clone(),
Signer::new(enc_signer, enc_pubkey, enc_weight),
);
Ok(())
})?;

Ok(WorkerSet::new(
vs.created_at,
signers,
U256::from_be(to_u256_be(vs.threshold.u128())),
))
}

fn to_pub_key(pk: &multisig::key::PublicKey) -> Result<PublicKey> {
Ok(match pk {
multisig::key::PublicKey::Ecdsa(hb) => {
PublicKey::new_ecdsa(hb.to_array::<ECDSA_COMPRESSED_PUBKEY_LEN>()?)
eloylp marked this conversation as resolved.
Show resolved Hide resolved
}
multisig::key::PublicKey::Ed25519(hb) => {
PublicKey::new_ed25519(hb.to_array::<ED25519_PUBKEY_LEN>()?)
}
})
}

// Fits a u128 into a u256 in big endian representation.
fn to_u256_be(u: u128) -> [u8; 32] {
let mut uin256 = [0u8; 32];
uin256[16..32].copy_from_slice(&u.to_be_bytes());
uin256
}
eloylp marked this conversation as resolved.
Show resolved Hide resolved

impl TryFrom<&crate::payload::Payload> for Payload {
type Error = ContractError;
fn try_from(value: &crate::payload::Payload) -> std::result::Result<Self, Self::Error> {
Ok(match value {
crate::payload::Payload::Messages(msgs) => {
Payload::new_messages(msgs.iter().map(to_msg).collect_vec())
}
crate::payload::Payload::VerifierSet(vs) => {
Payload::new_worker_set(to_worker_set(&vs)?)
}
})
}
}

fn to_msg(msg: &router_api::Message) -> Message {
let enc_cc_id = CrossChainId::new(msg.cc_id.chain.to_string(), msg.cc_id.id.to_string());

Message::new(
enc_cc_id,
msg.source_address.to_string(),
msg.destination_chain.to_string(),
msg.destination_address.to_string(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@roberts-pumpurs roberts-pumpurs Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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).
  2. 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.

Copy link

@tilacog tilacog Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@eloylp eloylp Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@tilacog tilacog Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

msg.payload_hash,
)
}

pub fn to_weighted_signature(sig: &SignerWithSig) -> Result<WeightedSignature> {
let enc_pub_key = to_pub_key(&sig.signer.pub_key)?;
let enc_signature = to_signature(&sig.signature)?;
let enc_weight = U256::from_be(to_u256_be(sig.signer.weight.u128()));

Ok(WeightedSignature::new(
enc_pub_key,
enc_signature,
enc_weight,
))
}

fn to_signature(sig: &multisig::key::Signature) -> Result<Signature> {
match sig {
multisig::key::Signature::Ecdsa(_) => unimplemented!(), // Todo: should we implement this in axelar_encoding ?
eloylp marked this conversation as resolved.
Show resolved Hide resolved

// 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) => {
let data = r
.as_ref()
.try_into()
.map_err(|e: TryFromSliceError| ContractError::SolEncodingError(e.to_string()))?;
Ok(Signature::EcdsaRecoverable(data))
}
multisig::key::Signature::Ed25519(ed) => {
let data = ed
.as_ref()
.try_into()
.map_err(|e: TryFromSliceError| ContractError::SolEncodingError(e.to_string()))?;

Ok(Signature::new_ed25519(data))
}
eloylp marked this conversation as resolved.
Show resolved Hide resolved
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn conversion_to_u256_be_works() {
let integer = to_u256_be(u128::MAX);
let expected = [
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 255, 255, 255, 255, 255, 255, 255,
255, 255, 255, 255, 255, 255, 255, 255,
];
assert_eq!(expected, integer);
}
}
5 changes: 5 additions & 0 deletions contracts/multisig-prover/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ pub enum ContractError {
#[error(transparent)]
BcsError(#[from] bcs::Error),

/// Todo, Below error throws: binary operation `==` cannot be applied to type `EncodingError<1024>`
/// this is a workaround.
#[error("encoding/decoding failure: [0]")]
SolEncodingError(String),
eloylp marked this conversation as resolved.
Show resolved Hide resolved

#[error("verifier set has not changed sufficiently since last update")]
VerifierSetUnchanged,

Expand Down
29 changes: 28 additions & 1 deletion contracts/multisig-prover/src/payload.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use cosmwasm_schema::cw_serde;
use cosmwasm_std::{from_json, HexBinary, StdResult};
use cw_storage_plus::{Key, KeyDeserialize, PrimaryKey};
use itertools::Itertools;
use sha3::{Digest, Keccak256};

use axelar_wasm_std::hash::Hash;
Expand All @@ -9,7 +10,11 @@ use multisig::verifier_set::VerifierSet;
use router_api::{CrossChainId, Message};

use crate::{
encoding::{abi, Encoder},
encoding::{
abi,
sol::{to_weighted_signature, to_worker_set},
Encoder,
},
error::ContractError,
};

Expand Down Expand Up @@ -46,6 +51,11 @@ impl Payload {
match encoder {
Encoder::Abi => abi::payload_hash_to_sign(domain_separator, cur_verifier_set, self),
Encoder::Bcs => todo!(),
Encoder::Solana => Ok(axelar_encoding::hash_payload(
&domain_separator,
&to_worker_set(cur_verifier_set)?,
&axelar_encoding::types::Payload::try_from(self)?,
)),
}
}

Expand All @@ -70,6 +80,23 @@ impl Payload {
abi::execute_data::encode(verifier_set, signers_with_sigs, &payload_hash, payload)
}
Encoder::Bcs => todo!(),
Encoder::Solana => {
let mut enc_signatures = Vec::new();

for s in signers_with_sigs {
enc_signatures.push(to_weighted_signature(&s)?)
}

let bytes = axelar_encoding::encode::<1024>(
eloylp marked this conversation as resolved.
Show resolved Hide resolved
// Todo reason about this "1024" magic number.
&to_worker_set(&verifier_set)?,
enc_signatures,
axelar_encoding::types::Payload::try_from(payload)?,
)
.map_err(|e| ContractError::SolEncodingError(e.to_string()))?;

Ok(HexBinary::from(bytes)) // Todo, what kind of conversion if any comes from here. Can we expect hex repr directly ?
eloylp marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
}
Expand Down
Loading