-
Notifications
You must be signed in to change notification settings - Fork 72
added ncw header #329
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.
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
added ncw header #329
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for including a custom NCW header (X-End-User-Wallet-Id) in all SDK API calls and updates Web3 connection methods to accept an optional requestOptions parameter.
- Introduce an optional
requestOptionsargument to Web3 connection methods inFireblocksSDK - Centralize NCW header injection into a new
addNCWHeaderhelper inApiClient - Refactor each
issue*Requestmethod to calladdNCWHeaderinstead of inline header logic
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/fireblocks-sdk.ts | Extended getWeb3Connections, submitWeb3Connection, and removeWeb3Connection to accept requestOptions |
| src/api-client.ts | Added private addNCWHeader method and updated all issueGetRequest, issuePostRequest, issuePutRequest, issuePatchRequest, and issueDeleteRequest to use it |
Comments suppressed due to low confidence (2)
src/fireblocks-sdk.ts:1626
- Add JSDoc for the new
requestOptionsparameter togetWeb3Connectionsto explain its purpose and structure.
+ order,
src/api-client.ts:68
- No tests verify that
addNCWHeadercorrectly setsX-End-User-Wallet-Id; consider adding unit tests for scenarios with and withoutrequestOptions.ncw.walletId.
+ this.addNCWHeader(headers, requestOptions);
Pull Request Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes (link to the issue here)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: