xJonathanLEI / starknet-rs

Complete Starknet library in Rust™
https://starknet.rs
Apache License 2.0
277 stars 96 forks source link

Implementation of `compute_hash_on_elements` for stark orders #127

Closed rhobro closed 2 years ago

rhobro commented 2 years ago

Hi there,

I am trying out using starknet as part of signing transactions for the dYdX exchange. As part of the hashing, I have a series of pedersen hashes like the following:

pedersen(
    &pedersen(
        &pedersen(
            &pedersen(
                &asset_id_sell.to_fe(),
                &asset_id_buy.to_fe(),
            ),
            &self.message.asset_id_fee.to_fe(),
        ),
        &part_1,
    ),
    &part_2,
)

However, I found starknet::core::crypto::compute_hash_on_elements which seemed to do the same job? When I tried it, it didn't seem to produce valid signatures for the orders on the exchange. Upon reading the implementation, it seems to do the same job all the way until the end when it seems to further hash it again with the data length:

let data_len = FieldElement::from(data.len());
pedersen_hash(&current_hash, &data_len)

dYdX uses stark exchange and it seems to work without the final hashing with data length but not with. Does this mean that the implementation of compute_hash_on_elements is wrong or that the implementation of starkex is wrong on dYdX's side? I'd like to put all of this into my future PR, so just confirming.

xJonathanLEI commented 2 years ago

Hi. compute_hash_on_elements is not wrong. It's ported from here. So please do not submit a PR to "fix" it lol.

But this does NOT mean StarkEx is wrong. dYdX is just using a different encoding schema (which goes without the final length based on your description). You should be able to implement their version without forking the library since all the building blocks are exposed (though you do have to import starknet-crypto).

xJonathanLEI commented 2 years ago

I'm closing this issue optimistically. Please feel free to re-open if needed. Thanks!