Refactoring sharding package for proposer/client separation #85

prestonvanloon · 2018-03-31T22:05:49Z

This builds on the work of @nisdas in #82.

Why refactor?

We started this package under the original sharding design that would have only a collator. Now that we need to split responsibilities into 2+ roles, it makes sense to keep things isolated in their own packages.

Following @nisdas's design along with suggestions in:

We split up the client aspect of the roles into a client package. This package is responsible for the actual communications (i.e. connecting to a running geth node, initializing the SMC, handling account unlocking). Then we can have a collator that is more focused in its own package with responsibilities such as listening for block headers, joining the collator pool, etc. Furthermore, we start the proposer package to run with its own responsibilities.

Why have `collator_client.go` and `collator.go`?

Good question! At first, I didn't like this, but the collator_client could/should expose the public methods that the geth/shardingcmd can call (i.e. Start and NewCollator) while the interesting code lives in collator.go. This felt helpful to separate for testing purposes, but I can be convinced out of this pattern if your argument is strong enough :)

This supersedes (and includes) @nisdas's work in #82.
Closes #82
Closes #83
Required for #79 and #80

TODO (in future PR):

More testing of collator
Move common testing methods (mock client) into sharding/client/testing

terencechain · 2018-03-31T22:18:56Z

Looks good! Two questions I'd like to discuss:

The necessity to separate proposer and collator into different packages. It means whenever a new actor is introduced, it needs to be its own package. What benefit do we get out of that
I don't think initSMC should be on the client level because not all types of client will need to interact with SMC

prestonvanloon · 2018-03-31T22:29:37Z

Good questions!

The benefit is a clear separation of concerns as new responsibilities join sharding.This seems like it would scale better than 1 sharding package with many files in it. Is there a reason we would prefer to have all of them in one package?
Hmm yes, perhaps the smc.go could live in contracts? Would the executor not need access to the SMC?

prestonvanloon · 2018-04-01T01:12:26Z

Just to be clear, I am OK with one sharding package.
I'd like to reference a definitive authority (if one exists) on what is preferred. If there is none, then it is just personal preference and we should establish a standard as a team or determine what upstream prefers.

rauljordan

Looks good to me. I like the interfaces you created and how the methods in collator and proposer simply take this interface as an argument. Thank you for putting this together and explaining your reasoning - I will do the same moving forward on future PRs.

rauljordan · 2018-04-01T02:16:54Z

sharding/client/smc.go

