Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 15 additions & 4 deletions modules/sdk-core/src/bitgo/address-book/address-book.ts

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated!

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 ?? {})

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated!

.result();
}

/**
Expand Down Expand Up @@ -84,7 +88,6 @@ export class AddressBook implements IAddressBook {
const response: GetAddressBookListingResponse = await this.bitgo
.get(url)
.set('enterprise-id', this.enterpriseId)
.send()
.result();
this._listing = response;
return this.listing() as AddressBookListing;
Expand Down Expand Up @@ -125,7 +128,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 +142,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
156 changes: 156 additions & 0 deletions modules/sdk-core/test/unit/bitgo/address-book/address-book.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
import * as sinon from 'sinon';
import * as superagent from 'superagent';
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 });

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');

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated!

return { setStub, queryStub };
}

function makeParameterlessGetStub(response: Record<string, unknown> = {}) {
const resultStub = sinon.stub().resolves(response);
const setStub = sinon.stub().returns({ result: resultStub });
mockBitGo.get.returns({ set: setStub });
return { setStub, resultStub };
}

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.calledWith(mockBitGo.get, 'https://app.bitgo.com/api/address-book/v1/connections');
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.calledWith(mockBitGo.get, 'https://app.bitgo.com/api/address-book/v1/connections');
sinon.assert.calledOnce(queryStub);
sinon.assert.calledWith(queryStub, {});
});

it('should pass array filters to query unchanged', async function () {
const { queryStub } = makeGetStub();
const params = {
ownerWalletId: ['wallet-a', 'wallet-b'],
targetWalletId: ['wallet-c'],
};

await addressBook.getConnections(params);

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

describe('getConnections array query param serialization', function () {
it('superagent serializes string[] as repeated keys, which address-book accepts via nonEmptyArrayFromQueryParam', function () {
const req = superagent.get('https://example.com').query({
ownerWalletId: ['wallet-a', 'wallet-b'],
targetWalletId: ['wallet-c'],
});
// Trigger superagent's query-string assembly without sending the request.
req.end(() => undefined);

req.url!.should.equal(
'https://example.com?ownerWalletId=wallet-a&ownerWalletId=wallet-b&targetWalletId=wallet-c'
);
});
});

describe('getListing', function () {
it('should use GET with enterprise-id header and no query or body', async function () {
const listing = {
id: 'listing-id',
enterpriseId,
name: 'Test Listing',
owner: 'owner',
createdAt: '2024-01-01',
updatedAt: '2024-01-01',
};
const { setStub, resultStub } = makeParameterlessGetStub(listing);

const result = await addressBook.getListing();

sinon.assert.calledWith(mockBitGo.get, 'https://app.bitgo.com/api/address-book/v1/listing/global');
sinon.assert.calledOnce(setStub);
sinon.assert.calledWith(setStub, 'enterprise-id', enterpriseId);
sinon.assert.calledOnce(resultStub);
result.should.deepEqual(listing);
});
});

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.calledWith(mockBitGo.get, 'https://app.bitgo.com/api/address-book/v1/listing/entry/contacts');
sinon.assert.calledOnce(queryStub);
sinon.assert.calledWith(queryStub, params);
});

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

await addressBook.getListingEntryContacts();

sinon.assert.calledWith(mockBitGo.get, 'https://app.bitgo.com/api/address-book/v1/listing/entry/contacts');
sinon.assert.calledOnce(queryStub);
sinon.assert.calledWith(queryStub, {});
});
});

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, {});
});

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated!


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.calledWith(mockBitGo.get, 'https://app.bitgo.com/api/address-book/v1/listing/entry/directory');
sinon.assert.calledOnce(queryStub);
sinon.assert.calledWith(queryStub, params);
});

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

await addressBook.getListingEntryDirectory();

sinon.assert.calledWith(mockBitGo.get, 'https://app.bitgo.com/api/address-book/v1/listing/entry/directory');
sinon.assert.calledOnce(queryStub);
sinon.assert.calledWith(queryStub, {});
});
});
});
Loading