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] trustless relays servicer token validation #803

Merged
merged 28 commits into from
Jun 29, 2023

Conversation

adshmh
Copy link
Contributor

@adshmh adshmh commented Jun 2, 2023

Description

Add validation of application's session tokens to the servicer.

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

  • Servicer now validates the availability of tokens in the current session before executing relays.

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)

@adshmh adshmh self-assigned this Jun 2, 2023
@reviewpad reviewpad bot added the medium Pull request is medium label Jun 2, 2023
@adshmh adshmh added utility Utility specific changes and removed medium Pull request is medium labels Jun 2, 2023
@reviewpad reviewpad bot added waiting-for-review medium Pull request is medium labels Jun 2, 2023
@adshmh adshmh changed the title Feat utility trustless relays servicer token validation [Utility] trustless relays servicer token validation Jun 2, 2023
@adshmh adshmh requested review from Olshansk and dylanlott and removed request for Olshansk June 2, 2023 21:09
@adshmh
Copy link
Contributor Author

adshmh commented Jun 5, 2023

Will update this PR shortly to add/use Persistence module placeholders for maintaining state.

@reviewpad reviewpad bot added large Pull request is large and removed medium Pull request is medium labels Jun 6, 2023
@adshmh adshmh requested a review from h5law June 6, 2023 14:03
@adshmh
Copy link
Contributor Author

adshmh commented Jun 6, 2023

@Olshansk @h5law this is now ready for review.

@adshmh
Copy link
Contributor Author

adshmh commented Jun 6, 2023

Also pushed a commit to implement the relay execution.

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

  1. This is amazing work! 🙌
  2. Thank you for taking the initiative and working on this so quickly. There is more to come ;)
  3. This almost in the right direction with the expectation of me leading you down the wrong path. Alongside all the comments, see the comment I left in utility/service/service.go and let me know if you have any more questions!

shared/modules/persistence_module.go Show resolved Hide resolved
persistence/servicer.go Show resolved Hide resolved
persistence/servicer.go Show resolved Hide resolved
runtime/configs/proto/utility_config.proto Outdated Show resolved Hide resolved
runtime/configs/proto/utility_config.proto Outdated Show resolved Hide resolved
utility/service/service.go Outdated Show resolved Hide resolved
utility/service/service.go Outdated Show resolved Hide resolved
utility/service/service.go Outdated Show resolved Hide resolved
utility/service/service.go Outdated Show resolved Hide resolved
utility/service/service.go Outdated Show resolved Hide resolved
@adshmh adshmh requested a review from Olshansk June 12, 2023 13:58
@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Patch coverage: 47.48% and project coverage change: +0.28 🎉

Comparison is base (2d4f789) 31.52% compared to head (f3bfe13) 31.80%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #803      +/-   ##
==========================================
+ Coverage   31.52%   31.80%   +0.28%     
==========================================
  Files         107      107              
  Lines        9034     9190     +156     
==========================================
+ Hits         2848     2923      +75     
- Misses       5846     5914      +68     
- Partials      340      353      +13     
Impacted Files Coverage Δ
utility/servicer/module.go 53.40% <47.48%> (-8.39%) ⬇️

... and 1 file with indirect coverage changes

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

@adshmh
Copy link
Contributor Author

adshmh commented Jun 21, 2023

@adshmh regarding the three outstanding comments, please see my replies:

* [[Utility] trustless relays servicer token validation #803 (comment)](https://github.com/pokt-network/pocket/pull/803#discussion_r1228679186)

* [[Utility] trustless relays servicer token validation #803 (comment)](https://github.com/pokt-network/pocket/pull/803#discussion_r1228679364)

* [[Utility] trustless relays servicer token validation #803 (comment)](https://github.com/pokt-network/pocket/pull/803#discussion_r1228705803)

Will take a look at the changes shortly

Thank you for the review comments. Checked and addressed all 3.

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 last-minute NITS.

This PR has been open for a while and more changes are being introduced so I'm beginning to get some PR fatigue. After the last few nits are tended to, let's merge it in and follow up in a separate PR if we find issues.

persistence/local/module.go Show resolved Hide resolved
shared/modules/persistence_module.go Outdated Show resolved Hide resolved
utility/servicer/module_test.go Outdated Show resolved Hide resolved
persistence/local/module.go Outdated Show resolved Hide resolved
utility/servicer/module.go Outdated Show resolved Hide resolved
runtime/configs/config.go Outdated Show resolved Hide resolved
runtime/configs/config.go Outdated Show resolved Hide resolved
runtime/configs/proto/servicer_config.proto Outdated Show resolved Hide resolved
@adshmh adshmh requested a review from Olshansk June 26, 2023 16:06
@adshmh
Copy link
Contributor Author

adshmh commented Jun 26, 2023

@adshmh Just a few last-minute NITS.

This PR has been open for a while and more changes are being introduced so I'm beginning to get some PR fatigue. After the last few nits are tended to, let's merge it in and follow up in a separate PR if we find issues.

@Olshansk thank you for the reminder and multiple rounds of review.
In the most recent commits I have tried to address the remaining review comments through minimum changes, even reverting a config change that had been introduced late in the PR and had not received thorough review.

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.

Just one more comment around naming & comments.

We are probably missing something, but we can merge it in and fix it later on main. We've already made A TON of progress on this!

utility/servicer/module.go Show resolved Hide resolved
@adshmh
Copy link
Contributor Author

adshmh commented Jun 28, 2023

@Olshansk sorry for the delay on this. I will address the remaining review comments within 1 day.

@adshmh adshmh requested a review from Olshansk June 29, 2023 13:58
utility/servicer/module.go Outdated Show resolved Hide resolved
utility/servicer/module.go Outdated Show resolved Hide resolved
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.

Just a couple of last-minute stylistic NITs but approving since it looks good to me!

@adshmh
Copy link
Contributor Author

adshmh commented Jun 29, 2023

Just a couple of last-minute stylistic NITs but approving since it looks good to me!

Thank you for the detailed review. The 2 remaining review comments are now addressed. I will merge once CI checks pass.

@adshmh adshmh merged commit 208ffbb into main Jun 29, 2023
8 checks passed
bryanchriswhite added a commit that referenced this pull request Jun 30, 2023
* pokt/main:
  Update E2E_FEATURE_LIST.md
  [IBC] Implement ICS-23 CommitmentProof verification for the SMT (#845)
  [Utility] trustless relays servicer token validation (#803)
bryanchriswhite added a commit that referenced this pull request Jun 30, 2023
* feat/integrate-bg-router:
  Update E2E_FEATURE_LIST.md
  [IBC] Implement ICS-23 CommitmentProof verification for the SMT (#845)
  [Utility] trustless relays servicer token validation (#803)
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.

None yet

3 participants