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

Currency should not be enabled if the `electrum` calls fails #492

Open
sindresorhus opened this issue Jul 22, 2019 · 10 comments

Comments

@sindresorhus
Copy link

commented Jul 22, 2019

When enabling the currency OOT using the electrum endpoint:

{
	coin: "OOT"
	method: "electrum"
	servers: [{url: "electrum1.utrum.io:10088"}, {url: "electrum2.utrum.io:10088"}]
	userpass: "…"
}

it fails with 500 (Internal Server Error), but the get_enabled_coins endpoint still returns OOT. get_enabled_coins should only return ones that were successfully enabled.

I would also like mm2 to return a proper error for the electrum endpoint than 500.

(I realize OOT has not been tested with mm2 yet, but it should still not cause mm2 to behave like this).

@cipig

This comment has been minimized.

Copy link

commented Jul 22, 2019

the above electrums are reachable, but they are outdated

(echo '{ "id": 1, "method": "server.version", "params": ["barterDEX", ["1.1", "2.0"]] }'; sleep 0.5) | ncat electrum1.utrum.io 10088
{"jsonrpc": "2.0", "result": ["ElectrumX 1.4.3", "1.2"], "id": 1}

could be a good idea to send server.version with a min. protocol version 1.4 to the electrums, which would result in this answer

(echo '{ "id": 1, "method": "server.version", "params": ["barterDEX", ["1.4", "2.0"]] }'; sleep 0.5) | ncat electrum1.utrum.io 10088
{"jsonrpc": "2.0", "error": {"code": 1, "message": "unsupported protocol version: ['1.4', '2.0']"}, "id": 1}

and if the answer is "unsupported protocol version" to not use this electrums

@artemii235

This comment has been minimized.

Copy link
Collaborator

commented Jul 23, 2019

@sindresorhus Hi, thank you for report. When you have the RPC error please always provide the response body as it will contain error description. Status code doesn't contain any useful info for troubleshooting. The error is easily reproducible in this case, but it might be environment related so the response body and app logs might be very helpful.
@cipig It's good idea, however MM2 should work with protocol version 1.2, but utrum electrums return 1.2 as supported as I understand, but they don't have the methods that were added even in 1.1 like blockchain.scripthash.get_balance, is there something wrong?

@sindresorhus

This comment has been minimized.

Copy link
Author

commented Jul 23, 2019

@artemii235 I forgot to mention that there was no response body as mm2 didn't include the Access-Control-Allow-Origin header, so the response body is not available:

Access to fetch at 'http://127.0.0.1:45675/' from origin 'app://-' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.

Can you make sure that header is always sent no matter what?

@artemii235

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2019

@sindresorhus What browser do you use? MM2 already includes the access-control-allow-origin header with default value http://localhost:3000, e.g.:
curl -v --url "http://127.0.0.1:7783" --data "{\"method\":\"electrum\",\"coin\":\"ETOMIC\",\"servers\":[{\"url\":\"test1.cipig.net:10025\"},{\"url\":\"test2.cipig.net:10025\"}],\"userpass\":\"$userpass\",\"mm2\":1}"

< HTTP/1.1 200 OK
< content-type: application/json
< access-control-allow-origin: http://localhost:3000
< transfer-encoding: chunked
< date: Wed, 24 Jul 2019 12:09:24 GMT

Also you might explicitly override the value by setting rpccors in MM2 JSON config, it was implemented some time ago

I guess that browser environment doesn't match the standard as according to the rfc2616 Field names are case-insensitive.

@artemii235

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2019

Or maybe there's some another issue Maybe we need to include other access-control headers to response, @sindresorhus could you please check if we require it? You stated that it worked before so I expect some changes in environment.

@sindresorhus

This comment has been minimized.

Copy link
Author

commented Jul 24, 2019

The headers are sent normally, but were not sent for the 500 code response mentioned earlier.

@artemii235

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2019

@sindresorhus, I see, let me check the cases when headers might be not sent.

@artemii235

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2019

Right, the header isn't included:

< HTTP/1.1 500 Internal Server Error
< content-type: application/json
< transfer-encoding: chunked
< date: Wed, 24 Jul 2019 13:05:22 GMT
< 
* Connection #0 to host 127.0.0.1 left intact
{"error":"rpc:229] utxo:1120] JsonRpcError { request: JsonRpcRequest { jsonrpc: \"2.0\", id: \"3\", method: \"blockchain.scripthash.get_balance\", params: [String(\"a70a7a7041ef172ce4b5f8208aabed44c81e2af75493540f50af7bd9afa9955d\")] }, error: Response(Object({\"code\": Number(-32601), \"message\": String(\"unknown method \\\"blockchain.scripthash.get_balance\\\"\")})) }"}* Closing connection 0

Will let you know once it's fixed.

@ArtemGr ArtemGr added this to Iteration in MM 2.0 via automation Aug 3, 2019

ArtemGr added a commit that referenced this issue Aug 3, 2019

@ArtemGr

This comment has been minimized.

Copy link
Collaborator

commented Aug 3, 2019

@sindresorhus, @lukechilds, HTTP 500 should return the JSON error now (pushed to master).

@sindresorhus

This comment has been minimized.

Copy link
Author

commented Aug 5, 2019

I tested it now. I can confirm that errors are now correctly shown.

sindresorhus added a commit to atomiclabs/hyperdex that referenced this issue Aug 5, 2019

Update mm2
To get better error messages: KomodoPlatform/atomicDEX-API#492 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.