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

[Utility] Add trustless relay to CLI #778

Merged
merged 13 commits into from
Jun 20, 2023
Merged

Conversation

adshmh
Copy link
Contributor

@adshmh adshmh commented May 22, 2023

Description

Summary generated by Reviewpad on 19 Jun 23 16:17 UTC

This pull request contains various changes including updates to shared types, implementation of new validation functions, modifications to RPC documentation and API endpoint schema, implementation of a new servicer command in the CLI package, and changes to the JSON payload types. There is also a suggestion for improving the Session message and added TECHDEBT for signature field.

Issue

Part of work on #754

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

  • Added a "Relay" sub-command to CLI's "Servicer" command.

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)

@reviewpad
Copy link

reviewpad bot commented May 22, 2023

Thank you @adshmh for this first contribution!

@reviewpad reviewpad bot added the medium Pull request is medium label May 22, 2023
@Olshansk
Copy link
Member

@adshmh One of the things we discussed in today's backlog refinement is that if you need preliminary feedback on a PR that's unfinished (e.g. no tests), keep it as a draft, but add the reviewers. If you don't hear back within 48-72 hours, that's when pinging them in a discord channel is considered 👌. We're planning to add this to #773.

@adshmh adshmh requested a review from Olshansk May 25, 2023 19:30
@Olshansk Olshansk added the utility Utility specific changes label May 25, 2023
@Olshansk
Copy link
Member

@adshmh I updated the PR metadata on this. Check it out and feel free to ask questions if you don't know what to select in future PRs.

Screenshot 2023-05-25 at 1 38 25 PM

app/client/cli/actor.go Outdated Show resolved Hide resolved
app/client/cli/servicer.go Outdated Show resolved Hide resolved
app/client/cli/servicer.go Outdated Show resolved Hide resolved
app/client/cli/servicer.go Outdated Show resolved Hide resolved
app/client/cli/servicer.go Outdated Show resolved Hide resolved
app/client/cli/servicer.go Outdated Show resolved Hide resolved
app/client/cli/servicer.go Outdated Show resolved Hide resolved
app/client/cli/servicer.go Outdated Show resolved Hide resolved
app/client/cli/servicer.go Show resolved Hide resolved
app/client/cli/servicer.go Show resolved Hide resolved
@adshmh adshmh mentioned this pull request May 26, 2023
11 tasks
@Olshansk
Copy link
Member

@adshmh When this needs a review or follow up on the responses, please re-request it on GitHub so I get a notification.

Screenshot 2023-05-26 at 10 50 17 AM

@reviewpad reviewpad bot removed the medium Pull request is medium label May 28, 2023
@codecov
Copy link

codecov bot commented May 28, 2023

Codecov Report

Patch coverage: 27.50% and project coverage change: -0.10 ⚠️

Comparison is base (2d4f789) 31.52% compared to head (acec260) 31.43%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #778      +/-   ##
==========================================
- Coverage   31.52%   31.43%   -0.10%     
==========================================
  Files         107      109       +2     
  Lines        9034     9233     +199     
==========================================
+ Hits         2848     2902      +54     
- Misses       5846     5990     +144     
- Partials      340      341       +1     
Impacted Files Coverage Δ
app/client/cli/actor.go 30.04% <ø> (-0.29%) ⬇️
app/client/cli/servicer.go 18.53% <18.53%> (ø)
shared/core/types/relay.go 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@reviewpad reviewpad bot added the large Pull request is large label May 28, 2023
@adshmh adshmh marked this pull request as ready for review May 28, 2023 09:45
@adshmh adshmh requested a review from Olshansk May 28, 2023 09:59
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.

@adshmh Feel free to resolve / close out any comments where the change / suggestion is trivial or minor. There's a level of trust we got to have with others :)

Leave open comments where we need to continue the discussion.

app/client/cli/servicer.go Outdated Show resolved Hide resolved
app/client/cli/servicer.go Outdated Show resolved Hide resolved
app/client/cli/servicer.go Outdated Show resolved Hide resolved
app/client/cli/servicer.go Outdated Show resolved Hide resolved
app/client/cli/servicer.go Outdated Show resolved Hide resolved
shared/core/types/relay.go Outdated Show resolved Hide resolved
shared/core/types/relay.go Show resolved Hide resolved
shared/core/types/relay.go Outdated Show resolved Hide resolved
shared/core/types/relay_test.go Outdated Show resolved Hide resolved
shared/core/types/relay_test.go Outdated Show resolved Hide resolved
@adshmh adshmh requested a review from Olshansk June 14, 2023 19:14
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.

@adshmh There are still some comments that:

  1. You did not tend to
  2. You mentioned are resolved but are not

