search

coinbase / coinbase-sdk-nodejs

Other
74 stars 28 forks source link

Update getDefaultAddress and getAddress to fetch addresses if not loaded #217

Closed rohan-agarwal-coinbase closed 2 months ago

rohan-agarwal-coinbase commented 2 months ago

What changed? Why?

Updates getDefaultAddress and getAddress to fetch addresses if it is not loaded/cached, which occurs right when creating a new wallet.

This PR tightens the interfaces for both functions. getDefaultAddress no longer returns undefined and throws an error. getAddress now returns a Promise<WalletAddress> instead of Address since it calls listAddresses. This is backward-incompatible

Testing

Updated unit tests. I also wrote and ran a test script locally that succeeds to get the default address after fetching a wallet, which was earlier failing.

Qualified Impact

Getting addresses has changed with new side effects, and the interfaces have changed in an backward-incompatible way so fixing issues would require client-side changes.

cb-heimdall commented 2 months ago

🟡 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
John-peterson-coinbase commented 2 months ago

Since this is a breaking change, we will need to update README.md examples, the quickstarts in ./quickstart-template, and CDP docs

rohan-agarwal-coinbase commented 2 months ago

Since this is a breaking change, we will need to update README.md examples, the quickstarts in ./quickstart-template, and CDP docs

thanks @John-peterson-coinbase i updated the PR:

Will update the CDP docs next