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.

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

[TRA-102] add subaccountNumber field in assetPositions return result #1187

Merged
merged 3 commits into from
Mar 19, 2024
Merged

[TRA-102] add subaccountNumber field in assetPositions return result #1187

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
98ab835
add subaccountNumber field in assetPositions return result
shrenujb Mar 16, 2024
6ecc564
fix lint and tests
shrenujb Mar 16, 2024
e04918b
address comments
shrenujb Mar 18, 2024
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
4 changes: 4 additions & 0 deletions indexer/services/comlink/__tests__/controllers/api/v4/asset-positions-controller.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ describe('asset-positions-controller#V4', () => {
side: PositionSide.LONG,
size,
assetId: testConstants.defaultAssetPosition.assetId,
subaccountNumber: testConstants.defaultSubaccount.subaccountNumber,
};

expect(response.body.positions).toEqual(
Expand Down Expand Up @@ -75,6 +76,7 @@ describe('asset-positions-controller#V4', () => {
side: PositionSide.SHORT,
size: testConstants.defaultAssetPosition.size,
assetId: testConstants.defaultAssetPosition.assetId,
subaccountNumber: testConstants.defaultSubaccount.subaccountNumber,
};

expect(response.body.positions).toEqual(
Expand Down Expand Up @@ -110,6 +112,7 @@ describe('asset-positions-controller#V4', () => {
size: testConstants.defaultAssetPosition.size,
side: PositionSide.LONG,
assetId: testConstants.defaultAssetPosition.assetId,
subaccountNumber: testConstants.defaultSubaccount.subaccountNumber,
}],
);
});
Expand Down Expand Up @@ -154,6 +157,7 @@ describe('asset-positions-controller#V4', () => {
size: '9500',
side: PositionSide.LONG,
assetId: testConstants.defaultAssetPosition.assetId,
subaccountNumber: testConstants.defaultSubaccount.subaccountNumber,
}],
);
});
Expand Down
4 changes: 4 additions & 0 deletions indexer/services/comlink/__tests__/lib/helpers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,7 @@ describe('helpers', () => {
side: PositionSide.LONG,
assetId: '0',
size: '1',
subaccountNumber: 0,
},
};
const unsettledFunding: Big = Big('300');
Expand Down Expand Up @@ -458,6 +459,7 @@ describe('helpers', () => {
side: PositionSide.LONG,
assetId: '0',
size: '1',
subaccountNumber: 0,
},
};

Expand Down Expand Up @@ -513,6 +515,7 @@ describe('helpers', () => {
side: PositionSide.LONG,
assetId: '0',
size: '1',
subaccountNumber: 0,
},
};

Expand Down Expand Up @@ -569,6 +572,7 @@ describe('helpers', () => {
side: PositionSide.LONG,
assetId: '0',
size: '1',
subaccountNumber: 0,
},
};

