Skip to content
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

remove verifyingContract from stark sig #626

Merged
merged 4 commits into from
Aug 7, 2024
Merged

Conversation

pscott
Copy link
Contributor

@pscott pscott commented Jul 3, 2024

Removes the verifyingContract from the stark-sig authenticator. This is done because SNIP12 does not support the verifyingContract key.
We also rename StarkEIP712 to SNIP12 for clarity.

The only important changes in this PR are:

  1. Removing the verifyingContract from and updating the domain hash:
    // StarknetKeccak('StarkNetDomain(name:felt252,version:felt252,chainId:felt252)')
    const DOMAIN_TYPEHASH: felt252 = 0x7652a980b9a4920425c858386f09477bb210dea95169abd576f5ef7c9d5ca2;
    (check git blame)
  2. Removing the contract_address from the get_domain_hash function (was previously here, you can check git blame:
    starknet::get_contract_address().serialize(ref encoded_data);
    )

The remaining are StarkEIP712 -> SNIP12. I realize now it would've been cleaner with only two distinct commits.

@Sekhmet
Copy link
Member

Sekhmet commented Jul 4, 2024

@pscott could you deploy starkSig authenticator on sepolia with this removed so I could test full flow?

I remember that at some point there was some issue with either Metamask or ArgentX not being happy with verifyingContract not being passed.

@pscott
Copy link
Contributor Author

pscott commented Jul 4, 2024

@pscott could you deploy starkSig authenticator on sepolia with this removed so I could test full flow?

I remember that at some point there was some issue with either Metamask or ArgentX not being happy with verifyingContract not being passed.

I think it was metamask that was causing issues, but here it only concerns sx-sig so shouldn't be an issue.

New contract: 0x04a7805f0d4b34d801f2fcf7aace7bd990f3acc30e542523998f9c0d5f203055 on sepolia

Copy link
Member

@Sekhmet Sekhmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pscott pscott merged commit a135ae7 into develop Aug 7, 2024
2 checks passed
@pscott pscott deleted the remove_verifyingContract branch August 7, 2024 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants