Repository navigation
refactor(auth): address EIP-712 follow-up feedback - #3898
VAIBHAVJINDAL3012 wants to merge 19 commits into
Conversation
|
Hi @mmagician, I have the helper out of the signature.masm. Can you please take a look? |
partylikeits1983
left a comment
partylikeits1983
left a comment
There was a problem hiding this comment.
Looks good!
I just think we need to make a slight adjustment to the tests (see comment below)
| CodeExecutor::with_default_host() | ||
| .extend_advice_inputs(advice) | ||
| .run(&script) | ||
| .await | ||
| .map(|_| ()) | ||
| verify_eip712_signature( | ||
| domain.separator().into(), | ||
| transaction.eip712_hash_struct().into(), | ||
| public_key, | ||
| signature, | ||
| ) | ||
| .await |
There was a problem hiding this comment.
We should still use the MASM transaction summary adapter here. Computing the EIP-712 domain separator and transaction summary struct hash in Rust skips part of the production MASM code, so these fixtures no longer test the entire in production execution path.
partylikeits1983
left a comment
partylikeits1983
left a comment
There was a problem hiding this comment.
Looks good to me!
Lets get 1 more approval before merging though cc @mmagician @PhilippGackstatter
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
LGTM with one suggestion.
| #! - signature verification fails. | ||
| #! | ||
| #! Invocation: exec | ||
| pub proc verify_eip712_signature( |
There was a problem hiding this comment.
Isn't this more like a helper for signatures::verify_signatures rather than general-purpose? Consider making it a helper procedure in signatures.masm to remove it from the public API. Having this and verify isn't super clean.
There was a problem hiding this comment.
I would prefer keeping the EIP-712 logic together under eip712, consistent with @mmagician’s earlier feedback. I agree this procedure is specific to the signature-verification flow, though. Is your main concern exposing it publicly, or the module placement itself?
There was a problem hiding this comment.
Is your main concern exposing it publicly, or the module placement itself?
The main concern is that it is publicly accessible and that I don't think any external caller would call it, because it is effectively an miden-standards-internal helper.
I prefer moving it to signatures.masm, as it is a helper for that module, but making it private where it is is also fine, if you prefer the current structure.
There was a problem hiding this comment.
Thanks @PhilippGackstatter, I’ve moved verify_eip712_signature into signature.masm as a private helper. Making the procedure private in the eip712 module would prevent signature.masm from calling it.
|
Looks good to me, I think all thats left is to resolve merge conflict with changelog & address Philipp's comment. |
This branch has not been deployed
4 participants
Summary
Follow-up to #3856 addressing review feedback:
auth::eip712;Tests
cargo +nightly fmt --all -- --checkcargo test -p miden-testing --test lib— 725 passedforge test -vv— 1 passed