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

Sign-In with Ethereum + ERC1271 #246

Merged
merged 8 commits into from
Jul 18, 2022

Conversation

rkreutz
Copy link
Contributor

@rkreutz rkreutz commented Jun 19, 2022

Hey there,

This PR adds some structs/classes around Sign-In with Ethereum (EIP-4361) which is a new standard for trustless sign-ins. This was based on the TypeScript implementation of the same feature by the people behind the proposal.

As part of the implementation, I also added ERC-1271 support. To test it I've deployed a couple contracts to Rinkeby, repo is here.

I've added tests and made sure they are also running fine on Linux (since this will be specially useful for Server Side).

Cheers

@DarthMike
Copy link
Member

Thanks @rkreutz for the PR. Will take a bit to review as need to read EIP and a bit loaded with other work this week. But definitely good addition to library!

@dioKaratzas dioKaratzas self-requested a review June 21, 2022 10:49
@dioKaratzas
Copy link
Collaborator

dioKaratzas commented Jun 21, 2022

Please check ERC20Tests.swift line 115 about how to make the newly added tests to be tested with EthereumWebSocketClient also.

@rkreutz
Copy link
Contributor Author

rkreutz commented Jun 23, 2022

@dnKaratzas added the tests there

@dioKaratzas
Copy link
Collaborator

dioKaratzas commented Jun 24, 2022

@dnKaratzas added the tests there

I know but need to rename the corresponding class names from WSS to WebSocket

@rkreutz
Copy link
Contributor Author

rkreutz commented Jun 24, 2022

@dnKaratzas done

Copy link
Member

@DarthMike DarthMike left a comment

Choose a reason for hiding this comment

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

@rkreutz Looks good! Only one small comment, and we can merge it

/// - Returns: a `Bool` indicating if the pair message-signature is verified (whether or not the signature came from the address in the message)
/// - Throws: `SiweVerifier.Error` if message is not verifiable;
/// might throw `KeyUtilError` in case recovering the address that signed the message fails
open func verify(message: SiweMessage, against signature: String) async throws -> Bool {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be public not open?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 39e0169

@rkreutz rkreutz requested a review from DarthMike June 29, 2022 16:02
Copy link
Collaborator

@dioKaratzas dioKaratzas left a comment

Choose a reason for hiding this comment

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

LGTM!

web3sTests/ERC1271/ERC1271Tests.swift Outdated Show resolved Hide resolved
web3sTests/SIWE/SIWETests.swift Outdated Show resolved Hide resolved
web3sTests/SIWE/SiweVerifierTests.swift Outdated Show resolved Hide resolved
@rkreutz
Copy link
Contributor Author

rkreutz commented Jul 14, 2022

@DarthMike I believe this got forgotten at some point? 😄

@DarthMike DarthMike merged commit 17e3693 into argentlabs:develop Jul 18, 2022
@DarthMike
Copy link
Member

Thanks @rkreutz, merged

@rkreutz rkreutz deleted the feature/sign-in-with-ethereum branch September 6, 2022 15:00
@rkreutz
Copy link
Contributor Author

rkreutz commented Sep 6, 2022

Hey guys, any plans on making a new release to include this one? Thanks

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.

3 participants