Added verify_eth_signature to secp256k1

[yuvalsw]

This change is 

[yuvalsw]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion

a discussion (no related file):
change target branch to main

---

[orizi]

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @yuvalsw)

---

corelib/src/starknet/secp256k1.cairo line 109 at r2 (raw file):

    // TODO(yg): either have 2 syscalls to get x and y of a point, and use the keccak libfunc (if
    // it makes sense), or have a single syscall to get the keccak of a point. Seconds seems better,
    //  unless getters of x and y are needed anywhere else.

I think having a syscall returning x and y is better - since it makes the other libfuncs usable.
otherwise we simply would have had the recover libfunc and nothing else.

Code quote:

    // TODO(yg): either have 2 syscalls to get x and y of a point, and use the keccak libfunc (if
    // it makes sense), or have a single syscall to get the keccak of a point. Seconds seems better,
    //  unless getters of x and y are needed anywhere else.

---

corelib/src/starknet/secp256k1.cairo line 112 at r2 (raw file):

    let x = secp256k1_ec_point_get_x(public_key_point);
    let y = secp256k1_ec_point_get_y(public_key_point);
    let point_hash = keccak(x, y);

Suggestion:

    let mut keccak_input = ArrayTrait::new();
    keccak::keccak_add_uint256_be(ref keccak_input, x);
    keccak::keccak_add_uint256_be(ref keccak_input, y);
    keccak::add_padding(ref keccak_input);
    let point_hash = starknet::syscalls::keccak_syscall(keccak_input.span()).unwrap_syscall();

---

corelib/src/starknet/secp256k1.cairo line 120 at r2 (raw file):

    let address_u256 = u256 { high: high_32_bits, low: point_hash.low };
    // TODO(yg): need U256TryIntoFelt, and use the relevant trait.
    EthAddress { address: address_u256.try_into().unwrap() }

let high_32_bits = point_hash.high % 0x100000000;

Suggestion:

    // The Ethereum address is the 160 (128+32) least significant bits of the keccak of the public key.
    let high_32_bits = point_hash.high % 0x100000000;
    let address: felt252 = high_32_bits * 0x100000000000000000000000000000000 + point_hash.low;
    EthAddress { address }

[orizi]

Reviewed 36 of 36 files at r8.
Reviewable status: 36 of 37 files reviewed, 5 unresolved discussions (waiting on @yuvalsw)

---

corelib/src/starknet/secp256k1.cairo line 109 at r8 (raw file):

/// Checks whether `value` is in the range [1, N).
fn is_signature_entry_valid(value: u256) -> bool {
    value > u256 { high: 0, low: 0 } & value < get_N()

no need for the rangechecks

Suggestion:

value != u256 { high: 0, low: 0 }

[yuvalsw]

Reviewable status: 32 of 39 files reviewed, 5 unresolved discussions (waiting on @orizi)

---

corelib/src/starknet/secp256k1.cairo line 109 at r2 (raw file):

Previously, orizi wrote…

I think having a syscall returning x and y is better - since it makes the other libfuncs usable.
otherwise we simply would have had the recover libfunc and nothing else.

Done.

---

corelib/src/starknet/secp256k1.cairo line 112 at r2 (raw file):

    let x = secp256k1_ec_point_get_x(public_key_point);
    let y = secp256k1_ec_point_get_y(public_key_point);
    let point_hash = keccak(x, y);

Done.

---

corelib/src/starknet/secp256k1.cairo line 120 at r2 (raw file):

Previously, orizi wrote…

let high_32_bits = point_hash.high % 0x100000000;

Done.

---

corelib/src/starknet/secp256k1.cairo line 109 at r8 (raw file):

Previously, orizi wrote…

no need for the rangechecks

Done.

[orizi]

Reviewed 1 of 3 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: 34 of 39 files reviewed, 2 unresolved discussions (waiting on @yuvalsw)

---

corelib/src/test/secp256k1_test.cairo line 94 at r12 (raw file):

#[test]
#[should_panic]

Suggestion:

#[should_panic(expected: ('Invalid signature', ))]

---

corelib/src/test/secp256k1_test.cairo line 107 at r12 (raw file):

#[test]
#[should_panic]

Suggestion:

#[should_panic(expected: ('Signature out of range', ))]

[orizi]

Reviewed 2 of 6 files at r9, 1 of 2 files at r10.
Reviewable status: 37 of 39 files reviewed, 2 unresolved discussions (waiting on @yuvalsw)

[orizi]

Reviewed 1 of 6 files at r9, 1 of 3 files at r11.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @yuvalsw)

[orizi]

Reviewed 2 of 2 files at r13, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yuvalsw)

[yuvalsw]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yuvalsw)

---

corelib/src/test/secp256k1_test.cairo line 94 at r12 (raw file):

#[test]
#[should_panic]

Done.

---

corelib/src/test/secp256k1_test.cairo line 107 at r12 (raw file):

#[test]
#[should_panic]

Done.

[orizi]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yuvalsw)