[P2P / runtime] chore: p2p connection type #450

bryanchriswhite · 2023-01-19T19:46:04Z

Description

Replaces boolean IsEmptyConnectionType with enum ConnectionType in P2PConfig to enable future support for additional connection types.

Issue

Fixes #268

Type of change

Please mark the relevant option(s):

List of changes

move ConnectionType enum into its own package to avoid a cyclic import between configs and defaults packages (i.e. configs -> defaults -> configs) in the resulting, generated go package
update makefile protogen_local target to build additional proto file and include it in the import path for runtime/configs/proto/p2p_config.proto
replace P2PConfig#IsEmptyConnectionType with P2PConfig#ConnectionType
replace DefaultP2PIsEmptyConnectionType with DefaultP2PConnectionType

Testing

make develop_test
LocalNet w/ all of the steps outlined in the README

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 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 diagrams in the corresponding README(s)
I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

…ectionType`

Olshansk · 2023-01-20T16:49:29Z

Makefile

@@ -244,7 +244,8 @@ protogen_local: go_protoc-go-inject-tag ## Generate go structures for all of the
 	$(PROTOC) -I=./shared/codec/proto      --go_out=./shared/codec      ./shared/codec/proto/*.proto

 	# Runtime
-	$(PROTOC_SHARED) -I=./runtime/configs/proto  --go_out=./runtime/configs ./runtime/configs/proto/*.proto
+	protoc --go_opt=paths=source_relative  -I=./runtime/configs/types/proto				--go_out=./runtime/configs/types	./runtime/configs/types/proto/*.proto     --experimental_allow_proto3_optional


Could we use the PROTOC_SHARED helper here?

Olshansk · 2023-01-20T16:51:55Z

runtime/configs/types/proto/connection.proto

@@ -0,0 +1,10 @@
+syntax = "proto3";


I think consolidating this with runtime/configs/proto/p2p_config.proto would be cleaner. Wdyt?

I agree that it would be simpler for P2PConfig and ConnectionTypes to be in the same package (as it was before my changes). There's a slight hangup though in the runtime/defaults package. When replacing defaults.DefaultP2PIsEmptyConnectionType with defaults.DefaultP2PConnectionType we need to import the ConnectionType enum, which causes a cyclic import (as configs/config.go is currently consuming defaults). I'm not sure there's much that can be done in the way of consolidation so long as configs is importing defaults and config.go shares a package with the ConnectionType generated proto code. We could, of course, move more of the runtime/configs package into the new one (e.g. P2PConfig). The most aggressive version of this would result in separating the all the generated protobuf into the new package. While I think, in principal, this is probably the right direction to go, it would require many more changes to account for imports of other configs proto types.

Another alternative would be to consider moving (at least) the ConnectionType enum to the shared protobuf files (e.g. shared/types/proto) as this would necessarily change its package but makes less sense in my opinion as I don't think it's a fair to characterize this enum as "shared" based on current usage.

I'm also very open to suggestions on better names for both the go and proto package (types and conn, respectively, as of 33aaafd).

Appreciate the explanation @bryanchriswhite. I understand the point around consolidation and would say that lets just keep it as you have it.

The primary goal of this issue of using enums has been achieved relatively cleanly, and we'll have a lot more opportunities to clean up the code in the future.

The name lgtm!

Olshansk · 2023-01-23T23:33:10Z

@bryanchriswhite Friendly reminder :)

Olshansk

One more minor change request. See the definition of PROTOC as to why I'm suggesting it.

The only reason I'm not approving yet is that I saw you didn't check the LocalNet checkbox of the testing section. Until we have automated E2E tests, can you confirm that works?

Makefile

Olshansk · 2023-01-24T03:11:39Z

runtime/configs/types/proto/connection.proto

@@ -0,0 +1,10 @@
+syntax = "proto3";


Appreciate the explanation @bryanchriswhite. I understand the point around consolidation and would say that lets just keep it as you have it.

The primary goal of this issue of using enums has been achieved relatively cleanly, and we'll have a lot more opportunities to clean up the code in the future.

The name lgtm!

bryanchriswhite · 2023-01-24T06:08:30Z

One more minor change request. See the definition of PROTOC as to why I'm suggesting it.

Thanks for elaborating, I was not paying close enough attention and missed the detail there. 😅

The only reason I'm not approving yet is that I saw you didn't check the LocalNet checkbox of the testing section. Until we have automated E2E tests, can you confirm that works?

No worries, regarding localnet: I made an attempt to follow the localnet instructions but lost the thread around steps 4/5:

Check the state of each node

Trigger the next view to ensure everything is working

I'm not sure what "working" looks like yet (expected behavior), is any other documentation that elaborates on this?

Co-authored-by: Daniel Olshansky <[email protected]>

Olshansk · 2023-01-24T17:15:25Z

I'm not sure what "working" looks like yet (expected behavior), is any other documentation that elaborates on this?

Discussed offline but covering here.

The gif in https://github.com/pokt-network/pocket/blob/main/docs/development/README.md is a bit outdated but that's the closest "source of truth" we have for working at the moment. Simply making sure that the height increases from the logs.

We'll automate the test and improve the docs over time, but a manual sanity check of that for now should suffice.

bryanchriswhite · 2023-01-25T08:35:56Z

@Olshansk after actually taking a second to read a bit of the makefile and get more familiar with the docker-compose file, I gave it another go and things seems to be making more sense, and indeed working. 😄 I think maybe what I was seeing before was related to timeouts but having no awareness of them (I'm blaming sensory overload 😅).

bryanchriswhite · 2023-01-25T10:30:43Z

FYI: that last force-push was me changing my mind about how I think it makes sense to handle merge conflict resolutions in changelogs genrally. In this case the upstream change had a newer date, but the same unreleased version number.

For anyone who isn't aware, the words "force-pushed" in the comment GH entry is a link to the diff between the mentioned commits:

I've collected my thoughts on the matter and opened #461.

runtime/docs/CHANGELOG.md

Co-authored-by: Daniel Olshansky <[email protected]>

Olshansk · 2023-01-25T18:00:42Z

@bryanchriswhite Make sure to squash-and-merge the PR with the Github's PR description as the commit message.

bryanchriswhite added core Core infrastructure - protocol related p2p P2P specific changes labels Jan 19, 2023

bryanchriswhite self-assigned this Jan 19, 2023

bryanchriswhite force-pushed the chore/p2p-connection-type branch 3 times, most recently from 6d12436 to 787606c Compare January 20, 2023 11:46

bryanchriswhite added 4 commits January 20, 2023 12:46

refactor: move ConnectionType to avoid cyclic import

33aaafd

chore: update makefile

9addbb0

chore: replace P2PConfig#IsEmptyConnectionType with `P2PConfig#Conn…

