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

Support wallet disambiguation and deeplinking in protocol spec, url encoding, and url parsing #115

Conversation

Saqif280
Copy link

@Saqif280 Saqif280 commented Apr 5, 2022

Problem Overview

This branch addresses requests in #100 to update the Solana Pay protocol specification to support wallet disambiguation in iOS clients and deeplinking to address some security vulnerabilities.

Solution & Decision Log

The open issue proposes several changes; this PR is the first step towards implementing those changes from Solana Pay's end. In addition to this, there will need to be followup PRs to fully close the issue.

Handling the Spec Changes

The issue poster suggested to allow for a url like https://phantom.app/code? instead of the current url scheme solana:. However, the suggested URL has a path, which should be used for the recipient according to the current protocol spec. There are a few options of how we might want to handle this:

  1. We can make a breaking change to the protocol spec and require recipient as a query parameter. I'm avoiding this since breaking changes are costly.
  2. We can continue to keep the recipient in the path, and just parse it out of the end of the wallet URL (in the cases where we use a wallet URL). This is a bit of a rigid solution since it relies on wallet applications registering a wildcard route to handle recipient in the path. The parsing also feels a little hacky to me, so I’m not a fan of this one.
  3. We can allow for a recipient query parameter. This will be encoded in the URL when a wallet is provided, otherwise it will default to putting recipient in the path. This solution is backwards compatible and flexible so I decided to implement this.

Supporting Wallet Disambiguation

This solution creates a TypeScript enum for tracking supported wallets called SupportedWallet. Wallets can create PRs to add their wallet and their registered url to this enum. This branch leaves the enum empty for now, which means we can't build much around this until enum entries are added.

Testing

  • Documentation changes were manually verified in local docusaurus
  • Code changes were tested via unit tests
    • Note: encodeURL.test.ts will need additional tests once supported wallets have been added to the SupportedWallet enum.

Next Steps

  1. System diagrams in documentation need to be updated to reflect complete call paths (given the new request query param usage). This could be included in this PR but I don't have access to the files for these diagrams.
  2. Documentation needs to be added on how to set up authentication servers for merchants and how to interact with authentication servers as wallets. This could also be included in this PR, but would be confusing without the updated system diagrams.
  3. The Point of Sale package needs to be updated to provide UIs for wallet selection.

@vercel
Copy link

vercel bot commented Apr 5, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

solana-pay-docs – ./docs

🔍 Inspect: https://vercel.com/solana-labs/solana-pay-docs/HVFn3XnpayvM7wNReFheFFhGn2g8
✅ Preview: https://solana-pay-docs-git-fork-saqif280-support-wa-9473aa-solana-labs.vercel.app

[Deployment for e29c906 failed]

solana-pay – ./point-of-sale

🔍 Inspect: https://vercel.com/solana-labs/solana-pay/CwYsp1hrdLDZGFGUuvJGfTqQh8Dq
✅ Preview: https://solana-pay-git-fork-saqif280-support-wallet-6ee23b-solana-labs.vercel.app

Comment on lines 107 to +108
try {
await validateTransactionSignature(
connection,
signature,
MERCHANT_WALLET,
amount,
undefined,
reference
);
await validateTransactionSignature(connection, signature, MERCHANT_WALLET, amount, undefined, reference);
Copy link
Author

Choose a reason for hiding this comment

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

I didn't touch this file, I think my prettier caught it but going to leave in this PR

@jordaaash
Copy link
Collaborator

This is a breaking change, and is outdated, so probably won't be included.

@jordaaash jordaaash closed this Jul 29, 2022
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