Can you please:

  1. Double check and resolve all outstanding comments
  2. Click "Resolve" once you have tended to a comment
  3. Push the latest changes

Re-request review once the above are taken care of.

Screenshot 2023-06-14 at 2 07 09 PM

Screenshot 2023-06-14 at 2 06 37 PM

@adshmh
Copy link
Contributor Author

adshmh commented Jun 15, 2023

@adshmh There are still some comments that:

1. You did not tend to

2. You mentioned are resolved but are not

Can you please:

1. Double check and resolve all outstanding comments

2. Click "Resolve" once you have tended to a comment

3. Push the latest changes

Re-request review once the above are taken care of.

Screenshot 2023-06-14 at 2 07 09 PM

Screenshot 2023-06-14 at 2 06 37 PM

Thank you for the reminder. All review comments are addressed now, except for items that may need further discussion.

@adshmh adshmh requested a review from Olshansk June 15, 2023 14:31
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.

@adshmh Just a few minor comments, but approving regardless.

Please see & then to them and then feel free to squash & merge.

app/client/cli/servicer.go Show resolved Hide resolved
app/client/cli/servicer.go Outdated Show resolved Hide resolved
app/client/cli/servicer.go Outdated Show resolved Hide resolved
app/client/cli/servicer.go Outdated Show resolved Hide resolved
app/client/cli/servicer.go Show resolved Hide resolved
rpc/v1/openapi.yaml Outdated Show resolved Hide resolved
shared/core/types/relay_test.go Outdated Show resolved Hide resolved
@gitguardian
Copy link

gitguardian bot commented Jun 19, 2023

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request
GitGuardian id Secret Commit Filename
5841025 Generic High Entropy Secret a8481c9 charts/pocket/templates/configmap-genesis.yaml View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

@Olshansk
Copy link
Member

Using my admin powers to bypass CI checks due to the following reasons:

Screenshot 2023-06-20 at 2 23 10 PM

@Olshansk
Copy link
Member

Tests were confirmed to pass. COdecov is being disabled. Ggshield issue is being handled in #848.

I will merged this in.

@Olshansk Olshansk merged commit a516c51 into main Jun 20, 2023
@Olshansk Olshansk deleted the utility-cli-trustless-relay branch June 20, 2023 22:17
dylanlott pushed a commit that referenced this pull request Jun 21, 2023
## Description
<!-- reviewpad:summarize:start -->
### Summary generated by Reviewpad on 19 Jun 23 16:17 UTC
This pull request contains various changes including updates to shared types, implementation of new validation functions, modifications to RPC documentation and API endpoint schema, implementation of a new servicer command in the CLI package, and changes to the JSON payload types. There is also a suggestion for improving the `Session` message and added `TECHDEBT` for signature field.
<!-- reviewpad:summarize:end -->

## Issue

Part of work on #754 

## Type of change

Please mark the relevant option(s):

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

## List of changes

- Added a "Relay" sub-command to CLI's "Servicer" command.

## 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
- [ ] `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
- [ ] [k8s LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md); if any infrastructure or configuration changes were made

## 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
- [ ] 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
- [x] 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)
bryanchriswhite added a commit that referenced this pull request Jun 22, 2023
* pokt/main:
  [Persistence] TreeStore Refactor (#756)
  [General] Ignore false positive secret uncovered by `ggshield` on main (#848)
  [Utility] Add trustless relay to CLI (#778)
  Devlog 9 (#846)
  [Configs] Cleanup private keys and genesis file (#827)
bryanchriswhite added a commit that referenced this pull request Jun 22, 2023
* pokt/main:
  [Persistence] TreeStore Refactor (#756)
  [General] Ignore false positive secret uncovered by `ggshield` on main (#848)
  [Utility] Add trustless relay to CLI (#778)
  Devlog 9 (#846)
  [Configs] Cleanup private keys and genesis file (#827)
bryanchriswhite added a commit that referenced this pull request Jun 22, 2023
* pokt/main:
  [Persistence] TreeStore Refactor (#756)
  [General] Ignore false positive secret uncovered by `ggshield` on main (#848)
  [Utility] Add trustless relay to CLI (#778)
  Devlog 9 (#846)
  [Configs] Cleanup private keys and genesis file (#827)
bryanchriswhite added a commit that referenced this pull request Jun 22, 2023
* refactor/unicast-router:
  chore: cleanup TODOs
  [Persistence] TreeStore Refactor (#756)
  [General] Ignore false positive secret uncovered by `ggshield` on main (#848)
  [Utility] Add trustless relay to CLI (#778)
  Devlog 9 (#846)
  [Configs] Cleanup private keys and genesis file (#827)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
large Pull request is large utility Utility specific changes waiting-for-review
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants