{"action":"created","issue":{"url":"https://api.github.com/repos/AugurProject/ethrpc/issues/12","repository_url":"https://api.github.com/repos/AugurProject/ethrpc","labels_url":"https://api.github.com/repos/AugurProject/ethrpc/issues/12/labels{/name}","comments_url":"https://api.github.com/repos/AugurProject/ethrpc/issues/12/comments","events_url":"https://api.github.com/repos/AugurProject/ethrpc/issues/12/events","html_url":"https://github.com/AugurProject/ethrpc/pull/12","id":208619857,"number":12,"title":"Refactors transport layer.","user":{"login":"MicahZoltu","id":886059,"avatar_url":"https://avatars1.githubusercontent.com/u/886059?v=3","gravatar_id":"","url":"https://api.github.com/users/MicahZoltu","html_url":"https://github.com/MicahZoltu","followers_url":"https://api.github.com/users/MicahZoltu/followers","following_url":"https://api.github.com/users/MicahZoltu/following{/other_user}","gists_url":"https://api.github.com/users/MicahZoltu/gists{/gist_id}","starred_url":"https://api.github.com/users/MicahZoltu/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/MicahZoltu/subscriptions","organizations_url":"https://api.github.com/users/MicahZoltu/orgs","repos_url":"https://api.github.com/users/MicahZoltu/repos","events_url":"https://api.github.com/users/MicahZoltu/events{/privacy}","received_events_url":"https://api.github.com/users/MicahZoltu/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":8,"created_at":"2017-02-18T06:47:11Z","updated_at":"2017-03-11T22:27:05Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/AugurProject/ethrpc/pulls/12","html_url":"https://github.com/AugurProject/ethrpc/pull/12","diff_url":"https://github.com/AugurProject/ethrpc/pull/12.diff","patch_url":"https://github.com/AugurProject/ethrpc/pull/12.patch"},"body":"Note\r\n----\r\nThis PR is not complete yet and should _*NOT*_ be merged.\r\n\r\nI have *only* changed the `version` function over to use the new connection stuff and I have left all of the old code in.  Once this PR has gone through the review process and there is buy-in from stake holders I will go through and migrate all functions to use the new transport layer and delete a bunch of code that is no longer necessary.\r\n\r\nThis PR also does not have an implementation for `IPC` or `SYNC` transports.  If it is decided to move forward with these changes, implementing both should be fairly straightforward and easy as the system was setup to be easily extendable and thought was given to the `SYNC` mode when designing it.\r\n\r\nThe one thing that I haven't put much thought into yet is pub/sub.  I don't believe there will be any problems, but it is possible that when it comes time to implement it something will pop up.  I believe this to be the highest risk factor in moving forward with these changes.\r\n\r\nMore testing is also warranted.  Right now the coverage is just enough to give me confidence that the changes all work, but for thoroughness more regression tests should be added for the various transport types and edge cases.\r\n\r\n----\r\n\r\nThis extracts/isolates the transport layer from the rest of the code in ethrpc.  This gives a layer of abstraction that allows each transport type to deal with queueing, reconnecting and errors in isolation without the rest of the code needing to have any understanding about them, their differences or transient connectivity issues.\r\n\r\nOn connect, one or more will be instantiated and connections will attempted to be established internally.  If at least one transport is working, then the application will proceed.  If no transports are able to be established then connect will fail.\r\n\r\nWhen a request is made, if it fails in a way that is retryable then the transport will retry under-the-hood, reconnecting if necessary, only calling back to the user know once the request has gone through."},"comment":{"url":"https://api.github.com/repos/AugurProject/ethrpc/issues/comments/285905220","html_url":"https://github.com/AugurProject/ethrpc/pull/12#issuecomment-285905220","issue_url":"https://api.github.com/repos/AugurProject/ethrpc/issues/12","id":285905220,"user":{"login":"MicahZoltu","id":886059,"avatar_url":"https://avatars1.githubusercontent.com/u/886059?v=3","gravatar_id":"","url":"https://api.github.com/users/MicahZoltu","html_url":"https://github.com/MicahZoltu","followers_url":"https://api.github.com/users/MicahZoltu/followers","following_url":"https://api.github.com/users/MicahZoltu/following{/other_user}","gists_url":"https://api.github.com/users/MicahZoltu/gists{/gist_id}","starred_url":"https://api.github.com/users/MicahZoltu/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/MicahZoltu/subscriptions","organizations_url":"https://api.github.com/users/MicahZoltu/orgs","repos_url":"https://api.github.com/users/MicahZoltu/repos","events_url":"https://api.github.com/users/MicahZoltu/events{/privacy}","received_events_url":"https://api.github.com/users/MicahZoltu/received_events","type":"User","site_admin":false},"created_at":"2017-03-11T22:27:05Z","updated_at":"2017-03-11T22:27:05Z","body":"> I haven't used MetaMask before -- perhaps it only works on mainnet?\r\n\r\nWhen testing MetaMask, did you move the augur connect code into `onload`?\r\n\r\nMetaMask support is tenuously tested at best.  I added support for it and did some *basic* testing , but I wouldn't be surprised if it was buggy.  At the moment, my primary concern is getting my outstanding branches merged because they are already quite large and I really am not comfortable with large long running pull requests.  Since the MetaMask support is new and doesn't impact existing code I put off doing rigorous testing for the time being.  If you like, we can wait to merge this until after MetaMask is fully verified but I'm worried that may take a while since I'm not particularly familiar with it and debugging it is likely going to be a bit of a pain.  The only reason I added the MetaMask stuff into this branch/PR was because the change was so small, it didn't impact the rest of the code much at all and I wanted to verify it would fit in the current transport model.  If you want, I can extract the MetaMask code into a separate PR?\r\n\r\n> Have you been able to successfully use the UI via websockets? I'm running a local geth instance that has websockets enabled but it shows augur.subscriptionsSupported=false in the console. (I assume this should be true if I'm using WS or IPC.)\r\n> * Chrome's network tab shows a pending websocket connection to 127.0.0.1 that is spamming eth_getFilterChanges RPC requests. Two concerns here: first, I'm not sure this RPC request is even available over websockets. Second, if using websockets, filters shouldn't be used at all -- all listeners should be subscriptions.\r\n> * Chrome's network tab also shows HTTP requests sent to https://eth3.augur.net and WS requests sent to wss://ws.augur.net. My local node is not connected to the Ropsten network (I'm on a local, private test chain), so there should be 0 requests directed to eth3.augur.net or ws.augur.net, as these will return conflicting information compared with that of my local node.\r\n\r\nI just fixed a bug in the subscription detection code in augur.js, I think I introduced it in one of my last changes and didn't go back and retest web sockets, that was my bad.\r\n\r\nAll requests that work with HTTP also work over WS, though you are correct that it shouldn't have been taking the `getFilterChanges` code path when subscriptions were supported.  That is the bug I fixed.\r\n\r\nI'm not seeing any contact with `eth3.augur.net` or `ws.augur.net` when I connect to a local node.  At the moment, the way things are hooked up, if you give it no websocket node to talk to but you tell it to use fallbacks then it will fallback to `ws.augur.net`.  The same is true for `eth3.augur.net`.  Alternatively, if it is unable to connect to the URL you provided then it will fallback to hosted nodes for either of those.  It sounds like something is causing you to hit the fallback logic?  What `env.json` are you using?  Note, at the moment the the fallback nodes are hard coded to `eth3.augur.net` and `ws.augur.net` here https://github.com/ethereumjs/ethereumjs-connect/blob/ethrpc-update/index.js#L229-L230 which seems like the wrong place to add fallbacks, but it required the least changes.\r\n\r\n> How do I see which transport I am using with the new ethrpc? This information needs to be available for debugging. On the old ethrpc, I could check wsStatus and ipcStatus to see if I'm connected; what is the equivalent in the new ethrpc?\r\n\r\n`augurjs.augur.rpc.internalState.transporter.internalState` contains the transports that are currently being used.  You can look at `.metaMaskTransport`, `.httpTransport`, `.wsTransport` and `.syncTransport` on that object to find out which are available.  If it is null, it means that transport isn't available in this session.  Note: Those things shouldn't be used outside of debugging, which is why they are in the `internalState` object.\r\n\r\n> The broadcast debugging option needs to behave the same way it did before: it needs to print the RPC requests (exactly as they are sent to geth) to the console, and it needs to print exactly what endpoints the requests are being sent to, and whether the requests are sync or async.\r\n\r\nI have added this in, so if `.debug.broadcast === true` then it will print out the request and the destination.\r\nProfessional opinion: there shouldn't be debugging code in production (master) and instead a debugger should be used if you need to inspect this data while debugging.  In this case in particular, all of the information you want is available in Chrome's networking tab.\r\n\r\n> Running the UI's test suite (yarn test) shows that all gas fees in transactions are now set to 0, instead of their expected values.\r\n\r\nThis is because the tests are using ethrpc without first calling connect (as mentioned elsewhere in this PR, this is incorrect usage of the library).  The quick-fix for this would be to have the tests set `rpc.gasPrice = 20000000000`.  This would make the tests pass, though it is unclear what exactly the tests are testing if this solution is employed.  Let me know if you want me to get a PR out that applies this, or if you want to explore other options for addressing this issue."}}
{"actor":{"display_login":"MicahZoltu"}}