Skip to content

fix: use query params for GET address-book methods #9084

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

Open
seanchen0818 wants to merge 1 commit into
base: master
Choose a base branch
from
+88 −3
Open

fix: use query params for GET address-book methods #9084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions modules/sdk-core/src/bitgo/address-book/address-book.ts
View file
Open in desktop

@lokesh-bitgo lokesh-bitgo Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit (follow-up): getListing() (line ~82) also calls .send() on a GET with no arguments — same anti-pattern as the methods fixed here. Less harmful since there are no params to drop, but it's inconsistent in the same class. Consider cleaning it up in a follow-up: just remove the .send() call entirely since .get().set(...).result() is sufficient for a parameterless GET.

Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,11 @@ export class AddressBook implements IAddressBook {
*/
getConnections(params?: GetAddressBookConnectionsParams): Promise<GetAddressBookConnectionsResponse> {
const url = this.bitgo.microservicesUrl('/api/address-book/v1/connections');
return this.bitgo.get(url).set('enterprise-id', this.enterpriseId).send(params).result();
return this.bitgo
.get(url)
.set('enterprise-id', this.enterpriseId)
.query(params ?? {})

@lokesh-bitgo lokesh-bitgo Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: GetAddressBookConnectionsParams includes array fields (ownerWalletId?: string[], targetWalletId?: string[]). Superagent's default .query() serializes arrays as repeated keys — ?ownerWalletId=a&ownerWalletId=b. Worth verifying the address-book service parses that format (vs bracket notation ownerWalletId[]=a or comma-separated). If the format doesn't match, these filters would silently not work.

.result();
}

/**
Expand Down Expand Up @@ -125,7 +129,11 @@ export class AddressBook implements IAddressBook {
params?: GetAddressBookListingEntryContactsParams
): Promise<GetAddressBookListingEntryContactsResponse> {
const url = this.bitgo.microservicesUrl('/api/address-book/v1/listing/entry/contacts');
return this.bitgo.get(url).set('enterprise-id', this.enterpriseId).send(params).result();
return this.bitgo
.get(url)
.set('enterprise-id', this.enterpriseId)
.query(params ?? {})
.result();
}

/**
Expand All @@ -135,7 +143,11 @@ export class AddressBook implements IAddressBook {
params?: GetAddressBookListingEntryDirectoryParams
): Promise<GetAddressBookListingEntryDirectoryResponse> {
const url = this.bitgo.microservicesUrl('/api/address-book/v1/listing/entry/directory');
return this.bitgo.get(url).set('enterprise-id', this.enterpriseId).send(params).result();
return this.bitgo
.get(url)
.set('enterprise-id', this.enterpriseId)
.query(params ?? {})
.result();
}

/**
Expand Down
73 changes: 73 additions & 0 deletions modules/sdk-core/test/unit/bitgo/address-book/address-book.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import * as sinon from 'sinon';
import 'should';
import { AddressBook } from '../../../../src/bitgo/address-book/address-book';

describe('AddressBook', function () {
let addressBook: AddressBook;
let mockBitGo: any;
const enterpriseId = 'test-enterprise-id';

function makeGetStub() {
const queryStub = sinon.stub().returns({ result: sinon.stub().resolves({}) });
const setStub = sinon.stub().returns({ query: queryStub });
mockBitGo.get.returns({ set: setStub });

@lokesh-bitgo lokesh-bitgo Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Low: mockBitGo.get is stubbed to accept any URL — a typo in the endpoint path (e.g. /connections → /connection) would go undetected. Consider asserting the URL too:

sinon.assert.calledWith(mockBitGo.get, 'https://app.bitgo.com/api/address-book/v1/connections');

return { setStub, queryStub };
}

beforeEach(function () {
mockBitGo = {
get: sinon.stub(),
microservicesUrl: sinon.stub().callsFake((path: string) => `https://app.bitgo.com${path}`),
};
addressBook = new AddressBook(enterpriseId, mockBitGo);
});

afterEach(function () {
sinon.restore();
});

describe('getConnections', function () {
it('should pass params as query string, not request body', async function () {
const { queryStub } = makeGetStub();
const params = { connectionType: 'DVP' as const, status: 'INACTIVE' as const, offset: 0, limit: 10 };

await addressBook.getConnections(params);

sinon.assert.calledOnce(queryStub);
sinon.assert.calledWith(queryStub, params);
});

it('should work with no params', async function () {
const { queryStub } = makeGetStub();

await addressBook.getConnections();

sinon.assert.calledOnce(queryStub);
sinon.assert.calledWith(queryStub, {});
});
});

describe('getListingEntryContacts', function () {
it('should pass params as query string, not request body', async function () {
const { queryStub } = makeGetStub();
const params = { status: 'ACTIVE' as const, limit: 5 };

await addressBook.getListingEntryContacts(params);

sinon.assert.calledOnce(queryStub);
sinon.assert.calledWith(queryStub, params);
});
});

@lokesh-bitgo lokesh-bitgo Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Low: getConnections has a no-params test verifying undefined → {} (the ?? {} path), but getListingEntryContacts and getListingEntryDirectory only have one test each (with params). Consider adding a no-params case for both:

it('should work with no params', async function () {
  const { queryStub } = makeGetStub();
  await addressBook.getListingEntryContacts();
  sinon.assert.calledWith(queryStub, {});
});


describe('getListingEntryDirectory', function () {
it('should pass params as query string, not request body', async function () {
const { queryStub } = makeGetStub();
const params = { status: 'ACTIVE' as const, limit: 5 };

await addressBook.getListingEntryDirectory(params);

sinon.assert.calledOnce(queryStub);
sinon.assert.calledWith(queryStub, params);
});
});
});
Loading