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

Implementing getWallet functionality #27

Merged
merged 6 commits into from
May 23, 2024
Merged

Implementing getWallet functionality #27

merged 6 commits into from
May 23, 2024

Conversation

erdimaden
Copy link
Contributor

@erdimaden erdimaden commented May 23, 2024

What changed? Why?

  • Removed addressCount parameter and adding addressModels
  • Updating deriveAddress method

Qualified Impact

@erdimaden erdimaden changed the title [WIP] Adding getWallet functionality Implementing getWallet functionality May 23, 2024
@erdimaden erdimaden marked this pull request as ready for review May 23, 2024 20:08
walletsModels.push(wallet);
}
for (const wallet of walletsModels) {
const addressList = await Coinbase.apiClients.address!.listAddresses(
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we fetching these all synchronously?

Curious if we can easily fetches these concurrently, and then wait for all the responses before stitching them back together?

Not blocking, but this is much easier in JS than in ruby to do (and easiest in golang 😂).

Copy link
Contributor

@alex-stone alex-stone left a comment

Choose a reason for hiding this comment

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

Looks good, had one performance-related question. Non-blocking, but would be nice to have a ticket to implement later!

@erdimaden erdimaden merged commit 3f64c65 into master May 23, 2024
6 checks passed
@erdimaden erdimaden deleted the feat/getWallets branch June 21, 2024 18:04
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.

2 participants