0b792c7

…ectionType`

chore: replace DefaultIsEmptyConnectionType with DefaultConnectionType

0fa1cd7

bryanchriswhite force-pushed the chore/p2p-connection-type branch from 787606c to 0fa1cd7 Compare January 20, 2023 11:48

bryanchriswhite marked this pull request as ready for review January 20, 2023 12:30

bryanchriswhite requested a review from Olshansk January 20, 2023 12:30

bryanchriswhite mentioned this pull request Jan 20, 2023

[Runtime] docs: fix broken link #451

Merged

16 tasks

Olshansk requested changes Jan 20, 2023

View reviewed changes

fix: use PROTOC_SHARED in makefile

49c177b

bryanchriswhite requested a review from Olshansk January 23, 2023 17:44

Olshansk added this to the M1: Pocket PoS (Proof of Stake) milestone Jan 23, 2023

Olshansk requested changes Jan 24, 2023

View reviewed changes

chore: remove redundant protoc args

246e670

Co-authored-by: Daniel Olshansky <[email protected]>

bryanchriswhite requested a review from Olshansk January 25, 2023 08:33

Merge branch 'main' into chore/p2p-connection-type

8f87f88

bryanchriswhite force-pushed the chore/p2p-connection-type branch from ab37ad5 to 8f87f88 Compare January 25, 2023 09:19

bryanchriswhite mentioned this pull request Jan 25, 2023

[Tooling][Proposal] changelog change group convention #461

Closed

9 tasks

Olshansk requested changes Jan 25, 2023

View reviewed changes

runtime/docs/CHANGELOG.md Outdated Show resolved Hide resolved

fix: revert unreleased version numbers & push date

5dfadfd

Co-authored-by: Daniel Olshansky <[email protected]>

bryanchriswhite requested a review from Olshansk January 25, 2023 17:51

Olshansk approved these changes Jan 25, 2023

View reviewed changes

bryanchriswhite merged commit e22d619 into main Jan 25, 2023

bryanchriswhite deleted the chore/p2p-connection-type branch January 25, 2023 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[P2P / runtime] chore: p2p connection type #450

[P2P / runtime] chore: p2p connection type #450

bryanchriswhite commented Jan 19, 2023 •

edited

Loading

Olshansk Jan 20, 2023

Olshansk Jan 20, 2023

bryanchriswhite Jan 23, 2023 •

edited

Loading

bryanchriswhite Jan 23, 2023

Olshansk Jan 24, 2023

Olshansk commented Jan 23, 2023

Olshansk left a comment

Olshansk Jan 24, 2023

bryanchriswhite commented Jan 24, 2023 •

edited

Loading

Olshansk commented Jan 24, 2023

bryanchriswhite commented Jan 25, 2023

bryanchriswhite commented Jan 25, 2023 •

edited

Loading

Olshansk commented Jan 25, 2023

[P2P / runtime] chore: p2p connection type #450

[P2P / runtime] chore: p2p connection type #450

Conversation

bryanchriswhite commented Jan 19, 2023 • edited Loading

Description

Issue

Type of change

List of changes

Testing

Required Checklist

If Applicable Checklist

Olshansk Jan 20, 2023

Choose a reason for hiding this comment

Olshansk Jan 20, 2023

Choose a reason for hiding this comment

bryanchriswhite Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

bryanchriswhite Jan 23, 2023

Choose a reason for hiding this comment

Olshansk Jan 24, 2023

Choose a reason for hiding this comment

Olshansk commented Jan 23, 2023

Olshansk left a comment

Choose a reason for hiding this comment

Olshansk Jan 24, 2023

Choose a reason for hiding this comment

bryanchriswhite commented Jan 24, 2023 • edited Loading

Olshansk commented Jan 24, 2023

bryanchriswhite commented Jan 25, 2023

bryanchriswhite commented Jan 25, 2023 • edited Loading

Olshansk commented Jan 25, 2023

bryanchriswhite commented Jan 19, 2023 •

edited

Loading

bryanchriswhite Jan 23, 2023 •

edited

Loading

bryanchriswhite commented Jan 24, 2023 •

edited

Loading

bryanchriswhite commented Jan 25, 2023 •

edited

Loading