-
Notifications
You must be signed in to change notification settings - Fork 289
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
base: main
Are you sure you want to change the base?
Conversation
🟡 Heimdall Review Status
|
|
||
const network = { | ||
protocolFamily: "evm" as const, | ||
networkId: config.networkId || "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.
network ID for base sepolia is base-sepolia
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, fixed!
thanks @John-peterson-coinbase, added fixes here f49f51d, including package-lock.json (though on reflection wasn't sure about the policy there). Let me know if there is anything else! |
Thanks @azf20 - please rebase and sign the commits following the commit signing instructions |
bbb3359
to
fca9cf3
Compare
thanks so much, I rebased and verified, hope that looks right! |
fca9cf3
to
452f2b0
Compare
Hey @azf20 I tested this locally and it's working great! As a couple final asks, could you:
Thank you! |
452f2b0
to
6d6eb39
Compare
OK great! I rebased and added an example - I added a comment explaining that Privy server wallets are embedded & controllable via the dashboard & API. Let me know if that makes sense or if you wanted that elsewhere |
Nice, thanks! Two things:
|
Also, what are your thoughts on this ask:
Is this simply saving off the wallet ID, then using that to query Privy? |
4b584eb
to
73a8080
Compare
Thanks! |
typescript/agentkit/src/wallet-providers/privyWalletProvider.ts
Outdated
Show resolved
Hide resolved
Also @azf20 two more things before we can merge:
Almost there! |
61d7a46
to
0b67c61
Compare
Thanks so much! Rebased + updated README and Changelog |
/** The ID of the wallet to use */ | ||
walletId?: string; | ||
/** Optional network ID to connect to */ | ||
networkId?: string; |
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 of networkId
. 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 as CdpWalletProvider
and therefore should take chainId
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
Review Error for John-peterson-coinbase @ 2025-02-13 23:31:28 UTC |
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.
Wow, this is great! Thanks for doing this. I'm Colfax from the Privy team, it's nice to meet you, this was enjoyable to review.
The core change that needs to be made is around how the authorization private key + ID is handled during wallet creation. Feel free to let me know if you have any questions!
networkId: config.networkId || "base-sepolia", | ||
}; | ||
|
||
const chain = NETWORK_ID_TO_VIEM_CHAIN[network.networkId!]; |
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!
const walletId = | ||
config.walletId ?? | ||
( | ||
await privy.walletApi.create({ |
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
( | ||
await privy.walletApi.create({ | ||
chaintype: "ethereum", | ||
authorizationKeyIds: config.authorizationKey ? [config.authorizationKey] : undefined, |
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:
- You can implement authorization key registration with Privy before wallet creation. We don't have support for it in
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. - You can take in an
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:
⨯ Error: Missing `privy-authorization-signature` header.
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.
/** Optional network ID to connect to */ | ||
networkId?: string; | ||
/** Optional authorization key for the wallet API */ | ||
authorizationKey?: string; |
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
- PRIVY_APP_ID= | ||
- PRIVY_APP_SECRET= | ||
- PRIVY_WALLET_ID=[optional, otherwise a new wallet will be created] | ||
- PRIVY_WALLET_AUTHORIZATION_KEY=[optional, only if you are using authorization keys for your server wallets] |
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
|
||
This example demonstrates an agent setup as a self-aware terminal style chatbot with a [Privy server wallet](https://docs.privy.io/guide/server-wallets/). | ||
|
||
Privy wallets are embedded wallets - learn more at https://docs.privy.io/guide/server-wallets/. The Agentkit integration assumes you have a Privy server wallet ID which you want to use for your agent - creation and management of Privy wallets can be done via the Privy dashboard or 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.
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
Review Error for colfax23 @ 2025-02-14 04:56:41 UTC |
Thanks so much for taking a look @colfax23! I fixed and tested new wallet creation, see the approach taken here: #242 (comment). I encountered some issues which might be on the Privy side, so I went ahead with an approach that worked to create new server wallets in Privy: Also updated to take any chainId (assuming it's supported by viem), as well as some docs / naming clarifications. Appreciate the help! |
An update: @colfax23 shared a URL which will work for wallet creation, and I go that working locally, with the slight downside that it creates a new signing key (with the same public key) on every run. Would be good to clarify expected behaviour on |
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.
Looks great! Left a few minor comments, but once addressed this looks good to go from my perspective.
* appId: "your-app-id", | ||
* appSecret: "your-app-secret", | ||
* walletId: "wallet-id", | ||
* networkId: "base-sepolia" |
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!
* ``` | ||
*/ | ||
public static async configureWithWallet(config: PrivyWalletConfig): Promise<PrivyWalletProvider> { | ||
const privy = new PrivyClient(config.appId, config.appSecret, { |
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 not authorizationKeyId
which will get things into a funky state. Can you do a check to enforce that either both or neither are provided before initializing the PrivyClient
?
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.
- PRIVY_APP_ID= | ||
- PRIVY_APP_SECRET= | ||
- PRIVY_WALLET_ID=[optional, otherwise a new wallet will be created] | ||
- PRIVY_WALLET_AUTHORIZATION_PRIVATE_KEY=[optional, only if you are using authorization keys for your server wallets] |
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.
updating README
( | ||
await privy.walletApi.create({ | ||
chaintype: "ethereum", | ||
authorizationKeyIds: config.authorizationKey ? [config.authorizationKey] : undefined, |
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.
Review Error for colfax23 @ 2025-02-14 13:26:16 UTC |
Thanks, sounds good - updated READMEs accordingly |
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.
LGTM pending a few instances of networkID
-> chainID
remaining, a few documentation updates, and a small change to the example.
We can land this in today's EOW release if these are resolved.
typescript/agentkit/README.md
Outdated
const config = { | ||
appId: "PRIVY_APP_ID", | ||
appSecret: "PRIVY_APP_SECRET", | ||
networkId: "base-sepolia", |
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 discussed
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.
👍
* @param id - The chain ID | ||
* @returns The chain | ||
*/ | ||
function getChain(id: number) { |
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.
👍
typescript/agentkit/README.md
Outdated
#### Exporting Privy Wallet information | ||
|
||
The `PrivyWalletProvider` can export wallet information by calling the `exportWallet` method. | ||
|
||
```typescript | ||
const walletData = await walletProvider.exportWallet(); | ||
|
||
// walletData will be in the following format: | ||
{ | ||
walletId: string; | ||
authorizationKey: string | undefined; | ||
networkId: string; | ||
} | ||
``` |
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 discussed
Also - 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.
👍
appSecret: string; | ||
/** The ID of the wallet to use, if not provided a new wallet will be created */ | ||
walletId?: string; | ||
/** Optional network ID to connect to */ |
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.
/** Optional network ID to connect to */ | |
/** Optional chain ID to connect to */ |
/** The ID of the wallet to use, if not provided a new wallet will be created */ | ||
walletId?: string; | ||
/** Optional network ID to connect to */ | ||
chainId?: number; |
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.
chainId?: number; | |
chainId?: string; |
Update type to conform with Network
interface typing.
networkId?: string; | ||
} { | ||
return { | ||
walletId: this.#walletId, | ||
authorizationPrivateKey: this.#authorizationPrivateKey, | ||
networkId: this.getNetwork().networkId, |
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.
networkId?: string; | |
} { | |
return { | |
walletId: this.#walletId, | |
authorizationPrivateKey: this.#authorizationPrivateKey, | |
networkId: this.getNetwork().networkId, | |
chainId: string; | |
} { | |
return { | |
walletId: this.#walletId, | |
authorizationPrivateKey: this.#authorizationPrivateKey, | |
chainId: this.getNetwork().chainId, |
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.
👍
const config = { | ||
appId: process.env.PRIVY_APP_ID as string, | ||
appSecret: process.env.PRIVY_APP_SECRET as string, | ||
networkId: process.env.NETWORK_ID || "base-sepolia", |
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: use chain 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.
👍
// Warn about optional NETWORK_ID | ||
if (!process.env.NETWORK_ID) { | ||
console.warn("Warning: NETWORK_ID not set, defaulting to base-sepolia testnet"); | ||
} |
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: use chain 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.
👍
const exportedWallet = walletProvider.exportWallet(); | ||
fs.writeFileSync(WALLET_DATA_FILE, JSON.stringify(exportedWallet)); |
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.
If WALLET_DATA_FILE
already exists, the example should read the exported wallet data from the file and instantiate the existing wallet.
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.
updated to do this
typescript/agentkit/README.md
Outdated
|
||
#### Authorization Keys | ||
|
||
Privy offers the option to use authorization keys to secure your server wallets. When using authorization keys, you must provide the `authorizationPrivateKey` and `authorizationKeyId` parameters to the `configureWithWallet` method. |
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 👍
thanks so much for the thorough feedback, good catches! pushed changes |
if (!config.authorizationPrivateKey) { | ||
throw new Error("authorizationPrivateKey is required when creating a new wallet"); | ||
} | ||
if (!config.authorizationKeyId) { | ||
throw new Error( | ||
"authorizationKeyId is required when creating a new wallet with an authorization key, this can be found in your Privy Dashboard", | ||
); |
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:
- This UX of requiring it in all cases if a key is created is not intuitive, we're working on some updates here
- The error message when you try to create a wallet without specifying the key, when it is required, is not very clear (it is
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
try { | ||
const wallet = await privy.walletApi.create({ | ||
chainType: "ethereum", | ||
authorizationKeyIds: [config.authorizationKeyId], |
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.
@azf20 config.authorizationKeyId
could be null
here, we need to optionally pass it:
authorizationKeyIds: config.authorizationKeyId ? [config.authorizationKeyId] : undefined,
if (config.authorizationPrivateKey && !config.authorizationKeyId) { | ||
throw new Error( | ||
"authorizationKeyId is required when creating a new wallet with an authorization key, this can be found in your Privy Dashboard", | ||
); | ||
} |
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 ran into the opposite error scenario which I think we should handle: I was passing in an authorizationKeyId
but no authorizationPrivateKey
. Let's add an error case for this as well
if (config.authorizationKeyId && !config.authorizationPrivateKey) {
throw new Error(
"authorizationPrivateKey is required when creating a new wallet with an authorization key. " +
"If you don't have it, you can create a new one in your Privy Dashboard, or delete the authorization key.",
);
}
What changed? Why?
EvmWalletProvider
), extending the viem provider using @privy-io/server-auth (which has acreateViemAccount
method)Qualified Impact