-
-
Notifications
You must be signed in to change notification settings - Fork 276
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
chore: update socks-proxy-agent from 6.1.1 to 8.0.4 #14202
Conversation
46fedff
to
51b59dd
Compare
51b59dd
to
bb55010
Compare
d60ec7e
to
42eab0b
Compare
bb0213a
to
69ed9d5
Compare
@marekrjpolak It would be nice if you have a look at this PR. I have made changes keeping in mind not to introduce breaking changes in connect API. Let me know what you think. |
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.
looks ok, did not test
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.
I repeatedly wasn't able to do a basic all seed discovery with Tor turned on. On current develop it was ok.
Also, do you know why is it necessary to pass the uri to the new SocksProxyAgent constructor as a string instead of object as noted here? I've tried it and it required more fields than we have and than is shown in the example, but I have no idea why.
@marekrjpolak I probably broke it by trying to satisfy the types but I should work now with this latest change eb2eeee As you mentioned using the other way it requires some fields that we do not use. But they way I am using is the one they use in their tests https://github.com/TooTallNate/proxy-agents/blob/main/packages/socks-proxy-agent/test/test.ts#L34 and it should work now. |
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.
Ok, I've checked basic Ehtereum (multi-identity) discovery, Electrum backend on onion address and CJ discovery and all seems to be working. 👍 Still, I'm a bit paranoid whether the identities are correctly separated which is kinda hard to verify. But it's probably fine.
It is hard to verify in the complex context of suite, but it is quite straight forward using the simple node script below while Tor from suite running: import https from 'https';
import { SocksProxyAgent } from 'socks-proxy-agent';
const socksServerUrl = new URL(`socks://127.0.0.1:36133`);
socksServerUrl.username = 'user';
socksServerUrl.password = 'password';
const agent = new SocksProxyAgent(socksServerUrl);
https.get('https://ipinfo.io', { agent }, (res) => {
console.log(res.headers);
res.pipe(process.stdout);
}); With that script if you change user or password get different IP in response. |
eb2eeee
to
4bdcebf
Compare
Description
update socks-proxy-agent from 6.1.1 to 8.0.4
SocksProxyAgentOptions
in order to keep the same API that we had before and not introduce breaking changes.SocksProxyAgent
.Related Issue
Resolve #14179