A Proposal for Sharding Client Abstraction

[rauljordan]

Building on #82, we were discussing how to abstract common methods from collators/proposers into a general sharding client as an effort to have no code duplication in collator.go and proposer.go respectively.

@terenc3t and I came up with this scheme that works in this PR, and wanted to get @nisdas and @prestonvanloon's opinions on.

Here is an explanation of the changes

• We changed collator_client.go to client.go. This file exposes an abstract ShardingClient struct that will implement the collator and proposer interface. It contains all common methods for both.
• We refactored the collator.go file to contain an interface with only the functions collators will need, as well as specific collator functionality. The same thing can be done with proposer.go.
• We removed the smc.go file, as InitSMC should be a common function to both collators and proposers.
• We modified the cmd entrypoint by calling the functions sharding.startCollatorClient and sharding.startProposerClient. These functions take in a struct that implements the collator interface and the proposer interface, respectively. For example:

func collatorClient(ctx *cli.Context) error {
	c := sharding.MakeShardingClient(ctx)
	if err := sharding.StartCollatorClient(c); err != nil {
		return err
	}
	sharding.WaitCollatorClient(c)
	return nil
}

Here is where we need feedback

• How can we maximize the benefit of abstraction? Are there methods in the collator interface in collator.go that we can obfuscate even more?
• Are there methods we should keep unexported? Which ones?
• Does the entry point and initialization of the client make sense?

[nisdas]

This looks fine and works well too. I think we can still further abstract all this:
creating the RPC client , unlocking the account and deploying the SMC should not have to be run when sharding.StartCollatorClient(c) is called.

This should all be run before the collator client is called , otherwise we end up having repetition again on the proposer client.
Though wouldn't it be cleaner to have subpackages, further down along in our implementation the code for both out proposal and collator clients would be a lot more complex. It makes sense to keep them separate if we can

[terencechain]

It depends if we are just implementing for phase 1. As we introduce Executor client in phase 2, it will not need to deploy SMC
We should abstract creating RPC client and unlocking the account though. I can take care of it later tonight

[prestonvanloon]

I'm not sure how @nisdas feels about this but I find it quite frustrating. @nisdas put out a design doc for review and that was the place for suggestions like this, not a pull request. If we were to merge this, it would effectively throw away all of the work he has done so far (and has not pushed to his branch yet).

I don't understand the motivation for this change. It appears you guys are against introducing new packages under sharding in order to separate the consumer/producer of a client in the sharding paradigm but you have provided no basis why your argument is better. There was one comment that said it would make it more difficult to merge upstream which is not true at all. Furthermore, @nisdas and I provided links to various go style guides and standards to support our argument that splitting these up would be more effective.

I recommend that we close this PR entirely and take the discussion back to the google doc and perhaps have a review phone call about it. https://docs.google.com/document/d/1__74pTWFpzfLLJ8017XeWQWJfs89YgTGxuefjkFUAaY/edit

[terencechain]

@prestonvanloon we don't mean to frustrate you or @nisdas or step into his shoes. The reasons this PR got created was to help us kick start #79 and #80. We wanted to work on them over the weekend, but it was blocked by #82. We are more than happy to merge back with Nishant's #82 after he is done. In the mean time, we can have a call when everyone is free

[rauljordan]

Fixed in #85. This PR will be closed.