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 9 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 hyperlink to Privy documentation on Authorization Keys given this may be a new concept to AgentKit developers.
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.
Shuffled it around to put the link to the docs before this 👍
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.
This function should be exported from https://github.com/coinbase/agentkit/blob/main/typescript/agentkit/src/network/network.ts
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.
Update type to conform with
Network
interface typing.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 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.
It's my understanding that an authorization key is optional when creating a Privy server wallet, so can we relax the requirement here?
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.
That was also my understanding, though when I tested the header was required - @colfax23 can you confirm if that's only in the case where an account has set up Authorization keys (which mine had)? If so we can update to helpfully handle the resulting error, rather than blocking here
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.
Authorization keys are not required for wallet creation unless you've created one in the dashboard and have given in the permissions
Can create and modify wallets
. If you've done that, we require it for all wallet creation requests.Two pieces of feedback we're taking from this & working on:
Missing privy-authorization-signature header.
)For the next steps, I think we can relax this constraint, and @azf20 if you can handle the error gracefully if a key is required but not present that'd be great!
Also, @azf20 when you mention in the README about creating authorization keys in the Privy dashboard, can you add a note to uncheck
Can create and modify wallets
to help prevent people from getting into this state by mistake like you did? That way they can create a key that is intended for wallet transaction signing purposes only.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.
Got it, thanks for the clarification @colfax23!
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 both! I added a more helpful error message for that specific case + updated the README
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.
Include
networkId
in network object. You can use the record here to map between chain ID and network IDThere 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.
Hmm actually thinking do we need this network object? we only use it to fetch the Viem chain, to be passed into the Viem WalletClient - could just pass chainId in directly? the underlying viem provider actually provides the network info https://github.com/coinbase/agentkit/blob/main/typescript/agentkit/src/wallet-providers/viemWalletProvider.ts#L170
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.
Update to use chain ID as previously discussed.
This should return a named interface object for clarity.
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.
Can you link the key creation docs and / or point folks to the Privy dashboard to create their key + register it with Privy to get their ID?
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.
updating README