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

[CLI] Consistent config/flag parsing & common helpers #891

Merged
merged 10 commits into from
Jul 26, 2023
Merged

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Jul 7, 2023

@Reviewer

This PR may be more digestible / reviewable on a commit-by-commit basis. Commits are organized logically and any given line is only modified in a single commit, with few exceptions*.

*(In the interest of preserving the git-time-continuum 👮🚨, this applies in batches of commits between comments or reviews by humans, only once "in review")


Description

I discovered some inconsistencies in how/whether we parse the config in different subcommands.

There was also some code that turns out to be common between some existing commands and the ones I'm adding for #730.

Issue

Related:

Dependents:

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other

List of changes

  • simplified debug message broadcasting by using the p2p modules broadcast method
  • factored out common CLI helper functions
    • FetchPeerstore()
    • GetBusFromCmd()
  • ensured config and flag parsing is consistent
  • ensured a consistent identity across subsequent usages of the debug CLI

Testing

  • make develop_test; if any code changes were made
  • make test_e2e on k8s LocalNet; if any code changes were made
  • e2e-devnet-test passes tests on DevNet; if any code was changed
  • Docker Compose LocalNet; if any major functionality was changed or introduced
  • k8s LocalNet; 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 on touched members (see: 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 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)

@bryanchriswhite bryanchriswhite added code health Nice to have code improvement tooling tooling to support development, testing et al labels Jul 7, 2023
@bryanchriswhite bryanchriswhite self-assigned this Jul 7, 2023
@reviewpad reviewpad bot added the medium Pull request is medium label Jul 7, 2023
@bryanchriswhite bryanchriswhite linked an issue Jul 7, 2023 that may be closed by this pull request
10 tasks
@bryanchriswhite bryanchriswhite added the do not merge Prevent merging even with sufficient approvals label Jul 10, 2023
@bryanchriswhite bryanchriswhite changed the base branch from chore/introduce-submodule to refactor/p2p-submodules July 11, 2023 11:46
@bryanchriswhite bryanchriswhite mentioned this pull request Jul 11, 2023
20 tasks
@bryanchriswhite bryanchriswhite force-pushed the refactor/p2p-submodules branch 3 times, most recently from 3acfbc8 to 8199ab5 Compare July 11, 2023 16:43
@bryanchriswhite bryanchriswhite added push-image Build and push a container image on non-main builds e2e-devnet-test Runs E2E tests on devnet labels Jul 12, 2023
bryanchriswhite added a commit that referenced this pull request Jul 13, 2023
## @Reviewer
This PR may be more digestible / reviewable on a commit-by-commit basis.
Commits are organized logically and any given line is only modified in a
single commit, with few exceptions*.

*(In the interest of preserving the git-time-continuum
:police_officer::rotating_light:, this applies in batches of commits
between comments or reviews *by humans*, only once "in review")

---

## Description

Refactor P2P module dependencies as submodules, ultimately to support
usage P2P module usage in the CLI.

## Issue

Related:

- #730 

Dependant(s):

- #891 
- #801 
- #892 

## 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

- decoupled P2P module dependencies from Consensus module (disambiguates
modules registry names)
  - added "current_height_provider" module registry slot
  - promoted `CurrentHeightProvider` to a submodule interface type
  - added `consensusCurrentHeightProvider` implementation
- promoted `Router` interface to a submodule interface type
  - added "staked_actor_router" modules registry slot
  - added "unstaked_actor_router" modules registry slot
  - converted `backgroundRouter` implementation to submodule
  - converted `rainTreeRouter` implementation to submodule
  - simplified router base config

## Testing

- [x] `make develop_test`; if any code changes were made
- [x] `make test_e2e` on [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any code changes were made
- [x] `e2e-devnet-test` passes tests on
[DevNet](https://pocketnetwork.notion.site/How-to-DevNet-ff1598f27efe44c09f34e2aa0051f0dd);
if any code was changed
- [ ] [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

<!-- REMOVE this comment block after following the instructions
 If you added additional tests or infrastructure, describe it here.
 Bonus points for images and videos or gifs.
-->

## Required Checklist

- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] 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))
- [x] 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)

---------