Expand Down
1 change: 1 addition & 0 deletions indexer/services/comlink/__tests__/lib/validation/schemas.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ describe('schemas', () => {
const positiveNonInteger: number = 3.2;
const negativeInteger: number = -1;
const zeroInteger: number = 0;
const stringNumber: any = '3';
const defaultSubaccountNumber: number = testConstants.defaultSubaccount.subaccountNumber;
const defaultAddress: string = testConstants.defaultSubaccount.address;
describe('CheckSubaccountSchema', () => {
Expand Down
46 changes: 30 additions & 16 deletions indexer/services/comlink/public/api-documentation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,13 +110,15 @@ fetch('https://dydx-testnet.imperator.co/v4/addresses/{address}',
"symbol": "string",
"side": "LONG",
"size": "string",
"assetId": "string"
"assetId": "string",
"subaccountNumber": 0
},
"property2": {
"symbol": "string",
"side": "LONG",
"size": "string",
"assetId": "string"
"assetId": "string",
"subaccountNumber": 0
}
},
"marginEnabled": true
Expand Down Expand Up @@ -234,13 +236,15 @@ fetch('https://dydx-testnet.imperator.co/v4/addresses/{address}/subaccountNumber
"symbol": "string",
"side": "LONG",
"size": "string",
"assetId": "string"
"assetId": "string",
"subaccountNumber": 0
},
"property2": {
"symbol": "string",
"side": "LONG",
"size": "string",
"assetId": "string"
"assetId": "string",
"subaccountNumber": 0
}
},
"marginEnabled": true
Expand Down Expand Up @@ -269,7 +273,7 @@ headers = {
'Accept': 'application/json'
}

r = requests.get('https://dydx-testnet.imperator.co/v4/assetPositions', params={
r = requests.get('https://dydx-testnet.imperator.co/v4/assetPositions/{address}/{subaccountNumber}', params={
'address': 'string', 'subaccountNumber': '0'
}, headers = headers)

Expand All @@ -283,7 +287,7 @@ const headers = {
'Accept':'application/json'
};

fetch('https://dydx-testnet.imperator.co/v4/assetPositions?address=string&subaccountNumber=0',
fetch('https://dydx-testnet.imperator.co/v4/assetPositions/{address}/{subaccountNumber}?address=string&subaccountNumber=0',
{
method: 'GET',

Expand All @@ -297,7 +301,7 @@ fetch('https://dydx-testnet.imperator.co/v4/assetPositions?address=string&subacc

```

`GET /assetPositions`
`GET /assetPositions/{address}/{subaccountNumber}`

### Parameters

Expand All @@ -317,7 +321,8 @@ fetch('https://dydx-testnet.imperator.co/v4/assetPositions?address=string&subacc
"symbol": "string",
"side": "LONG",
"size": "string",
"assetId": "string"
"assetId": "string",
"subaccountNumber": 0
}
]
}
Expand Down Expand Up @@ -2036,7 +2041,8 @@ This operation does not require authentication
"symbol": "string",
"side": "LONG",
"size": "string",
"assetId": "string"
"assetId": "string",
"subaccountNumber": 0
}

```
Expand All @@ -2049,6 +2055,7 @@ This operation does not require authentication
|side|[PositionSide](#schemapositionside)|true|none|none|
|size|string|true|none|none|
|assetId|string|true|none|none|
|subaccountNumber|number(double)|true|none|none|

## AssetPositionsMap

Expand All @@ -2063,13 +2070,15 @@ This operation does not require authentication
"symbol": "string",
"side": "LONG",
"size": "string",
"assetId": "string"
"assetId": "string",
"subaccountNumber": 0
},
"property2": {
"symbol": "string",
"side": "LONG",
"size": "string",
"assetId": "string"
"assetId": "string",
"subaccountNumber": 0
}
}

Expand Down Expand Up @@ -2135,13 +2144,15 @@ This operation does not require authentication
"symbol": "string",
"side": "LONG",
"size": "string",
"assetId": "string"
"assetId": "string",
"subaccountNumber": 0
},
"property2": {
"symbol": "string",
"side": "LONG",
"size": "string",
"assetId": "string"
"assetId": "string",
"subaccountNumber": 0
}
},
"marginEnabled": true
Expand Down Expand Up @@ -2217,13 +2228,15 @@ This operation does not require authentication
"symbol": "string",
"side": "LONG",
"size": "string",
"assetId": "string"
"assetId": "string",
"subaccountNumber": 0
},
"property2": {
"symbol": "string",
"side": "LONG",
"size": "string",
"assetId": "string"
"assetId": "string",
"subaccountNumber": 0
}
},
"marginEnabled": true
Expand Down Expand Up @@ -2255,7 +2268,8 @@ This operation does not require authentication
"symbol": "string",
"side": "LONG",
"size": "string",
"assetId": "string"
"assetId": "string",
"subaccountNumber": 0
}
]
}
Expand Down
9 changes: 7 additions & 2 deletions indexer/services/comlink/public/swagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,13 +116,18 @@
},
"assetId": {
"type": "string"
},
"subaccountNumber": {
"type": "number",
"format": "double"
}
},
"required": [
"symbol",
"side",
"size",
"assetId"
"assetId",
"subaccountNumber"
],
"type": "object",
"additionalProperties": false
Expand Down Expand Up @@ -1203,7 +1208,7 @@
]
}
},
"/assetPositions": {
"/assetPositions/{address}/{subaccountNumber}": {
"get": {
"operationId": "GetAssetPositions",
"responses": {
Expand Down
1 change: 1 addition & 0 deletions indexer/services/comlink/src/controllers/api/v4/addresses-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,7 @@ async function getSubaccountResponse(
return assetPositionToResponseObject(
assetPosition,
assetIdToAsset,
subaccount.subaccountNumber,
);
},
);
Expand Down
17 changes: 11 additions & 6 deletions indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { stats } from '@dydxprotocol-indexer/base';
import {parseNumber, stats} from '@dydxprotocol-indexer/base';
import {
AssetColumns,
AssetFromDatabase,
Expand Down Expand Up @@ -52,8 +52,8 @@ const router: express.Router = express.Router();
const controllerName: string = 'asset-positions-controller';

@Route('assetPositions')
class AddressesController extends Controller {
@Get('/')
class AssetPositionsController extends Controller {
@Get('/:address/:subaccountNumber')
async getAssetPositions(
shrenujb marked this conversation as resolved.
Show resolved Hide resolved
@Query() address: string,
@Query() subaccountNumber: number,
Expand Down Expand Up @@ -108,7 +108,9 @@ class AddressesController extends Controller {

let assetPositionsMap: AssetPositionsMap = _.chain(sortedAssetPositions)
.map(
(position: AssetPositionFromDatabase) => assetPositionToResponseObject(position, idToAsset),
(position: AssetPositionFromDatabase) => assetPositionToResponseObject(position,
idToAsset,
subaccountNumber),
).keyBy(
(positionResponse: AssetPositionResponseObject) => positionResponse.symbol,
).value();
Expand Down Expand Up @@ -159,11 +161,14 @@ router.get(
subaccountNumber,
}: AssetPositionRequest = matchedData(req) as AssetPositionRequest;

// The schema checks allow subaccountNumber to be a string, but we know it's a number here.
const subaccountNum : number = +subaccountNumber;
vincentwschau marked this conversation as resolved.
Show resolved Hide resolved

try {
const controller: AddressesController = new AddressesController();
const controller: AssetPositionsController = new AssetPositionsController();
const response: AssetPositionResponse = await controller.getAssetPositions(
address,
subaccountNumber,
subaccountNum,
);
Comment on lines +164 to +171
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The script output indicates that subaccountNumber is directly converted to a number without explicit error handling or range checking in the provided code snippets. This could potentially lead to issues if subaccountNumber is not a valid number. Therefore, the review comment highlighting the need for proper error handling and range checking for subaccountNumber is justified based on the available evidence.

Analysis chain

The handling of subaccountNumber as a number, including its parsing and usage in getAssetPositions, is crucial for the new functionality. Ensure that there's proper error handling for parsing and that subaccountNumber falls within expected ranges or values.

Scripts Executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify that subaccountNumber parsing handles errors and checks for valid ranges
grep -r "parseNumber" src/controllers/api/v4/asset-positions-controller.ts | grep -v "//"

Length of output: 173

Script:

#!/bin/bash
# Find the correct path for asset-positions-controller.ts
fd asset-positions-controller.ts

Length of output: 112

Script:

#!/bin/bash
# Inspect handling of subaccountNumber in asset-positions-controller.ts for error handling and range checking
grep -C 3 "subaccountNumber" indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts

Length of output: 1200


return res.send(response);
Expand Down
1 change: 1 addition & 0 deletions indexer/services/comlink/src/lib/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export const ZERO_USDC_POSITION: AssetPositionResponseObject = {
symbol: USDC_SYMBOL,
side: PositionSide.LONG,
assetId: USDC_ASSET_ID,
subaccountNumber: 0,
};

export const SPARKLINE_TIME_PERIOD_TO_LIMIT_MAP:
Expand Down
2 changes: 2 additions & 0 deletions indexer/services/comlink/src/request-helpers/request-transformer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,13 +115,15 @@ export function perpetualPositionToResponseObject(
export function assetPositionToResponseObject(
position: AssetPositionFromDatabase,
assetMap: AssetById,
subaccountNumber: number,
): AssetPositionResponseObject {

return {
symbol: assetMap[position.assetId].symbol,
side: position.isLong ? PositionSide.LONG : PositionSide.SHORT,
size: position.size,
assetId: position.assetId,
subaccountNumber,
};
}

Expand Down
1 change: 1 addition & 0 deletions indexer/services/comlink/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ export interface AssetPositionResponseObject {
side: PositionSide;
size: string;
assetId: string;
subaccountNumber: number;
}

export type AssetPositionsMap = { [symbol: string]: AssetPositionResponseObject };
Expand Down
Loading