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.

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

feat: add support for axios response interceptor, remove deprecated methods #141

Merged
merged 12 commits into from
Feb 16, 2023

Conversation

yarinvak
Copy link
Contributor

@yarinvak yarinvak commented Jan 19, 2023

BREAKING CHANGE: removed all deprecated SDK methods

Pull Request Description

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • Locally tested against Fireblocks API

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Any dependent changes have been merged and published in downstream modules
  • I have added corresponding labels to the PR

…ders in response

BREAKING CHANGE: changed response type for all of API methods, and removed deprecated transfer-assist operations
@github-actions github-actions bot added the enhancement New feature or request label Jan 19, 2023
Copy link
Collaborator

@idanya idanya left a comment

Choose a reason for hiding this comment

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

Consider refactoring and extracting all the different request methods logic to a single function that uses this.axiosInstance.request()

YoavBZ
YoavBZ previously requested changes Jan 22, 2023
Copy link
Collaborator

@YoavBZ YoavBZ left a comment

Choose a reason for hiding this comment

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

return await is considered redundant, please avoid using it (unless in try-catch block)

@yarinvak
Copy link
Contributor Author

@YoavBZ where do you see it in my changes? I'm not going to fix all old usages in this PR

@YoavBZ
Copy link
Collaborator

YoavBZ commented Jan 23, 2023

@YoavBZ where do you see it in my changes? I'm not going to fix all old usages in this PR

@yarinvak Yeah, it's actually the current implementation (in almost every public method in fireblocks-sdk.ts), we can fix it in the next releases.

@YoavBZ YoavBZ dismissed their stale review January 23, 2023 15:27

Not needed at the moment

Yarin Vaknin added 2 commits February 6, 2023 15:36
@github-actions github-actions bot added the chore label Feb 6, 2023
Yarin Vaknin added 6 commits February 9, 2023 13:54
BREAKING CHANGE: getVaultAccounts, getVaultAccount, getExchangeAccount methods were removed and replaced by getVaultAccountsWithPageInfo, getVaultAccountById, getExchangeAccountById - check the migration guide in README.md
@yarinvak yarinvak changed the title feat: return APIResponse instead of raw response data feat: add support for axios response interceptor, remove deprecated methods Feb 14, 2023
@yarinvak yarinvak merged commit f3aeb3d into master Feb 16, 2023
@yarinvak yarinvak deleted the return-api-response branch February 16, 2023 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking chore enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants