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

WP-1185 fix SDK using proxy #4169

Merged
merged 2 commits into from
Dec 22, 2023
Merged

Conversation

pengyuc-bitgo
Copy link
Contributor

@pengyuc-bitgo pengyuc-bitgo commented Dec 21, 2023

The superagent-proxy was removed earlier in this PR.
And we have to use the proxy-agent now. But that change missed some old usage of the superagent-proxy.

This PR also changed the proxy-agent version to 5.0. This is the version that was used under the hood before. The new version requires setting proxy url in the environment variable. It does not quite fit how we use the proxy-agent currently. If we want to upgrade to the latest proxy-agent, it should be done in a separate PR.

This change is tested with the new example codes and local proxy server. Verified that it is using the proxy by shutting down the proxy and saw the connection failure error.

  • Bug fix (non-breaking change which fixes an issue)

Ticket: WP-1185

Copy link

socket-security bot commented Dec 21, 2023

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
proxy-agent 5.0.0 network +22 1.05 MB tootallnate
proxy 2.1.1 network +5 105 kB tootallnate
superagent 4.1.0 environment +0 484 kB kornel

@pengyuc-bitgo pengyuc-bitgo marked this pull request as ready for review December 21, 2023 01:41
@pengyuc-bitgo pengyuc-bitgo requested review from a team as code owners December 21, 2023 01:41
Comment on lines 6 to 9
// This *must* match the environment you are using in the create-wallet script
const bitgoApi = 'https://app.bitgo-test.com';
// TODO: replace with your access token
const secretAccessToken = 'yourAccessToken';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we changing this example? The goal of this example is to show an example of the server injecting the access token opposed to needing it in the frontend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have roll-back the changes to this example and added another example for http proxy usage.
Got confused because of the name. The use case are actually different.

Copy link
Contributor

@zahin-mohammad zahin-mohammad left a comment

Choose a reason for hiding this comment

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

Changes lgtm, but I think the examples for create-wallet/server should stay the same. Feel free to sync with @alebusse to see why its having issues (we have clients using similar setups so we should ensure it still works)

We removed the superagent-proxy earlier to use proxy-agent.
The change missed some old usage.

Change the proxy-agent version to 5.0. This is the version
that was used under the hood. The new version requires setting
proxy url in the environment variable. It does not quite fit how
we use the proxy-agent currently.

Ticket: WP-1185
@pengyuc-bitgo
Copy link
Contributor Author

Changes lgtm, but I think the examples for create-wallet/server should stay the same. Feel free to sync with @alebusse to see why its having issues (we have clients using similar setups so we should ensure it still works)

Got confused by the example's name. Added another example for http proxy. Also updated the README.md.

@pengyuc-bitgo pengyuc-bitgo merged commit 536a1b0 into master Dec 22, 2023
10 checks passed
@pengyuc-bitgo pengyuc-bitgo deleted the WP-1185-fix-proxy-get-constant branch December 22, 2023 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants