Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABC5695d76dcf90: [Cashtab][Alias] Update alias-server wrappers to return JSON

Summary

As per feedback from D14212, the wrapper for the alias-server endpoints should be returning the JSON responses directly rather than leaving it to frontend components to parse.

Test Plan

npm test

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

emack created this revision.Jul 6 2023, 07:28

Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 6 2023, 07:28

emack requested review of this revision.Jul 6 2023, 07:28

Harbormaster completed remote builds in B24377: Diff 41284.Jul 6 2023, 07:31

emack mentioned this in D14212: [Cashtab][Alias] Update Send.js to use api endpoint in place of getAddressFromAlias.Jul 6 2023, 07:33

Fabien added a subscriber: Fabien.Jul 6 2023, 14:32

Fabien added inline comments.

cashtab/src/utils/aliasUtils.js
16 ↗	(On Diff #41284)	why are these variables declared at this scope ?
21 ↗	(On Diff #41284)	Is !aliasServerResp possible ?
27 ↗	(On Diff #41284)	I think `return await aliasServerResp.json();` does the same and removes one variable
91 ↗	(On Diff #41284)	same questions/remarks

improved error handling for when alias-server is non-responsive, improved code efficient per feedback

Harbormaster completed remote builds in B24384: Diff 41291.Jul 6 2023, 16:08

Fabien added inline comments.Jul 6 2023, 16:48

cashtab/src/utils/aliasUtils.js
80 ↗	(On Diff #41291)	Interesting how this method looks similar to the above one, right ? It's like only the endpoint differs.

Consolidated getAliasDetails and getAliasesForAddress into a single alias-server wrapper, passing in the endpoint and query param as inputs. Unit tests updated accordingly.

emack edited the summary of this revision. (Show Details)Jul 7 2023, 00:50

Harbormaster completed remote builds in B24398: Diff 41308.Jul 7 2023, 00:52

One nit, otherwise looks good to me.

cashtab/src/utils/aliasUtils.js
45 ↗	(On Diff #41308)	Nit: the function name needs to be updated. Also it would be good to print the endpoint as well, it will be useful for debugging.