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

WalletConnect V2 #187

Closed
wants to merge 3 commits into from
Closed

Conversation

AlexBHarley
Copy link
Contributor

@AlexBHarley AlexBHarley commented Dec 2, 2021

Trying to get this working with Steakwallet. The repository for Steakwallet is private at the moment, but I can release a version of it with the Solana <> WalletConnect enabled if anyone is interested in trying it.

Adapter mostly works but would be nice to:

  • checks persisted state on reload
  • handle multiple sessions
  • confirm with someone that we want our over the wire signature encoding to be base64, probably not

@jordaaash
Copy link
Collaborator

Wow, thanks for taking this on! I'd love to see how you're integrating this to be able to test it out.

@AlexBHarley
Copy link
Contributor Author

Take a look at https://twitter.com/steakwallet/status/1466803157233803270, it's not live right now on app stores though.

We already support Cosmos and Ethereum via WalletConnect V2, so Solana wasn't much of a stretch. Happy to just follow whatever you or the WC team recommend for the over the wire format.

@jordaaash
Copy link
Collaborator

This looks really clean.

I like that the JSON wire format of the transaction should easily allow a wallet to show it. It should probably include any signatures that were added (e.g. with partialSign).

It could also serialize the transaction (provided { requireAllSignatures: false, verifySignatures: false } as options) and then deserialize it.

Upside is a lot of bytes saved over the wire (accounts get compressed) and it follows the common format, but downside is the client needs to know how to deserialize it (not really an issue if the client has web3.js, but maybe a problem for a mobile app).

What do you think?

@jordaaash
Copy link
Collaborator

Actually, the more I think about it, the wallet app is going to need to know how to serialize the tx anyway to sign it. So we should probably use the ordinary wire format for it?

@AlexBHarley
Copy link
Contributor Author

AlexBHarley commented Dec 5, 2021

Thanks for the feedback! I've addressed most of the points.

So, I'd defer to you for what we're sending over the wire, you know the Solana ecosystem better than myself. This was just a quick proof of concept but whatever we ship here will likely become part of the WalletConnect documentation, so I'd hate to rush the format and not have it be the long term standard.

I agree that wallets should always be able to decode a serialised transaction and present some info to users when being signed. Althought there's not really a precedent in WalletConnect for what gets sent, here's what other networks do:

  • eth_sendTransaction is a well formed JSON object with the properties gas, to, from, but obviously data can be an encoded contract call
  • cosmos_signDirect is a well formed JSON object with signerAddress, chainId but the actual interesting bodyBytes field is encoded
  • Stellar stellar_signAndSubmitXDR is totally opaque with just an xdr field.

@pedrouid do you have an opinion on what would be better here?

@jordaaash
Copy link
Collaborator

Linking this important docs PR for reference: WalletConnect/walletconnect-docs#108

@anza-xyz anza-xyz deleted a comment from Syaba999 Jan 26, 2022
@jordaaash
Copy link
Collaborator

Closing for now since this isn't implemented in WalletConnect yet. We can reopen when it's ready there!

@jordaaash jordaaash closed this Mar 13, 2022
@jordaaash jordaaash mentioned this pull request Mar 13, 2022
@jordaaash jordaaash reopened this Mar 13, 2022
@AlexBHarley
Copy link
Contributor Author

Closing in favour of #366

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