feat(js-dapi-client): add contested resources query methods #2446
Conversation
WalkthroughThe changes introduce new functionality to the Dash Platform JavaScript client for interacting with contested resources and their vote states. Two new methods, Changes
Sequence DiagramsequenceDiagram
participant Client
participant PlatformMethodsFacade
participant gRPCTransport
participant ContestedResourcesService
Client->>PlatformMethodsFacade: getContestedResources()
PlatformMethodsFacade->>gRPCTransport: Create gRPC Request
gRPCTransport->>ContestedResourcesService: Send Request
ContestedResourcesService-->>gRPCTransport: Return Contested Resources
gRPCTransport-->>PlatformMethodsFacade: Process Response
PlatformMethodsFacade-->>Client: Return Contested Resources
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
packages/js-dapi-client/lib/methods/platform/getContestedResourceVoteState/GetContestedResourceVoteStateResponse.js (1)
35-37: Enhance error message with more contextThe error message could be more descriptive to help with debugging.
if ((typeof contestedResourceContenders === 'undefined' || contestedResourceContenders === null) && !proof) { - throw new InvalidResponseError('Contested Resource Contenders data is not defined'); + throw new InvalidResponseError('Contested Resource Contenders data is not defined and no proof was provided'); }packages/js-dapi-client/lib/methods/platform/getContestedResources/getContestedResourcesResponse.js (1)
6-11: Complete the JSDoc parameter documentationThe JSDoc is missing type information for several parameters.
/** - * @param {object} contestedResourceContenders - * @param contestedResources - * @param resultCase + * @param {Object} contestedResources - The contested resources data + * @param {number} resultCase - The result case identifier * @param {Metadata} metadata * @param {Proof} [proof] */packages/js-dapi-client/lib/methods/platform/getContestedResourceVoteState/getContestedResourceVoteStateFactory.js (1)
28-29: Fix typo in JSDoc return typeThere's a typo in the return type annotation.
- * @returns {Promise<gрetContestedResourceVoteStateResponse>} + * @returns {Promise<GetContestedResourceVoteStateResponse>}packages/js-dapi-client/test/unit/methods/platform/getContestedResources/GetContestedResourcesResponse.spec.js (3)
42-46: Add test cases for edge cases.The test setup looks good, but consider adding test cases for:
- Empty contested resource values
- Invalid/malformed contested resource values
- Maximum size contested resource values
58-63: Consider testing with multiple contested resources.Current test only verifies with empty string. Add test cases with multiple contested resources to ensure proper handling of arrays.
Line range hint
121-128: Add test for invalid metadata values.While testing for undefined metadata, also consider testing for invalid metadata values (negative heights, invalid timestamps, etc.).
packages/js-dapi-client/test/unit/methods/platform/getContestedResources/getContestedResourcesFactory.spec.js (2)
34-48: Consider using test data factories.Instead of hardcoding test values, consider creating test data factories for better maintainability and reusability.
- contractIdBuffer = Buffer.from('11c70af56a763b05943888fa3719ef56b3e826615fdda2d463c63f4034cb861c', 'hex'); - documentTypeName = 'domain'; - indexName = 'normalizedLabel'; + const testDataFactory = createTestDataFactory(); + ({ contractIdBuffer, documentTypeName, indexName } = testDataFactory.createBasicTestData());
83-124: Add test cases for pagination and filtering.Current test cases only cover basic scenarios. Add tests for:
- Different count values
- Different ordering
- Various index value combinations
- Edge cases in pagination
Also applies to: 126-178
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/js-dapi-client/lib/methods/platform/PlatformMethodsFacade.js(2 hunks)packages/js-dapi-client/lib/methods/platform/getContestedResourceVoteState/GetContestedResourceVoteStateResponse.js(1 hunks)packages/js-dapi-client/lib/methods/platform/getContestedResourceVoteState/getContestedResourceVoteStateFactory.js(1 hunks)packages/js-dapi-client/lib/methods/platform/getContestedResources/getContestedResourcesFactory.js(1 hunks)packages/js-dapi-client/lib/methods/platform/getContestedResources/getContestedResourcesResponse.js(1 hunks)packages/js-dapi-client/test/unit/methods/platform/getContestedResources/GetContestedResourcesResponse.spec.js(4 hunks)packages/js-dapi-client/test/unit/methods/platform/getContestedResources/getContestedResourcesFactory.spec.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (3)
packages/js-dapi-client/lib/methods/platform/getContestedResourceVoteState/GetContestedResourceVoteStateResponse.js (1)
28-29: Add version compatibility checkThe code assumes
proto.getV0()exists without validation. This could lead to runtime errors if an incompatible version is received.Add version check before accessing V0:
static createFromProto(proto) { + if (!proto.hasV0()) { + throw new InvalidResponseError('Unsupported response version'); + } const contestedResourceContenders = proto.getV0().getContestedResourceContenders();packages/js-dapi-client/lib/methods/platform/getContestedResourceVoteState/getContestedResourceVoteStateFactory.js (1)
66-86: Improve retry mechanismThe retry implementation has the same issues as in getContestedResourcesFactory.
Please apply the same improvements suggested for the retry mechanism in getContestedResourcesFactory.js.
packages/js-dapi-client/lib/methods/platform/PlatformMethodsFacade.js (1)
19-20: LGTM! Clean integration of new methods.The new methods are properly integrated following the established patterns in the codebase.
Also applies to: 47-48
| } | ||
|
|
||
| return new GetContestedResourcesResponse( | ||
| contestedResourceContenders !== undefined ? contestedResourceContenders.getContestedResourceValuesList() : '', |
There was a problem hiding this comment.
Add null check before accessing getContestedResourceValuesList
The code may throw if contestedResourceContenders is null but undefined check passes.
- contestedResourceContenders !== undefined ? contestedResourceContenders.getContestedResourceValuesList() : '',
+ contestedResourceContenders != null ? contestedResourceContenders.getContestedResourceValuesList() : '',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| contestedResourceContenders !== undefined ? contestedResourceContenders.getContestedResourceValuesList() : '', | |
| contestedResourceContenders != null ? contestedResourceContenders.getContestedResourceValuesList() : '', |
| if (Buffer.isBuffer(contractId)) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| contractId = Buffer.from(contractId); | ||
| } |
There was a problem hiding this comment.
Fix incorrect Buffer conversion logic
The current implementation creates a new Buffer from an existing Buffer unnecessarily, and the condition is reversed.
- if (Buffer.isBuffer(contractId)) {
+ if (!Buffer.isBuffer(contractId)) {
// eslint-disable-next-line no-param-reassign
contractId = Buffer.from(contractId);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (Buffer.isBuffer(contractId)) { | |
| // eslint-disable-next-line no-param-reassign | |
| contractId = Buffer.from(contractId); | |
| } | |
| if (!Buffer.isBuffer(contractId)) { | |
| // eslint-disable-next-line no-param-reassign | |
| contractId = Buffer.from(contractId); | |
| } |
| // TODO: simple retry before the dapi versioning is properly implemented | ||
| for (let i = 0; i < 3; i += 1) { | ||
| try { | ||
| // eslint-disable-next-line no-await-in-loop | ||
| const getContestedResourcesResponse = await grpcTransport.request( | ||
| PlatformPromiseClient, | ||
| 'getContestedResources', | ||
| getContestedResourcesRequest, | ||
| options, | ||
| ); | ||
|
|
||
| return GetContestedResourcesResponse | ||
| .createFromProto(getContestedResourcesResponse); | ||
| } catch (e) { | ||
| if (e instanceof InvalidResponseError) { | ||
| lastError = e; | ||
| } else { | ||
| throw e; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve retry mechanism
The retry implementation has several issues:
- Magic number for retry count
- No exponential backoff
- No delay between retries
Consider implementing a more robust retry mechanism:
+ const MAX_RETRIES = 3;
+ const BASE_DELAY = 1000; // 1 second
+
// TODO: simple retry before the dapi versioning is properly implemented
- for (let i = 0; i < 3; i += 1) {
+ for (let i = 0; i < MAX_RETRIES; i += 1) {
try {
// eslint-disable-next-line no-await-in-loop
const getContestedResourcesResponse = await grpcTransport.request(
PlatformPromiseClient,
'getContestedResources',
getContestedResourcesRequest,
options,
);
return GetContestedResourcesResponse
.createFromProto(getContestedResourcesResponse);
} catch (e) {
if (e instanceof InvalidResponseError) {
lastError = e;
+ // Exponential backoff with jitter
+ const delay = Math.min(BASE_DELAY * Math.pow(2, i), 5000) + Math.random() * 1000;
+ await new Promise(resolve => setTimeout(resolve, delay));
} else {
throw e;
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TODO: simple retry before the dapi versioning is properly implemented | |
| for (let i = 0; i < 3; i += 1) { | |
| try { | |
| // eslint-disable-next-line no-await-in-loop | |
| const getContestedResourcesResponse = await grpcTransport.request( | |
| PlatformPromiseClient, | |
| 'getContestedResources', | |
| getContestedResourcesRequest, | |
| options, | |
| ); | |
| return GetContestedResourcesResponse | |
| .createFromProto(getContestedResourcesResponse); | |
| } catch (e) { | |
| if (e instanceof InvalidResponseError) { | |
| lastError = e; | |
| } else { | |
| throw e; | |
| } | |
| } | |
| } | |
| const MAX_RETRIES = 3; | |
| const BASE_DELAY = 1000; // 1 second | |
| // TODO: simple retry before the dapi versioning is properly implemented | |
| for (let i = 0; i < MAX_RETRIES; i += 1) { | |
| try { | |
| // eslint-disable-next-line no-await-in-loop | |
| const getContestedResourcesResponse = await grpcTransport.request( | |
| PlatformPromiseClient, | |
| 'getContestedResources', | |
| getContestedResourcesRequest, | |
| options, | |
| ); | |
| return GetContestedResourcesResponse | |
| .createFromProto(getContestedResourcesResponse); | |
| } catch (e) { | |
| if (e instanceof InvalidResponseError) { | |
| lastError = e; | |
| // Exponential backoff with jitter | |
| const delay = Math.min(BASE_DELAY * Math.pow(2, i), 5000) + Math.random() * 1000; | |
| await new Promise(resolve => setTimeout(resolve, delay)); | |
| } else { | |
| throw e; | |
| } | |
| } | |
| } |
| if (Buffer.isBuffer(contractId)) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| contractId = Buffer.from(contractId); | ||
| } |
There was a problem hiding this comment.
Fix incorrect Buffer conversion logic
The Buffer conversion logic has the same issue as in getContestedResourcesFactory.
- if (Buffer.isBuffer(contractId)) {
+ if (!Buffer.isBuffer(contractId)) {
// eslint-disable-next-line no-param-reassign
contractId = Buffer.from(contractId);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (Buffer.isBuffer(contractId)) { | |
| // eslint-disable-next-line no-param-reassign | |
| contractId = Buffer.from(contractId); | |
| } | |
| if (!Buffer.isBuffer(contractId)) { | |
| // eslint-disable-next-line no-param-reassign | |
| contractId = Buffer.from(contractId); | |
| } |
| grpcTransportMock = { | ||
| request: this.sinon.stub().resolves(response), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling test cases.
The current test suite lacks test cases for error scenarios. Consider adding tests for:
- Network errors
- Invalid responses
- Timeout scenarios
Issue being fixed or feature implemented
JS DAPI Client is missing methods for querying contested resources info, that is used to retrieve current contested documents and vote resource state info.
Backport from https://github.com/owl352/dapi-client
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Improvements