Skip to content

feat: add svm keypair wallet provider - #246

Merged
0xRAG merged 19 commits into
coinbase:masterfrom
ngundotra:svm-keypair
Feb 11, 2025
Merged

feat: add svm keypair wallet provider#246
0xRAG merged 19 commits into
coinbase:masterfrom
ngundotra:svm-keypair

Conversation

@ngundotra

@ngundotra ngundotra commented Feb 2, 2025

Copy link
Copy Markdown
Contributor

What changed? Why?

Add Solana wallet provider abstract class called svmWalletProvider in order to pave the way for Solana integration with Coinbase's AgentKit. There is a concrete implementation called SvmKeypairWalletProvider that performs the basic functions to implement Solana actions.

This PR updates the WalletProvider instance from abstract getNetwork() -> Network to abstract getNetwork() -> Promise<Network> in order to support Solana network retrieval, which requires a network request to check genesis hash.

Qualified Impact

  • this change may have to be rolled back to prevent breaking changes to the WalletProvider interface

This PR has minimal evidence to show it works because there are currently no Solana actions tightly integrated with this SDK.

 PASS  src/wallet-providers/solanaKeypairWalletProvider.test.ts
  Solana Keypair Wallet
    ✓ should initialize correctly via convenience getters (502 ms)
    ✓ should error when the network genesis hash is unknown (29 ms)

@cb-heimdall

cb-heimdall commented Feb 2, 2025

Copy link
Copy Markdown

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@ngundotra
ngundotra force-pushed the svm-keypair branch 2 times, most recently from 9cb2b3e to 8e81267 Compare February 2, 2025 20:00
@ngundotra
ngundotra marked this pull request as ready for review February 2, 2025 23:54
Comment thread typescript/agentkit/src/wallet-providers/svmKeypairWalletProvider.ts Outdated
Comment thread typescript/agentkit/src/wallet-providers/svmKeypairWalletProvider.ts Outdated
Comment thread typescript/agentkit/src/network/svm.ts Outdated
* @param transaction - The transaction to sign.
* @returns The signed transaction.
*/
abstract signTransaction(transaction: VersionedTransaction): VersionedTransaction;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
abstract signTransaction(transaction: VersionedTransaction): VersionedTransaction;
abstract signTransaction(transaction: VersionedTransaction): Promise<VersionedTransaction>;

Signing will be async with some wallet provider implementations

Comment thread typescript/agentkit/src/wallet-providers/solanaKeypairWalletProvider.ts Outdated
@John-peterson-coinbase John-peterson-coinbase added wallet provider New wallet provider triaged changes requested PR / Issue has changes requested apple labels Feb 10, 2025
@John-peterson-coinbase

Copy link
Copy Markdown
Contributor

@ngundotra - LGTM. Please commit last pending change request + update package-lock.json and we will be happy to accept! 🚀

@ngundotra

Copy link
Copy Markdown
Contributor Author

@John-peterson-coinbase ty for ping! I addressed feedback & updated interface. Also added a test in wallet-providers, but looks a bit unwieldy. Lmk if I should move it somewhere else.

@0xRAG 0xRAG removed the triaged label Feb 10, 2025
@0xRAG

0xRAG commented Feb 11, 2025

Copy link
Copy Markdown
Contributor

Hey @ngundotra just fyi – two things:

  1. I don't think I'm seeing the test you mentioned. I only see your addition of solanaKeypairWalletProvider.test.ts
  2. I'm seeing invalid package-lock.json changes. Please make sure you're on npm 8.9.0 and re-run npm install

@ngundotra

Copy link
Copy Markdown
Contributor Author

@0xRAG Thanks for the tag.

  1. Yup that's the test
  2. Updated package-lock.json with nvm use 8.9.0 && npm i let me know if that worked, or if there's a command for me to check/run this myself

