-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: recover_sender #614
base: main
Are you sure you want to change the base?
feat: recover_sender #614
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💪💪
let r = tx.value.legacy_transaction.value.r; | ||
let s = tx.value.legacy_transaction.value.s; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be acceptable to do a TxImpl, in which you have a function get_signature_components
that returns (r, s, v) supporting all transaction types.
Once this is done you can move out the r, s checks step (which is copy/pasted in each tx variant) to be done right before. it's a very small overhead to pay for simplicity. Open to discuss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote to do this only if another function requires r, s, v to be extracted from Transaction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i'll do some utils for check_transaction pr
tempvar err = new EthereumException(InvalidSignatureError); | ||
return (Address(0), err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
considering that the error is never caught and acted upon, we can assert 1 = 0 and wrap in a with_attr
(for all errors in this function).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and change the signature to return only the address
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i tried it like this but it didn't work in the runner / serde for strict_raises
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since in unit tests, python will raise the ethereumexception in python, and in cairo an assert_eq
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, otherwise we can do with_attr error_message(name of the ethereum exception which will never be caught and acted upon) and reverse the test order to catch the python ethereum exception and match on name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would work now
test_U256_to_le_bytes20 has an endianess issue |
closes #563