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

[Feature] Implement dynamic EIP-712 typed data signing #202

Closed
DaniPopes opened this issue Feb 9, 2024 · 7 comments
Closed

[Feature] Implement dynamic EIP-712 typed data signing #202

DaniPopes opened this issue Feb 9, 2024 · 7 comments
Labels
enhancement New feature or request

Comments

@DaniPopes
Copy link
Member

DaniPopes commented Feb 9, 2024

Component

signers

Describe the feature you would like

Currently signing EIP-712 typed data can only be done via sign_typed_data, which uses a SolStruct generic type. This method cannot be used if the signer is not Sized, e.g. dyn Signer.

An additional method can be added to both Signers, possibly called sign_dynamic_typed_data, which accepts a alloy_dyn_abi::eip712::TypedData and by default delegates to sign_hash with the result of eip712_signing_hash.

This method should be feature-gated to "eip712", just like the existing one.

Additional context

No response

@DaniPopes DaniPopes added the enhancement New feature or request label Feb 9, 2024
@evchip
Copy link
Contributor

evchip commented Feb 22, 2024

Hi @DaniPopes, I'd like to take this on. Do you have any suggestions for unit test cases?

@DaniPopes
Copy link
Member Author

DaniPopes commented Feb 22, 2024

Sure, you can update the sol! typed data test to use TypedData::from_struct as well, and some made up domain

@evchip
Copy link
Contributor

evchip commented Feb 23, 2024

WIP branch. When I pass the FooBar struct to TypedData::from_struct and then call eip712_signing_hash() on it, it throws an error TypeMismatch { expected: "bytes", actual: "[102,105,122,122]" }.

It looks like there is a mismatch in how the data is being serialized for the bytes type. TypedData expects a byte array but it's being serialized as an array of numbers.

When I remove that bytes type from the FooBar struct via FooBarNoFizz, the tests pass. I'll need to dive into the serialization discrepancy to figure out what's going wrong. Any pointers would be appreciated though @DaniPopes 🙏.

@evchip
Copy link
Contributor

evchip commented Feb 23, 2024

foo_bar_dynamic.eip712_signing_hash() succeeds when I encode the resulting fizz field from TypedData::from_struct as a hex string and serialize it back to JSON. So it looks like the issue stems from differences in the ways in which SolStruct and TypedData encode the data. Specifically, I think the coerce() method on TypedData is the culprit. Will dive into how DynSolType.coerce_json() is handling byte arrays. If my suspicion is correct, the fix may result in a change to one of thse implementations.

use serde_json::{Value, json};

let foo_bar = FooBar {
    foo: I256::try_from(10u64).unwrap(),
    bar: U256::from(20u64),
    fizz: b"fizz".to_vec(),
    buzz: keccak256("buzz"),
    far: String::from("space"),
    out: Address::ZERO,
};

let mut foo_bar_dynamic = TypedData::from_struct(&foo_bar, Some(domain));

let fizz_array = [102, 105, 122, 122];
let fizz_hex = hex::encode(fizz_array);
let fizz_hex_prefixed = format!("0x{}", fizz_hex);

if let Value::Object(ref mut map) = foo_bar_dynamic.message {
    map.insert("fizz".to_string(), json!(fizz_hex_prefixed));
}
// succeeds because fizz is serialized as bytes instead of number array
let dynamic_hash = foo_bar_dynamic.eip712_signing_hash().unwrap();

@prestwich
Copy link
Member

you'll want to modify fn bytes() on L103 of crates/dyn-abi/src/eip712/coerce.rs to handle the "array of numbers" case with something like

fn bytes(value: &serde_json::Value) -> Option<Vec<u8>> {
    if let Some(s) = value.as_str() {
        return hex::decode(s).ok();
    }

    let arr = value.as_array()?;
    let mut vec = Vec::with_capacity(arr.len());
    for elem in arr.into_iter() {
        vec.push(elem.as_u64()?.try_into().ok()?);
    }
    Some(vec)
}

#[cfg(test)]
mod test {
    #[test]
    fn test_bytes_num_array() {
        let ty = DynSolType::Bytes;
        let j = json!([1, 2, 3, 4]);
        assert_eq!(ty.coerce_json(&j), Ok(DynSolValue::Bytes(vec![1, 2, 3, 4])));
    }
}

@evchip
Copy link
Contributor

evchip commented Feb 27, 2024

you'll want to modify fn bytes() on L103 of crates/dyn-abi/src/eip712/coerce.rs to handle the "array of numbers" case
@prestwich That works! Thanks so much for your help.

@onbjerg
Copy link
Member

onbjerg commented Mar 12, 2024

Closed in #235

@onbjerg onbjerg closed this as completed Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants