Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring of Code #82

Closed
wants to merge 3 commits into from
Closed

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented Mar 29, 2018

It was agreed by the team in #60 to refactor the code in order to prevent duplication of code and for easier testing.
The objectives of this PR are to

  • Abstract all common methods into a common interface which would be in the sub-package sharding/client
  • Add the collator and proposer specific methods in sharding/collator and sharding/proposer respectively.
  • Write tests for the general client, to ensure that it is running correctly

More documentation can be found here :
https://docs.google.com/document/d/1__74pTWFpzfLLJ8017XeWQWJfs89YgTGxuefjkFUAaY/edit

@nisdas nisdas added the Phase 1 label Mar 29, 2018
@nisdas nisdas added this to the Ruby milestone Mar 29, 2018
@nisdas nisdas self-assigned this Mar 29, 2018
@nisdas nisdas added this to To do in Validator Client via automation Mar 29, 2018
@nisdas nisdas added this to To do in Collation Proposals via automation Mar 29, 2018
@rauljordan
Copy link
Contributor

Not a big fan of creating individual packages, especially as this will be really difficult for upstream merging. Just created #83 as an alternative proposal to this problem.

@prestonvanloon
Copy link
Member

@rauljordan Individual packages under the sharding package make no difference on the ease or difficulty of merging upstream.

@terencechain
Copy link
Member

terencechain commented Mar 31, 2018

@prestonvanloon I agree there's no difference, the question is do we need it. When we introduce more actors (ex: executor), are we going to create a executor package as well?

@prestonvanloon
Copy link
Member

@terenc3t Yes, we need packages for proper code organization. As the sharding features grow, it will be unmaintainable to have a single package that does everything.

Absolutely, the execution would go in another package since it’s another responsibility.

@rauljordan
Copy link
Contributor

Referenced by #85 and it includes @nisdas's work. We can close this one.

@rauljordan rauljordan closed this 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
@renovate renovate bot mentioned this pull request Apr 13, 2020
1 task
Redidacove pushed a commit to Redidacove/prysm that referenced this pull request Aug 13, 2024
Co-authored-by: rahul <raxhvl@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants