Skip to content
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(PSDK-670): Support external wallet imports, wallet imports from CDP Python SDK #68

Merged
merged 7 commits into from
Dec 19, 2024

Conversation

derek-cb
Copy link
Contributor

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
  • Deprecate Wallet.load_seed method in favor of Wallet.load_seed_from_file
  • Deprecate Wallet.save_seed method in favor of Wallet.save_seed_to_file

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 NodeJS SDK v0.11.0

@cb-heimdall
Copy link

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/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

@derek-cb derek-cb changed the base branch from master to v0.13.0 December 19, 2024 00:15
@derek-cb
Copy link
Contributor Author

Next-up: test cases

@@ -19,24 +20,25 @@
from cdp.webhook import Webhook

__all__ = [
"__version__",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lint changes

def import_data(cls, data: WalletData) -> "Wallet":
"""Import a wallet from previously exported wallet data.
def import_data(cls, data: WalletData | MnemonicSeedPhrase) -> "Wallet":
"""Import a wallet from previously exported wallet data or a mnemonic seed phrase.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is importing a wallet from previously exported wallet data or "creating a new wallet" from a mnemonic seed phrase.

It feels like we should split out the "load" from the "create" flows here? Or is the intent to lazily create the wallet (if one does not exist)? In that world we'd need to be able to check if a wallet already exists when providing the Mnemonic Seed Phrase as well?

@derek-cb @John-peterson-coinbase what are y'alls thoughts here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Edit: See that this is a 2-step plan:
Move WalletData out of the import() method and into its own method (ie, load())

Seems reasonable, although I feel like import would make more sense than import_data for that behavior of taking a mnemonic seed phrase or seed hex, and creating a wallet?

Copy link
Contributor Author

@derek-cb derek-cb Dec 19, 2024

Choose a reason for hiding this comment

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

ah yes good point. while this is a two-step plan, as you mentioned it doesn't quite make sense in the context of the Python SDK since the existing method is called "import_data". i would propose:

  • renaming "import_data" to "import_wallet" ("import" is a reserved method name in Python)
  • create a new method called "import_data" for backwards compatibility (that we will remove in a future breaking release) that accepts only WalletData - not the mnemonic phrase - and simply calls "import_wallet".

@alex-stone what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems reasonable!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alex-stone committed now

@derek-cb derek-cb marked this pull request as ready for review December 19, 2024 21:54
@derek-cb derek-cb merged commit f1e9fbf into v0.13.0 Dec 19, 2024
7 checks passed
@derek-cb derek-cb mentioned this pull request Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants