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 Solana #584

Merged
merged 9 commits into from
Nov 13, 2023
Merged

Sign in with Solana #584

merged 9 commits into from
Nov 13, 2023

Conversation

Funkatronics
Copy link
Contributor

@Funkatronics Funkatronics commented Oct 24, 2023

Still needs a bit of tweaking and review, but SIWS is working!

Both fakedapp and fakewallet have been updated in this PR. Fakedapp now has a "Sign in with Solana" button that will initiate an authorize request with a sign in payload. Fakewallet has been updated with a new UI to handle authorize requests that contain a sign in payload. Fakewallet with both authorize the dapp and sign the sign in payload in one step. There is also a "Simulate Sign In Not Supported" button on this UI that will complete the authorize request and ignore the signin payload, to simulate a legacy wallet, or a wallet that does not the solana:signInWithSolana feature. In this case, Fakedapp will fall back on a generic sign message request immediately after the authorize request is completed by the wallet. In both casesfakedapp will verify the signed payload and display a success message.

completes implementation for #439

@Funkatronics Funkatronics changed the title [WIP] Sign in with Solana Sign in with Solana Oct 27, 2023
@Funkatronics
Copy link
Contributor Author

Funkatronics commented Oct 27, 2023

Review notes:

This pr is quite large, but can be broken down by lib

  • common
    • contains the core SIWS payload struct and message+json parsing
  • walletlib
    • added the sign_in_payload to the authorize request and forwards this to the wallet for handling (if present)
    • added the sign_in_result to the authorization result so wallets can pass the signed payload back to the dapp along with the authorize request
  • client lib
    • added the (optional) sign_in_payload to the authorize request input and returns the sign_in_result from the wallet

Sample apps:

  • fakedapp
    • added a "Sign in with Solana" button that does authorize with a sign_in_payload
    • if the wallet fails to return the sign_in_result, it falls back on sign_message immediately after the authorize request is successfully completed
      • this could be handled in clientlib but it required significant refactor so left it here for now. clientlib-ktx could easily handle this for the consumer however (cc @creativedrewy).
  • fakewallet
    • adds a new authorization fragment that specifically handles an authorize request with a sign_in_payload
      • There is also a "Simulate Sign In Not Supported" button on this UI that will complete the authorize request and ignore the signin payload, to simulate a legacy wallet, or a wallet that does not the solana:signInWithSolana feature

Copy link
Contributor

@creativedrewy creativedrewy left a comment

Choose a reason for hiding this comment

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

A lotta plumbing for...something useful?

@@ -145,13 +146,17 @@ public static class AuthorizationResult {
public final Uri walletUriBase;
@NonNull @Size(min = 1)
public final AuthorizedAccount[] accounts;
@Nullable
public final SignInResult signInResult;
Copy link
Contributor

Choose a reason for hiding this comment

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

The ergonomics of this object feels...odd, given its overlap with some of the values in the AuthorizedAccount object. But I don't really think there is a better option than this. I think you were forced to bolt this on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah im not thrilled with it. I could subclass the authorize objects but that would add complexity elsewhere so this felt the least bad (lol).

@Funkatronics Funkatronics merged commit f3f5739 into main Nov 13, 2023
6 checks passed
@Funkatronics Funkatronics deleted the sign_in_with_solana branch November 13, 2023 15:35
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.

None yet

2 participants