Comment on lines +21 to +27
try {
new SolanaKeypairWalletProvider({ keypair: Keypair.generate().secretKey, rpcUrl: clusterApiUrl('mainnet-beta'), genesisHash: "0x123" });
} catch (error) {
expect(error).toBeDefined();
expect(error).toBeInstanceOf(Error);
expect((error as Error).message).toContain("Unknown network with genesis hash");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please update to:

expect(() => new SolanaKeypairWalletProvider({ keypair: Keypair.generate().secretKey, rpcUrl: clusterApiUrl('mainnet-beta'), genesisHash: "0x123" })).toThrowError("Unknown network with genesis hash");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated! Thanks for this, way cleaner

@0xRAG

0xRAG commented Feb 11, 2025

Copy link
Copy Markdown
Contributor

@ngundotra

@0xRAG Thanks for the tag.

  1. Yup that's the test
  2. Updated package-lock.json with nvm use 8.9.0 && npm i let me know if that worked, or if there's a command for me to check/run this myself
  1. Got it! Yes that's fine, I think eventually we will want to group by folders there, but this is fine for now. BTW I added a comment about the actual test
  2. Still looks incorrect – try restoring the lockfile back to what it is on main, then run npm i again. BTW nvm use 8.9.0 tells nvm to use Node.js version 8.9.0, not npm. You can specify an npm version with npm i -g npm@8.9.0

@0xRAG

0xRAG commented Feb 11, 2025

Copy link
Copy Markdown
Contributor

Also the PR has a formatting failure – pls run npm run format from root

],
"dependencies": {
"@coinbase/coinbase-sdk": "^0.17.0",
"@solana/web3.js": "^1.98.0",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I prefer not to right now, since the interface is changing, and not all ecosystem packages have upgraded to use it.

But if you really want to get in the guts, it's absolutely more flexible than v1. Bit of a learning curve though

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Got it – that's fine. Only thing I would suggest is to preemptively rename solanaKeyPairWalletProvider and related to solanaV1KeyPairWalletProvider. BTW, why not call svmWalletProvider -> solanaWalletProvider?

@ngundotra ngundotra Feb 11, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Technically v1 and v2 of @solana/web3.js are compatible. @solana/web3.js@next is just a more opinionated & web-optimized version of v1. Therefore, I don't think it makes sense to name the implementation based on the version of the framework it uses.

That being said, I will version if you think it's better for longevity.

why not call svmWalletProvider -> solanaWalletProvider

I was following the evm naming convention. Here, svm refers to Solana Virtual Machine-like networks that have the same signing curve & similar virtual machine behavior (not formally defined). For example - Eclipse, Sonic, Soon, etc.

I reserved SolanaWalletProvider for an implementation that has some hard-coded checks against the canonical Solana clusters (mainnet-beta, testnet, and devnet)

@0xRAG

0xRAG commented Feb 11, 2025

Copy link
Copy Markdown
Contributor

Not sure what those Python test failures are about, maybe try rebasing the PR

There are some lint errors to fix now that the formatting is fixed

@0xRAG

0xRAG commented Feb 11, 2025

Copy link
Copy Markdown
Contributor

@ngundotra Thanks for pushing through on this! Two more things:

  1. To get this PR finalized and merged, please add a changelog entry typescript/agentkit/CHANGELOG.md
  2. Please add a screenshot(s) to the PR description showing the change working as expected

@0xRAG

0xRAG commented Feb 11, 2025

Copy link
Copy Markdown
Contributor

@ngundotra Almost there! Can you pls update the PR title to follow Conventional Commitsfeat: add svm keypair wallet provider

@0xRAG

0xRAG commented Feb 11, 2025

Copy link
Copy Markdown
Contributor

@ngundotra Great work here! Let's update the title and then we can merge 🚀

@John-peterson-coinbase

Copy link
Copy Markdown
Contributor

@ngundotra - you will also need to rebase onto signed commits before merging to master. Please follow these instructions to set up commit signing

@ngundotra ngundotra changed the title Add svm keypair wallet provider eat: add svm keypair wallet provider Feb 11, 2025
@0xRAG 0xRAG added in review PR / issue is in review and removed changes requested PR / Issue has changes requested labels Feb 11, 2025
@ngundotra

Copy link
Copy Markdown
Contributor Author

@John-peterson-coinbase sorry for the spam force rebases... I had added the key to SSH keys, not signing keys 🤦‍♂️

@0xRAG

0xRAG commented Feb 11, 2025

Copy link
Copy Markdown
Contributor

@John-peterson-coinbase sorry for the spam force rebases... I had added the key to SSH keys, not signing keys 🤦‍♂️

it looks like all but one commit is signed: f02b986

@ngundotra

Copy link
Copy Markdown
Contributor Author

@0xRAG should be good to go now

@0xRAG
0xRAG merged commit 1207d04 into coinbase:master Feb 11, 2025
@0xRAG

0xRAG commented Feb 11, 2025

Copy link
Copy Markdown
Contributor

@ngundotra thank you, merged! 🚀

gtspencer pushed a commit to gtspencer/agentkit that referenced this pull request Feb 13, 2025
gtspencer pushed a commit to gtspencer/agentkit that referenced this pull request Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

apple in review PR / issue is in review wallet provider New wallet provider

Development

Successfully merging this pull request may close these issues.

4 participants