feat(PSDK-670): Support external wallet imports, wallet imports from CDP Python SDK #347

derek-cb · 2024-12-17T22:17:00Z

What changed? Why?

Support external 1-of-1 wallet import via BIP-39 mnemonic phrases (12, 15, 18, 21, or 24 words)
Support import of wallet data exported from CDP Python SDK
Renamed saveSeed() to saveSeedtoFile(), and deprecated the former
Renamed loadSeed() to loadSeedFromFile(), and deprecated the former
Added retry mechanism to address.listTransactions() E2E test for better resiliency

Testing

Tested via:

Import of 12, 15, 18, 21, and 24-word mnemonic seed phrases
Export of wallets generated via mnemonic seed phrases
Import of wallets generated via mnemonic seed phrases, that were then exported
Successfully imported wallet exported from CDP Python SDK v0.12.0
Successfully imported wallet exported from CDP NodeJS SDK v0.11.0

Qualified Impact

… 18, 21, or 24 words) - Support import of wallet data exported from CDP Python SDK - Deprecated existing Wallet.import() method in favor of Wallet.load() (more descriptive of the fact that we're loading an existing CDP wallet, not importing one into CDP)

src/coinbase/wallet.ts

- Renamed loadSeed() to loadSeedFromFile(), and deprecated the former - Merged mnemonic phrase import into the import() method - Merged WalletData and WalletDataSnake into one type, for clarity in method args - Updated unit tests

# Conflicts: # src/coinbase/types.ts # src/coinbase/wallet.ts

…base/coinbase-sdk-nodejs into feat/external-wallet-import # Conflicts: # CHANGELOG.md

…iliency

0xRAG · 2024-12-19T18:23:44Z

src/coinbase/types.ts

+  /**
+   * The CDP wallet ID in either camelCase or snake_case format, but not both.
+   */
+  walletId?: string;
+  wallet_id?: string;


One option to try to enforce mutual exclusivity is to use never:

type WalletData = { walletId: string; wallet_id?: never; } | { walletId?: never; wallet_id: string; } const walletData: WalletData = { walletId: '123', wallet_id: '123' }

Now TypeScript will error with a message:

Type '{ walletId: string; wallet_id: string; }' is not assignable to type 'WalletData'. Types of property 'wallet_id' are incompatible. Type 'string' is not assignable to type 'undefined'

Admittedly, the error message could be clearer, but it's difficult to customize the message without reaching for a type validation library like Zod, which might be overkill here.

0xRAG · 2024-12-19T18:24:37Z

src/coinbase/types.ts

 */
-export type WalletData = {
-  walletId: string;
+export interface WalletData {


Curious on reasoning to convert this from a type to an interface?

0xRAG · 2024-12-19T18:36:29Z

src/coinbase/wallet.ts

+
+      // Convert mnemonic phrase to seed
+      const seedBuffer = mnemonicToSeedSync(data.mnemonicPhrase);
+      const seed = hexlify(seedBuffer).slice(2); // remove 0x prefix


Probably out-of-scope for this PR, but I would love to switch to viem and/or Ox.

That said, we do already have viem installed as a dependency, so we could use toHex instead!

0xRAG · 2024-12-19T18:37:38Z

src/coinbase/wallet.ts

+
+      // Create wallet using the provided seed
+      const wallet = await Wallet.createWithSeed({
+        seed: seed,


Nit: Shorthand syntax can be used here (this should be enforced ideally by the linter / formatter, I'll make a note of it)

0xRAG · 2024-12-19T18:40:44Z

src/coinbase/wallet.ts

+      // Create wallet using the provided seed
+      const wallet = await Wallet.createWithSeed({
+        seed: seed,
+        networkId: Coinbase.networks.BaseSepolia,


Should networkId be parameterized?

0xRAG · 2024-12-19T18:45:09Z

src/coinbase/wallet.ts

+   *
+   * @param options - The options to create the Wallet.
+   * @param options.seed - The seed to use for the Wallet. If undefined, a random seed will be generated.
+   * @param options.networkId - the ID of the blockchain network. Defaults to 'base-sepolia'.


Nit: the is lowercased here, but uppercased in the other strings

0xRAG

Left some comments & nits, but nothing blocking. 🚢

alex-stone · 2024-12-19T21:34:01Z

CHANGELOG.md

+- Deprecate `Wallet.loadSeed()` method in favor of `Wallet.loadSeedFromFile()`
+- Deprecate `Wallet.saveSeed()` method in favor of `Wallet.saveSeedToFile()`


We'll move these around as we prep the release, but we'll have a separate heading for "Address" and "Deprecated"

alex-stone · 2024-12-19T21:38:32Z

src/coinbase/wallet.ts

@@ -130,31 +136,67 @@ export class Wallet {
  }

  /**
-   * Imports a Wallet for the given Wallet data.
+   * Loads an existing CDP Wallet using a wallet data object or mnemonic seed phrase.


Can we document that it creates a new wallet for mnemonic seed phrases?

Or how can we best clarify that this leaves the existing behavior + adds new behavior, and lays the groundwork for the breaking change in the next release?

Not a blocker per se, but we should aim to make this clear in our public docs + here when we make the cutover

Yes great point. FYI this didn't make it into this PR, so will add a comment within our breaking changes to-list just to track

alex-stone

Looks good, I think having a big rev on the docs around wallet import v.s. wallet load, etc... is going to be important in the next 2 releases

derek-cb added 2 commits December 17, 2024 12:59

Implementing test cases. Lowered jest branch coverage criteria slightly

603df2d

derek-cb mentioned this pull request Dec 17, 2024

feat(PSDK-670): Support external wallet imports, wallet imports from CDP Python SDK #345

Closed

derek-cb marked this pull request as ready for review December 17, 2024 22:18

derek-cb requested review from John-peterson-coinbase and alex-stone December 17, 2024 22:18

Committing more unit tests

3087585

John-peterson-coinbase reviewed Dec 18, 2024

View reviewed changes

src/coinbase/wallet.ts Outdated Show resolved Hide resolved

derek-cb added 5 commits December 18, 2024 10:32

Address merge conflicts and add features to changelog

4b94116

# Conflicts: # src/coinbase/types.ts # src/coinbase/wallet.ts

Address merge conflicts and add features to changelog

6d6da23

# Conflicts: # src/coinbase/types.ts # src/coinbase/wallet.ts

Minor changelog edit

d8de91c

Merge branch 'feat/external-wallet-import' of https://github.com/coin…

fc7e978

…base/coinbase-sdk-nodejs into feat/external-wallet-import # Conflicts: # CHANGELOG.md

derek-cb requested a review from John-peterson-coinbase December 18, 2024 19:02

derek-cb added 6 commits December 18, 2024 11:14

Add retry logic to address.listTransactions() E2E test for better res…

026f9dd

…iliency

Added console log to E2E test for better debugging

3496294

Add cross-SDK compatibility for WalletData's new networkId field

283c81f

Merge branch 'v0.13.0' into feat/external-wallet-import

b0e08ab

Ensure optionality of fields within WalletData during wallet export

460927b

Minor word consistency improvement to changelog

5b1fdbc

0xRAG reviewed Dec 19, 2024

View reviewed changes

0xRAG approved these changes Dec 19, 2024

View reviewed changes

alex-stone reviewed Dec 19, 2024

View reviewed changes

alex-stone approved these changes Dec 19, 2024

View reviewed changes

alex-stone merged commit ac9d252 into v0.13.0 Dec 19, 2024
4 checks passed

alex-stone deleted the feat/external-wallet-import branch December 19, 2024 21:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(PSDK-670): Support external wallet imports, wallet imports from CDP Python SDK #347

feat(PSDK-670): Support external wallet imports, wallet imports from CDP Python SDK #347

derek-cb commented Dec 17, 2024 •

edited

Loading

0xRAG Dec 19, 2024

0xRAG Dec 19, 2024

0xRAG Dec 19, 2024

0xRAG Dec 19, 2024

0xRAG Dec 19, 2024

0xRAG Dec 19, 2024

0xRAG left a comment

alex-stone Dec 19, 2024

alex-stone Dec 19, 2024

derek-cb Dec 19, 2024

alex-stone left a comment

		- Deprecate `Wallet.loadSeed()` method in favor of `Wallet.loadSeedFromFile()`
		- Deprecate `Wallet.saveSeed()` method in favor of `Wallet.saveSeedToFile()`

feat(PSDK-670): Support external wallet imports, wallet imports from CDP Python SDK #347

feat(PSDK-670): Support external wallet imports, wallet imports from CDP Python SDK #347

Conversation

derek-cb commented Dec 17, 2024 • edited Loading

What changed? Why?

Testing

Qualified Impact

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xRAG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-stone left a comment

Choose a reason for hiding this comment

derek-cb commented Dec 17, 2024 •

edited

Loading