Remove y from EthAccount signature. #940
Conversation
src/account/utils/signature.cairo
Outdated
| use starknet::eth_signature::Signature; | ||
| use starknet::secp256_trait::{is_signature_entry_valid, recover_public_key}; | ||
| use starknet::secp256_trait; | ||
| use starknet::secp256k1::Secp256k1Point; | ||
|
|
||
| #[derive(Copy, Drop, Serde)] | ||
| pub struct Signature { | ||
| pub r: u256, | ||
| pub s: u256, | ||
| } |
There was a problem hiding this comment.
Is the new struct necessary? We can just keep the corelib eth_signature and not use the y parity or do you think this adds something?
There was a problem hiding this comment.
The Struct is used for Serde (we can't ignore y in the other one for deserialization).
There was a problem hiding this comment.
Mmm I see. In that case, the Signature name appears a little vague with both stark and eth signature validation fns in the same module. What do you think about changing the struct name to EthSignature and documenting the reason? IMO we should at least document it
There was a problem hiding this comment.
+1 to change to EthSignature, but I feel that's enough, not sure we should document the reason. The only difference between the signatures is the types (felt252 vs u256). Do you have something specific in mind for the documentation?
There was a problem hiding this comment.
I meant documenting the difference of the OZ signature::EthSignature and starknet::eth_signature::Signature just to make it clear why we're defining a new eth sig struct as opposed to using corelib's. I was thinking that above the struct we could comment something like: "Omits the y parity because it's not required for validation." If a user wonders which eth sig type to use, this might be helpful. WDYT?
There was a problem hiding this comment.
I don't think that comment adds much, since people don't need to be aware of corelib's Signature, or why is it using y. We just defined a struct representing an EthSignature, without the parity because for this use case is not required.
…eat/eth_account_improvement
Fixes #939
PR Checklist