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

refactor(nodebuilder): moving service/<service> services to respective node sub-packages #1056

Merged
merged 10 commits into from
Sep 30, 2022

Conversation

distractedm1nd
Copy link
Collaborator

@distractedm1nd distractedm1nd commented Sep 7, 2022

Moves services into nodebuilder and makes the internal structs private, offering a public interface instead.

Closes #960

Packages:

  • RPC
  • Share
  • Header
  • State

I took the branch from @renaynay's remote and pushed it to our remote here so that the diffs will be clear. #997 can then rebase off of that branch when things get merged into it.

Leaving RPC out of things since it will be ripped out anyways. Should we at least move it to the root though?

@distractedm1nd distractedm1nd added architecture Architecture / design-related issues area:api Related to celestia-node API labels Sep 7, 2022
@distractedm1nd distractedm1nd self-assigned this Sep 7, 2022
@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2022

Codecov Report

❗ No coverage uploaded for pull request base (node-refactorings@2441857). Click here to learn what that means.
The diff coverage is n/a.

@@                 Coverage Diff                  @@
##             node-refactorings    #1056   +/-   ##
====================================================
  Coverage                     ?   61.59%           
====================================================
  Files                        ?      150           
  Lines                        ?     8524           
  Branches                     ?        0           
====================================================
  Hits                         ?     5250           
  Misses                       ?     2815           
  Partials                     ?      459           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@distractedm1nd distractedm1nd marked this pull request as ready for review September 8, 2022 07:40
@distractedm1nd distractedm1nd changed the base branch from node-refactorings to main September 23, 2022 08:46
@distractedm1nd distractedm1nd changed the base branch from main to node-refactorings September 23, 2022 08:50
@distractedm1nd distractedm1nd changed the base branch from node-refactorings to main September 23, 2022 10:53
ipld/retriever.go Outdated Show resolved Hide resolved
header/p2p/subscriber.go Show resolved Hide resolved
das/daser_test.go Outdated Show resolved Hide resolved
das/daser_test.go Outdated Show resolved Hide resolved
nodebuilder/header/service.go Outdated Show resolved Hide resolved
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, those we will call Services as APIs further, might worth renaming it now, but up to you. E.g. state.API, header.API and etc

nodebuilder/share/service.go Outdated Show resolved Hide resolved
nodebuilder/header/service.go Show resolved Hide resolved
service/rpc/state.go Show resolved Hide resolved
nodebuilder/p2p/module.go Show resolved Hide resolved
Wondertan
Wondertan previously approved these changes Sep 30, 2022
@Wondertan
Copy link
Member

Hmm too many CI issues @distractedm1nd

@distractedm1nd
Copy link
Collaborator Author

All tests pass for me locally, let's see if something flakes

@distractedm1nd distractedm1nd enabled auto-merge (squash) September 30, 2022 15:04
@distractedm1nd distractedm1nd merged commit 646dd9e into celestiaorg:main Sep 30, 2022
vgonkivs pushed a commit to vgonkivs/celestia-node that referenced this pull request Oct 7, 2022
Bidon15 added a commit that referenced this pull request Oct 7, 2022
Getting address from node's account
This is a handy feature that testground's tests are already utilising in
upcoming PFD test-plans

Ref: 
#1056 #506 

External Ref: celestiaorg/test-infra#33
distractedm1nd added a commit that referenced this pull request Oct 26, 2022
distractedm1nd added a commit to distractedm1nd/celestia-node that referenced this pull request Oct 26, 2022
distractedm1nd added a commit to distractedm1nd/celestia-node that referenced this pull request Oct 26, 2022
distractedm1nd added a commit to distractedm1nd/celestia-node that referenced this pull request Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture Architecture / design-related issues area:api Related to celestia-node API
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Migrate service/<service> services to respective node sub-packages
5 participants