ethereum / sharding

Sharding manager contract, and related software and tests
480 stars 105 forks source link

SMCHandler/ShardTracker API improvements #103

Closed jannikluhn closed 6 years ago

jannikluhn commented 6 years ago

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):

1) 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).

2) 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).

3) 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.

4) ShardTracker expect a LogHandler which could just be created by the ShardTracker itself.

5) 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.

6) 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).

@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.

NIC619 commented 6 years ago

I will work on these improvements. Some thoughts: re. 2. 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).

re. 3. 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).

re. 6. I will dig into it that. Thanks for the information!

hwwhww commented 6 years ago

re 6. The fastest way for deploying is setting SMC bytecode in genesis. That may actually related to https://github.com/ethereum/eth-tester/issues/88 ...!

mhchia commented 6 years ago

re 1. I think renaming makes sense. re 4. Previously we crafted this LogHandler to replace the need for a server-side filter, and I thought it should be possibly replaced by other implementation, so I make it a dependency injected. Now it seems no special functionalities in this LogHandler anymore. I'm all fine with whether to change it or not.

pipermerriam commented 6 years ago

Re gas for transactions. I think you should be fine in most cases just letting Web3.py set the gas for you. It will use the gas estimation endpoint to determine the appropriate gas value for a transaction if it isn't explicitly provided.

NIC619 commented 6 years ago

Will also look into the new signing middleware mentioned in the #106 comment.