+
+// initSMC initializes the sharding manager contract bindings.
+// If the SMC does not exist, it will be deployed.
+func initSMC(c *shardingClient) (*contracts.SMC, error) {


Can we put this function in the client.go file? The client is the one that inits the SMC and it doesn't make sense for this file to exist at all if we only envision having a few functions or just 1 in it.

Sure. I don't feel very strongly about having this method in its own file so I'll move it.

rauljordan · 2018-04-01T02:18:38Z

sharding/client/client.go

-package sharding
-
-//go:generate abigen --sol contracts/sharding_manager.sol --pkg contracts --out contracts/sharding_manager.go
+package client


Let's please rename this to sharding_client.go. client is ambiguous with the ethclient that is a part of the struct.

This isn't related to the filename, but the package path. Do we need the redundancy of sharding/shardingclient/client.go?
Keep in mind, that it would only be used by the collator/proposer.
Personally, I don't think this conflicts with ethclient, but I see your point too.

Let's revisit this in the future. I'm going to leave for now.

terencechain

Looks good after resolving @rauljordan feedbacks

terencechain · 2018-04-01T06:44:46Z

sharding/client/client.go

-	keystore *keystore.KeyStore // Keystore containing the single signer
-	ctx      *cli.Context       // Command line context
-	smc      *contracts.SMC     // The deployed sharding management contract
+// General Client for Collator. Communicates to Geth node via JSON RPC.


This is not only for collator, it's for proposer and future actors too

terencechain · 2018-04-01T14:36:51Z

sharding/client/client.go

-	// TODO: this function should store the collator's SMC index as a property
-	// in the client's struct
-	if err := joinCollatorPool(c); err != nil {
+	smc, err := initSMC(c)


I don't know if executor node uses SMC, we might have to change this for phase 3

The executor node won't use this client anyway, it doesn't need main chain communication, right?

That's true

terencechain · 2018-04-01T14:45:18Z

sharding/collator/smc.go

+
+// joinCollatorPool checks if the account is a collator in the SMC. If
+// the account is not in the set, it will deposit 100ETH into contract.
+func joinCollatorPool(c client.Client) error {


This is a Collator specific function. Should we move it under collator.go and delete smc.go completely?

Sure, sounds good to me

prestonvanloon · 2018-04-01T21:05:49Z

Travis failed some times, but tests pass so i'll merge anyway

Refactoring sharding package for proposer/client separation

Refactoring sharding package for proposer/client separation align config with phase 1 spec update SMC to use 1000eth as collator deposit size fix deposit for collator_test fixed config test and added more test cases

Refactoring sharding package for proposer/client separation Former-commit-id: 9767017

Refactoring sharding package for proposer/client separation align config with phase 1 spec update SMC to use 1000eth as collator deposit size fix deposit for collator_test fixed config test and added more test cases Former-commit-id: d1c9341

Refactoring sharding package for proposer/client separation Former-commit-id: c7a28a1abc8886d15a20880ecba9c7b67633c323 [formerly 9767017] Former-commit-id: 1a341f7

Refactoring sharding package for proposer/client separation align config with phase 1 spec update SMC to use 1000eth as collator deposit size fix deposit for collator_test fixed config test and added more test cases Former-commit-id: 7fadb044ec7bb136964781ac271ce6abb4cff86f [formerly d1c9341] Former-commit-id: 9747bde

nisdas and others added 8 commits March 30, 2018 06:55

Adding sub-packages

ed8e799

Refactor Code

0e13521

Fix RPC error and change log statement

653ebf9

Cleanup refactoring for sharding client interface and collator.

96bc202

Refactor proposer

8d66667

Working on testing after refactoring

96cf826

lint

e081c08

Remove comment

b551bf7

prestonvanloon requested review from rauljordan, terencechain and nisdas March 31, 2018 22:08

prestonvanloon added the Phase 1 label Mar 31, 2018

prestonvanloon added this to To do in Validator Client via automation Mar 31, 2018

prestonvanloon added this to To do in Collation Proposals via automation Mar 31, 2018

prestonvanloon added this to the Ruby milestone Mar 31, 2018

Add back go generate comment

edd7cc8

rauljordan approved these changes Apr 1, 2018

View reviewed changes

This was referenced Apr 1, 2018

A Proposal for Sharding Client Abstraction #83

Closed

Refactoring of Code #82

Closed

terencechain approved these changes Apr 1, 2018

View reviewed changes

nisdas approved these changes Apr 1, 2018

View reviewed changes

terencechain reviewed Apr 1, 2018

View reviewed changes

sharding: Package comments and responding to PR feedback

ff59640

prestonvanloon merged commit 5efb934 into prysmaticlabs:master Apr 1, 2018

Validator Client automation moved this from To do to Done Apr 1, 2018

Collation Proposals automation moved this from To do to Done Apr 1, 2018

prestonvanloon deleted the refactoring branch April 1, 2018 21:06

prestonvanloon added a commit that referenced this pull request Jul 22, 2018

Merge pull request #85 from prestonvanloon/refactoring

9767017

Refactoring sharding package for proposer/client separation

prestonvanloon added a commit that referenced this pull request Jul 22, 2018

Merge pull request #85 from prestonvanloon/refactoring

1a341f7

Refactoring sharding package for proposer/client separation Former-commit-id: 9767017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring sharding package for proposer/client separation #85

Refactoring sharding package for proposer/client separation #85

prestonvanloon commented Mar 31, 2018 •

edited

Loading

terencechain commented Mar 31, 2018

prestonvanloon commented Mar 31, 2018

prestonvanloon commented Apr 1, 2018 •

edited

Loading

rauljordan left a comment

rauljordan Apr 1, 2018

prestonvanloon Apr 1, 2018

prestonvanloon Apr 1, 2018

rauljordan Apr 1, 2018

prestonvanloon Apr 1, 2018 •

edited

Loading

prestonvanloon Apr 1, 2018

terencechain left a comment

terencechain Apr 1, 2018

prestonvanloon Apr 1, 2018

terencechain Apr 1, 2018

prestonvanloon Apr 1, 2018

terencechain Apr 1, 2018

terencechain Apr 1, 2018

prestonvanloon Apr 1, 2018

prestonvanloon commented Apr 1, 2018

Refactoring sharding package for proposer/client separation #85

Refactoring sharding package for proposer/client separation #85

Conversation

prestonvanloon commented Mar 31, 2018 • edited Loading

Why refactor?

Why have collator_client.go and collator.go?

terencechain commented Mar 31, 2018

prestonvanloon commented Mar 31, 2018

prestonvanloon commented Apr 1, 2018 • edited Loading

rauljordan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestonvanloon Apr 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestonvanloon commented Apr 1, 2018

prestonvanloon commented Mar 31, 2018 •

edited

Loading

Why have `collator_client.go` and `collator.go`?

prestonvanloon commented Apr 1, 2018 •

edited

Loading

prestonvanloon Apr 1, 2018 •

edited

Loading