{"action":"created","issue":{"url":"https://api.github.com/repos/ethereum/sharding/issues/103","repository_url":"https://api.github.com/repos/ethereum/sharding","labels_url":"https://api.github.com/repos/ethereum/sharding/issues/103/labels{/name}","comments_url":"https://api.github.com/repos/ethereum/sharding/issues/103/comments","events_url":"https://api.github.com/repos/ethereum/sharding/issues/103/events","html_url":"https://github.com/ethereum/sharding/issues/103","id":328575187,"node_id":"MDU6SXNzdWUzMjg1NzUxODc=","number":103,"title":"SMCHandler/ShardTracker API improvements","user":{"login":"jannikluhn","id":29854669,"node_id":"MDQ6VXNlcjI5ODU0NjY5","avatar_url":"https://avatars1.githubusercontent.com/u/29854669?v=4","gravatar_id":"","url":"https://api.github.com/users/jannikluhn","html_url":"https://github.com/jannikluhn","followers_url":"https://api.github.com/users/jannikluhn/followers","following_url":"https://api.github.com/users/jannikluhn/following{/other_user}","gists_url":"https://api.github.com/users/jannikluhn/gists{/gist_id}","starred_url":"https://api.github.com/users/jannikluhn/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jannikluhn/subscriptions","organizations_url":"https://api.github.com/users/jannikluhn/orgs","repos_url":"https://api.github.com/users/jannikluhn/repos","events_url":"https://api.github.com/users/jannikluhn/events{/privacy}","received_events_url":"https://api.github.com/users/jannikluhn/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2018-06-01T16:01:28Z","updated_at":"2018-06-02T05:05:10Z","closed_at":null,"author_association":"COLLABORATOR","body":"Here are some ideas for improvements of the API of `SMCHandler` and `ShardTracker` I got when implementing `SMCService` (most of them for the `__init__` method and some probably a little bit subjective):\r\n\r\n1) I think `SMCHandler` should be renamed to just `SMC` as its a subclass of `web3.Contract`. While it technically does not refer to the contract itself, `SMC` would be more inline with web3's terminology (contract <- sharding manager contract).\r\n\r\n2) Both handler and tracker take a `config` dictionary which mainly contains parameters for the contract. I think it would be better if those parameters would be retrieved from the on-chain contract itself, otherwise it's possible that there is a mismatch. The remaining two (\"DEFAULT_GAS\" and \"GAS_PRICE\") could just be passed as standalone parameters or removed (see next bullet point).\r\n\r\n3) The various `send_transaction` methods in `SMCHandler` take a `gas` argument. As the required gas of these functions is known (it's part of the ABI) it would be nice if it would just use that number.\r\n\r\n4) `ShardTracker` expect a `LogHandler` which could just be created by the `ShardTracker` itself.\r\n\r\n5) To create the `SMCHandler` one needs to provide the ABI. As the ABI doesn't change that parameter is useless, the constructor could just load the ABI from JSON.\r\n\r\n6) Deploying the SMC is quite cumbersome. Two ideas here: Either make the test utils importable for other packages or, even better, make deploying easy by setting the bytecode. Then one can just use the `constructor` functionality provided by `web3` (if I understand the docs correctly, haven't actually tried it).\r\n\r\n@NIC619 what do you think of these? I would have already started making PRs, but wanted to discuss first, and also give you the chance to work on them if you like. I'm mainly blocked by the last point because there's not really a good way to test the `SMCService` right now."},"comment":{"url":"https://api.github.com/repos/ethereum/sharding/issues/comments/394059012","html_url":"https://github.com/ethereum/sharding/issues/103#issuecomment-394059012","issue_url":"https://api.github.com/repos/ethereum/sharding/issues/103","id":394059012,"node_id":"MDEyOklzc3VlQ29tbWVudDM5NDA1OTAxMg==","user":{"login":"NIC619","id":17670147,"node_id":"MDQ6VXNlcjE3NjcwMTQ3","avatar_url":"https://avatars0.githubusercontent.com/u/17670147?v=4","gravatar_id":"","url":"https://api.github.com/users/NIC619","html_url":"https://github.com/NIC619","followers_url":"https://api.github.com/users/NIC619/followers","following_url":"https://api.github.com/users/NIC619/following{/other_user}","gists_url":"https://api.github.com/users/NIC619/gists{/gist_id}","starred_url":"https://api.github.com/users/NIC619/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/NIC619/subscriptions","organizations_url":"https://api.github.com/users/NIC619/orgs","repos_url":"https://api.github.com/users/NIC619/repos","events_url":"https://api.github.com/users/NIC619/events{/privacy}","received_events_url":"https://api.github.com/users/NIC619/received_events","type":"User","site_admin":false},"created_at":"2018-06-02T05:05:10Z","updated_at":"2018-06-02T05:05:10Z","author_association":"COLLABORATOR","body":"I will work on these improvements. Some thoughts:\r\n2. Retrieving config parameters from contract has two cost: one is the cost of exposing these variables  (which will increase the contract deployment gas cost) and another one is the cost of additional contract call(-> web3 -> JSON-RPC).\r\n\r\n3. Agreed. We can use the estimated gas cost in ABI plus some extra gas(or multiplied by some number). It's implemented that way because in previous SMC there are for-loops and the gas estimator is not really reliable at the time(though I'm not sure how reliable it is right now).\r\n\r\n6. I will dig into it that. Thanks for the information!"}}
{"actor":{"display_login":"NIC619"}}