Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: add privy server wallet provider #242
feat: add privy server wallet provider #242
Changes from 6 commits
f8eb4dc
7b194fa
189ff6a
e4e3558
47c6477
0b67c61
ba189e3
aaf356c
cfab8cf
2f4e4b3
72e88b4
356ed23
4d13dd2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please update to
chainId
as previously discussedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please update to
chainId
as previously discussedAlso - should this return value be a named interface to make it more clear?
Let's also add an example of instantiating a
PrivyWalletProvider
from the exported object.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@0xRAG @azf20 - This config should take
chainId
instead ofnetworkId
. Network ID is a CDP concept and will limit the Privy implementation to only support CDP supported networks. Privy does not have the same constraints asCdpWalletProvider
and therefore should takechainId
which will unblock usage of all EVM.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! This is done now in ba189e3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider renaming this to authorizationPrivateKey so it is clear this is the private key, not the public key or ID, also to be parallel with the Privy config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call, done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: update to
chainId: 84532
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an edge case here where the
authorizationPrivateKey
is provided but notauthorizationKeyId
which will get things into a funky state. Can you do a check to enforce that either both or neither are provided before initializing thePrivyClient
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok to initialise with just an
authorizationPrivateKey
if you're using an existing wallet, not creating a key?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right - if the user has an existing wallet that has a key attached to it, you need to specify
authorizationPrivateKey
and technically don't need to specify the ID. My bad! Fine to leave as is, good catch.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this call returns the whole wallet object (including the address), so if you shift things around a bit you can save the second network hop below to
getWallet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice yes did that too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually takes as an input an
authorizationKeyId
which is an ID assigned by to an authorization key when it is registered with Privy server wallets. In order to use an authorization key with a wallet, you need to pre register it with Privy. We are actually working on supporting key registration during wallet creation, but it is not implemented yet.I can see two paths forward here:
server-auth
but we do via our REST API (docs - seeREST API
tab). The developer would generate thep256
private + public keypair (instructions in above docs as well) and input them both as a config, then you would send the public key to privy, and you'll get back an ID which you'd use during wallet creation. Theserver-auth
config above correctly takes in the private key.authorizationKeyId
in the config, however that would require the developer to register the key with Privy themselves beforehand, which isn't an ideal experience.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for that context, sorry I had missed that you need to pass IDs for creation.
I tried 1., but hit an issue where the https://api.privy.io/v1/wallets endpoint was returning a 404 - see this branch on my project repository as a test case azf20/hello-world-computer@cd61b55
So I reverted to 2, but also found that an authorization key was required on new wallet creation, not sure if that is expected? Got the following when I didn't pass in an auth key:
So I updated the error handling and docs accordingly - not an ideal experience but I think ok for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch - I'll look into this on our end and I think option 2 + pointing folks to the Privy dashboard to create their key is a good approach for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @John-peterson-coinbase mentioned above, Privy can support any EVM chain, so you can update the config to take in a
chainId
(eg 84532 for base sepolia). In order to resolve the viem chain object, here is a post I've found helpful.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, thanks for the hint!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider replacing "Privy wallets are embedded wallets" with "Privy's server wallets enable you to securely provision and manage cross-chain wallets via a flexible API"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is worth specifying that this is the authorization private key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1