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.

Sign up for GitHub

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

Jump to bottom

[TECHDEBT] [P2P] Remove stdNetwork and simplify Network interface #266

Closed
7 tasks
deblasis opened this issue Oct 3, 2022 · 4 comments · Fixed by #703
Closed
7 tasks

[TECHDEBT] [P2P] Remove stdNetwork and simplify Network interface #266

deblasis opened this issue Oct 3, 2022 · 4 comments · Fixed by #703
Assignees
@bryanchriswhite
Labels
code health Nice to have code improvement p2p P2P specific changes
Milestone
M*: Pocket QoL (Q...

Comments

@deblasis
Copy link
Contributor

deblasis commented Oct 3, 2022

Objective

Tend for the TODO items:

./p2p/types/network.go:// TECHDEBT(olshansky): When we delete `stdnetwork` and only go with `raintree`, this interface can be simplified greatly.
./p2p/stdnetwork/network.go:// TECHDEBT(olshansky): Delete this once we are fully comfortable with RainTree moving forward.

Refine the goals / priority of this issue since it looks like it has dependencies like "being comfortable with RainTree"

Origin Document

[ Why? Issue justification and/or link to another document]

Goals

  • _REPLACE_ME: List of things that are high level ideas or goals driving the task
  • ...

Deliverable

  • _REPLACE_ME: List of things that are concrete deliverables
  • ...

Non-goals / Non-deliverables

  • _REPLACE_ME: List of things that are out of scope
  • ...

General issue deliverables

  • Update the appropriate CHANGELOG
  • Update any relevant READMEs (local and/or global)
  • Update any relevant global documentation & references
  • If applicable, update the source code tree explanation
  • If applicable, add or update a state, sequence or flowchart diagram using mermaid

[Optional] Testing Methodology

  • _REPLACE_ME: Make sure to update the testing methodology appropriately_
  • Task specific tests: make ...
  • All tests: make test_all
  • LocalNet: verify a LocalNet is still functioning correctly by following the instructions at docs/development/README.md

Creator: @deblasis
Co-Owners: @Olshansk
@deblasis deblasis added the p2p P2P specific changes label Oct 3, 2022
@deblasis deblasis self-assigned this Oct 3, 2022
@deblasis deblasis mentioned this issue Oct 3, 2022
Closed
7 tasks
@Olshansk Olshansk moved this to Backlog in V1 Dashboard Oct 4, 2022
@Olshansk
Copy link
Member

Olshansk commented Oct 4, 2022

Not a high priority, so I'm moving this to M QOL

@Olshansk Olshansk added the code health Nice to have code improvement label Dec 15, 2022
@Olshansk
Copy link
Member

@bryanchriswhite Can you review if we can close this out?

@Olshansk Olshansk removed priority:low triage It requires some decision-making at team level (it can't be worked on as it stands) labels Apr 19, 2023
@bryanchriswhite
Copy link
Contributor

bryanchriswhite commented Apr 21, 2023 •
edited
Loading

No, stdnetwork still exits and is hooked up such that it could be used but I'm not aware of any config (test or otherwise) which exercises it (i.e. UseRainTree is false).

image
image

// pocket/runtime/defaults/defaults.go
...
	DefaultP2PUseRainTree     = true
...

I think this goes along with #553.

@Olshansk
Copy link
Member

@bryanchriswhite Cool. Let's keep this open until we decide where/how we need it (if at all) and resolve this ticket if removal ends up being the final approach.

Not an urgent call to make.

@bryanchriswhite bryanchriswhite mentioned this issue Apr 27, 2023
Merged
18 tasks
bryanchriswhite added a commit that referenced this issue Apr 28, 2023
@bryanchriswhite
[P2P] chore: remove unnecessary stdnetwork package (#703)
09b2bce 
## Description

Removes the unneeded `stdnetwork` package.

## Issue

Covers removal goal of #266. The simplification goal is tracked by #553.
TLDR; should close #266.

## Type of change

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

## List of changes

- Removed unneeded `stdnetwork` package
- Removed unneeded `use_rain_tree` P2P config field

## Testing

- [ ] `make develop_test`; if any code changes were made
- [x] [Docker Compose
LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md);
if any major functionality was changed or introduced
- [x] [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any infrastructure or configuration changes were made

## Required Checklist

- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added, or updated, [`godoc` format
comments](https://go.dev/blog/godoc) on touched members (see:
[tip.golang.org/doc/comment](https://tip.golang.org/doc/comment))
- [ ] I have tested my changes using the available tooling
- [ ] I have updated the corresponding CHANGELOG

### If Applicable Checklist

- [ ] I have updated the corresponding README(s); local and/or global
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have added, or updated,
[mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding
README(s)
- [ ] I have added, or updated, documentation and
[mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*`
if I updated `shared/*`README(s)
@github-project-automation github-project-automation bot moved this from Backlog to Done in V1 Dashboard Apr 28, 2023
Olshansk pushed a commit that referenced this issue Apr 28, 2023
@bryanchriswhite @Olshansk
[P2P] chore: remove unnecessary stdnetwork package (#703)
b83efc7 
Removes the unneeded `stdnetwork` package.

Covers removal goal of #266. The simplification goal is tracked by #553.
TLDR; should close #266.

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

- Removed unneeded `stdnetwork` package
- Removed unneeded `use_rain_tree` P2P config field

- [ ] `make develop_test`; if any code changes were made
- [x] [Docker Compose
LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md);
if any major functionality was changed or introduced
- [x] [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any infrastructure or configuration changes were made

- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added, or updated, [`godoc` format
comments](https://go.dev/blog/godoc) on touched members (see:
[tip.golang.org/doc/comment](https://tip.golang.org/doc/comment))
- [ ] I have tested my changes using the available tooling
- [ ] I have updated the corresponding CHANGELOG

- [ ] I have updated the corresponding README(s); local and/or global
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have added, or updated,
[mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding
README(s)
- [ ] I have added, or updated, documentation and
[mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*`
if I updated `shared/*`README(s)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bryanchriswhite bryanchriswhite

Labels
code health Nice to have code improvement p2p P2P specific changes
Projects
V1 Dashboard
Status: Done
Milestone
M*: Pocket QoL (Quality of Life)
Development

Successfully merging a pull request may close this issue.

[P2P] chore: remove unnecessary stdnetwork package pokt-network/pocket
3 participants
@bryanchriswhite @Olshansk @deblasis