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.

Sign up for GitHub

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

Jump to bottom

[PSDK-111] Wallet Import/Export/Save/Load #23

Merged
merged 1 commit into from
May 22, 2024
Merged

[PSDK-111] Wallet Import/Export/Save/Load #23

merged 1 commit into from
May 22, 2024

Conversation

John-peterson-coinbase
Copy link
Contributor

What changed? Why?

Added the following methods + tests:

  • User.saveWallet
  • User.loadWallets
  • User.importWallet
  • Wallet.export()

Qualified Impact

@John-peterson-coinbase John-peterson-coinbase force-pushed the psdk-111-wallet-import-export-store branch from 3cc7875 to c0699ba Compare May 22, 2024 06:44
@John-peterson-coinbase John-peterson-coinbase force-pushed the psdk-111-wallet-import-export-store branch 2 times, most recently from ec16e94 to f2c323e Compare May 22, 2024 07:44
erdimaden
erdimaden reviewed May 22, 2024
View reviewed changes
src/coinbase/tests/user_test.ts Outdated Show resolved Hide resolved
src/coinbase/tests/user_test.ts Outdated Show resolved Hide resolved
src/coinbase/tests/user_test.ts
expect(walletSeedData[walletId].authTag).toBeTruthy();
expect(walletSeedData[walletId].seed).not.toBe(seed);
});

Copy link
Contributor

Choose a reason for hiding this comment

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

test: Seed does not match wallet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seed is a private member so this test is not necessary on Node.js SDK. Ruby allows seed to be a public setter (Consider changing this)

src/coinbase/tests/user_test.ts Outdated Show resolved Hide resolved
src/coinbase/tests/wallet_test.ts Outdated Show resolved Hide resolved
@John-peterson-coinbase John-peterson-coinbase force-pushed the psdk-111-wallet-import-export-store branch from f2c323e to ccdc856 Compare May 22, 2024 16:56
@John-peterson-coinbase John-peterson-coinbase force-pushed the psdk-111-wallet-import-export-store branch from ccdc856 to 8f717d7 Compare May 22, 2024 17:34
@John-peterson-coinbase John-peterson-coinbase merged commit 055c8dd into master May 22, 2024
6 checks passed
@alex-stone
Copy link
Contributor

@John-peterson-coinbase I see a user.importWallet but no Wallet.import method.

The addressCount is also a bit weird since we're calling ListAddresses and then using that for a count, passing that in and then re-deriving and calling GetAddress on each of the individual addresses.

https://github.com/coinbase/coinbase-sdk-ruby/blob/6030264be58b1bcfa4a7d93fed1609a25286cc4c/lib/coinbase/wallet.rb#L24-L35

@John-peterson-coinbase
Copy link
Contributor Author

John-peterson-coinbase commented May 22, 2024 •
edited
Loading

@John-peterson-coinbase I see a user.importWallet but no Wallet.import method.

The addressCount is also a bit weird since we're calling ListAddresses and then using that for a count, passing that in and then re-deriving and calling GetAddress on each of the individual addresses.

https://github.com/coinbase/coinbase-sdk-ruby/blob/6030264be58b1bcfa4a7d93fed1609a25286cc4c/lib/coinbase/wallet.rb#L24-L35

+1 this implementation was based off of the previous version of ruby sdk where this logic is closer to parity. We will flag a follow up to update the wallet import logic to more closely track the v0.0.5 release of Ruby SDK

cc: @erdimaden @alex-stone

@erdimaden erdimaden deleted the psdk-111-wallet-import-export-store branch June 21, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erdimaden erdimaden erdimaden approved these changes

@alex-stone alex-stone Awaiting requested review from alex-stone

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@John-peterson-coinbase @alex-stone @erdimaden