search

xJonathanLEI / starknet-rs

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

Improve codebase, fix potential issues #446

Closed satoshiotomakan closed 1 year ago

satoshiotomakan commented 1 year ago

There are a few places that may lead to panic, have TODOs, and have documentation inconsistency.

Explicit panic

Is there a way to get panic in real world cases? https://github.com/xJonathanLEI/starknet-rs/blob/91425ad9c20cceaf87f5ae179341f7e1f7958507/starknet-crypto/src/fe_utils.rs#L54 https://github.com/xJonathanLEI/starknet-rs/blob/91425ad9c20cceaf87f5ae179341f7e1f7958507/starknet-crypto/src/rfc6979.rs#L52

TODO

https://github.com/xJonathanLEI/starknet-rs/blob/91425ad9c20cceaf87f5ae179341f7e1f7958507/starknet-crypto/src/rfc6979.rs#L58

Documentation inconsistency

Pedersen points at curve_params.rs are different from what said in the documentation: https://docs.starkware.co/starkex/crypto/pedersen-hash-function.html

xJonathanLEI commented 1 year ago

You're referencing starkex docs.

satoshiotomakan commented 1 year ago

@xJonathanLEI yes, so the mistake is in the starkex docs or in code implementation? I guess the first because starknet-rs generates valid signatures. If so, I'll address the question to them

satoshiotomakan commented 1 year ago

@xJonathanLEI I've seen you sent a reply to my last question. Not sure why I can't see it anymore, but is that correct?

Sorry what I meant was that this is a Starknet library not StarkEx. Afaik StarkEx uses different parameters.

Could you please send a documentation to the curve implemented by the starknet-rs?

xJonathanLEI commented 1 year ago

I deleted that comment because it's not correct. Turns out Starknet and StarkEx uses the exact same curve and Perdersen params.

Printing the values from the file you referenced:

SHIFT_POINT.x = 2089986280348253421170679821480865132823066470938446095505822317253594081284
SHIFT_POINT.y = 1713931329540660377023406109199410414810705867260802078187082345529207694986

PEDERSEN_P0.x = 996781205833008774514500082376783249102396023663454813447423147977397232763
PEDERSEN_P0.y = 1668503676786377725805489344771023921079126552019160156920634619255970485781

PEDERSEN_P1.x = 2251563274489750535117886426533222435294046428347329203627021249169616184184
PEDERSEN_P1.y = 1798716007562728905295480679789526322175868328062420237419143593021674992973

PEDERSEN_P2.x = 2138414695194151160943305727036575959195309218611738193261179310511854807447
PEDERSEN_P2.y = 113410276730064486255102093846540133784865286929052426931474106396135072156

PEDERSEN_P3.x = 2379962749567351885752724891227938183011949129833673362440656643086021394946
PEDERSEN_P3.y = 776496453633298175483985398648758586525933812536653089401905292063708816422

It's clear that starknet-rs uses the exact same params as in https://docs.starkware.co/starkex/crypto/pedersen-hash-function.html, just named differently. This should resolve the 3rd part of the submitted issue.

xJonathanLEI commented 1 year ago

Regarding this panic:

https://github.com/xJonathanLEI/starknet-rs/blob/91425ad9c20cceaf87f5ae179341f7e1f7958507/starknet-crypto/src/rfc6979.rs#L52

I should have added a comment above this line. This never panics. The value k comes from generate_k_shifted(), which ensures the value is smaller than EC_ORDER, and hence always a valid FieldElement.

xJonathanLEI commented 1 year ago

As for this panic:

https://github.com/xJonathanLEI/starknet-rs/blob/91425ad9c20cceaf87f5ae179341f7e1f7958507/starknet-crypto/src/fe_utils.rs#L54

This function itself could panic when the GCD is not one. However, this function is only used internally (never part of the public API) against EC_ORDER, which is a prime, so GCD is always one. Overall, the usage of the function inside the lib never panics.

This is not exactly a good design though. We should probably leave the unwrapping to callers themselves instead of making assumptions on input (if so maybe at least document it). However, this is just an implementation detail.

xJonathanLEI commented 1 year ago

As for the TODO, it doesn't really affect anything other than basically duplicating the upstream code (with slight modification for the bit shift of course). There's currently no plan on fixing that yet.

xJonathanLEI commented 1 year ago

Closing this as it's not actionable (or at least, no plan for performing the action). Feel free to submit a PR switching to use the upstream generate_k though.