Co-authored-by: Daniel Olshansky <[email protected]>
@reviewpad reviewpad bot added the medium Pull request is medium label Jul 13, 2023
@bryanchriswhite bryanchriswhite removed the do not merge Prevent merging even with sufficient approvals label Jul 13, 2023
@bryanchriswhite bryanchriswhite marked this pull request as ready for review July 13, 2023 15:13
runtime/manager.go Outdated Show resolved Hide resolved
app/client/cli/helpers/setup.go Show resolved Hide resolved
"github.com/pokt-network/pocket/shared/modules"
)

// TODO_THIS_COMMIT: add godoc comment explaining what this **is** and **is not**
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// TODO_THIS_COMMIT: add godoc comment explaining what this **is** and **is not**
// TODO_IN_THIS_COMMIT: add godoc comment explaining what this **is** and **is not**

OPTIONAL: Update build/linters/blockers.go so we capture other regexes as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Added a comment here to capture the meaning of the var

app/client/cli/debug.go Outdated Show resolved Hide resolved
@@ -210,51 +190,16 @@ func sendDebugMessage(cmd *cobra.Command, debugMsg *messaging.DebugMessage) {
}

// if the message needs to be broadcast, it'll be handled by the business logic of the message handler
//
// DISCUSS_THIS_COMMIT: The statement above is false. Using `#Send()` will only
Copy link
Member

Choose a reason for hiding this comment

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

Things have definitely changed/evolved so I'll provide some context & pointers, but would like to get your thoughts/ideas on what the right thing to do is.

For example, take this target to send a tx:

.PHONY: send_local_tx
send_local_tx: ## A hardcoded send tx to make LocalNet debugging easier
	go run -tags=debug app/client/*.go Account Send --non_interactive 00104055c00bed7c983a48aac7dc6335d7c607a7 00204737d2a165ebe4be3a7d5b0af905b0ea91d8 1000
  • We sent it to one address
  • That one address adds it to the mempool
  • It needs to be gossiped through the network

In utility/utility_message_handler.go, we handle a TxGossip like so where HandleTransaction just adds it to the local mempool.

func (u *utilityModule) HandleUtilityMessage(message *anypb.Any) error {
	switch message.MessageName() {
	case messaging.TxGossipMessageContentType:
		msg, err := codec.GetCodec().FromAny(message)
		// ...
		if txGossipMsg, ok := msg.(*typesUtil.TxGossipMessage); !ok {
			return fmt.Errorf("failed to cast message to UtilityMessage")
		} else if err := u.HandleTransaction(txGossipMsg.Tx); err != nil {
			return err
		}
		// ...
	default:
		return coreTypes.ErrUnknownMessageType(message.MessageName())
	}
	// ...
}

Our goal is: Gossip TxGossip throughout the network
Question: WHat do we change to make it happen? Account Send? s/Send/Broadcast? How we handle case messaging.TxGossipMessageContentType? Wdyt?

app/client/cli/helpers/common.go Outdated Show resolved Hide resolved
if err != nil {
return nil, err
}
// TECHDEBT(#810, #811): use `bus.GetPeerstoreProvider()` after peerstore provider
Copy link
Member

Choose a reason for hiding this comment

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

#810 is done, so are we only waiting on #811?

app/client/cli/helpers/common.go Outdated Show resolved Hide resolved
app/client/cli/helpers/common.go Show resolved Hide resolved
@h5law h5law requested a review from Olshansk July 25, 2023 09:55
@reviewpad reviewpad bot added large Pull request is large and removed medium Pull request is medium labels Jul 25, 2023
@reviewpad reviewpad bot added medium Pull request is medium and removed large Pull request is large labels Jul 25, 2023
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

@h5law One tiny lingering comment (going to merge it in myself).

When you squash & merge this to main, make sure to copy-paste your attribution below the message

Co-authored-by: harry <[email protected]>

app/client/cli/debug.go Outdated Show resolved Hide resolved
@h5law h5law merged commit 9639341 into main Jul 26, 2023
11 checks passed
@h5law h5law deleted the refactor/cli branch July 26, 2023 09:03
red-0ne pushed a commit that referenced this pull request Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Nice to have code improvement e2e-devnet-test Runs E2E tests on devnet medium Pull request is medium push-image Build and push a container image on non-main builds tooling tooling to support development, testing et al waiting-for-review
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[P2P] Peer discovery debug CLI sub-commands
3 participants