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

[Proxy] feat: implement relayer proxy full workflow #101

Merged
merged 25 commits into from
Nov 8, 2023
Merged

Conversation

red-0ne
Copy link
Contributor

@red-0ne red-0ne commented Oct 25, 2023

Summary

Human Summary

Implemented the needed parts of the RelayerProxy flow which involves:

  • Building the RelayRequest from the http.Request
  • Verifying the RelayRequest signature and session
  • Forwarding the request to the native service
  • Building the RelayReponse and signing it
  • Replying to the requester
  • Notifying the serverdRelays observable
  • Replying appropriately if any error happens

It assumes the availability BlockClient and Session query client with the appropriate SessionHeader fields (available when PRs #78 and #65 get merged)

AI Summary

Summary generated by Reviewpad on 08 Nov 23 22:31 UTC

This pull request includes various changes across multiple files in the codebase. Here's a summary of the changes:

  1. In the file "file_diff.go":

    • Updated imports to include the packages "github.com/pokt-network/poktroll/pkg/relayer" and "github.com/pokt-network/poktroll/x/supplier/types".
    • Added a new variable "rp.supplierAddress" and modified the types of two variables: "serviceEndpoints" and "server" to use types from the "relayer" package.
  2. In the file "relay_signer.go":

    • Introduced a new file that contains a method to sign the hash of the RelayResponse.
    • The method relies on the keyring and keyName to sign the payload and returns the signature.
    • There are TODO_TECHDEBT comments suggesting improvements for the method.
  3. In the file "interface.go":

    • Added new imports and defined interfaces for managing relay servers and relay sessions.
    • These interfaces provide methods for starting and stopping relay servers, retrieving served relays, and managing SMSTs.
    • Removed unused methods and modified existing type declarations.
  4. In the file "error_reply.go":

    • Introduced a new file that contains a method to generate and write JSONRPC error responses.
    • The method builds an error response based on the input error and writes it to the HTTP ResponseWriter.
    • There are TODO_TECHDEBT comments suggesting improvements for the method.
  5. In the file "other_file_diff.go":

    • Added imports for various packages.
    • Updated type declarations and added new methods.
    • Removed an unused interface.
  6. In the file "interface.go":

    • This file has been deleted. It contained interfaces for managing relay servers and relaying requests, which are no longer needed.
  7. In the file "proxy_test.go":

    • Introduced a new file that contains tests for the "relayerProxy" and its jsonRPC component.
    • There is a TODO comment suggesting adding more tests.
  8. In the file "relay.proto":

    • Made changes to import statements and uncommented a message field in the RelayRequestMetadata message and the RelayResponseMetadata message.
  9. In the file "jsonrpc.go":

    • Made changes related to implementing a JSON-RPC server for relaying requests.
    • Added imports, changed type assertions, and added new methods.
    • Modified the logic for serving relay requests and sending responses.
  10. In the file "relay_verifier.go":

    • Introduced a new file that contains a method to verify relay requests.
    • The method checks the relay request signature, session validity, and whether the relayer proxy can serve the request.
    • TODO comments suggest areas for further investigation.
  11. In the file "errors.go":

    • Made changes to define new error variables for specific error scenarios.
  12. In the file "new_file.go":

    • Added a new file that contains functions for building RelayRequest and RelayResponse objects from HTTP requests and responses.
    • The functions handle marshaling and unmarshaling of JSON data.

Please let me know if you need more information.

Issue

Type of change

Select one or more:

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (specify)

Testing

  • Run all unit tests: make go_test
  • Verify Localnet manually: See the instructions [here](TODO: add link to instructions)

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have performed a self-review of my own code
  • I have commented my code, updated documentation and left TODOs throughout the codebase

@red-0ne red-0ne added the proxy Changes related to the Proxy label Oct 25, 2023
@red-0ne red-0ne added this to the Shannon TestNet milestone Oct 25, 2023
@red-0ne red-0ne self-assigned this Oct 25, 2023
@red-0ne red-0ne requested a review from h5law October 25, 2023 22:59
pkg/relayer/proxy/error_reply.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/interface.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/errors.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/error_reply.go Show resolved Hide resolved
pkg/relayer/proxy/error_reply.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/relay_builders.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/relay_verifier.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/relay_verifier.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/relay_verifier.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/relay_verifier.go Show resolved Hide resolved
@red-0ne red-0ne requested a review from Olshansk October 26, 2023 18:40
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 left one update in a TODO comment, but otherwise really clean and g2g!

pkg/relayer/proxy/relay_verifier.go Outdated Show resolved Hide resolved
@red-0ne red-0ne changed the base branch from feat/only-proxy-implementation to main October 28, 2023 01:43
@Olshansk
Copy link
Member

@bryanchriswhite After you merge in #65, let's try to rebase & update this PR as it should be g2g.

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.

@red-0ne Please merge (and fix conflicts) with main, and will re-review then.

Please also add *_test.go files with a TODO_TEST comment where applicable explaining that we're biasing towards an E2E relay and that building test utilities for these off-chain components is a non-trivial amount of work.

@red-0ne red-0ne marked this pull request as ready for review November 8, 2023 00:09
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.

Reviewed everything with the exception of pkg/relayer/proxy/relay_verifier.go. Going to check that out tomorrow. Great work!

pkg/relayer/interface.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/relay_signer.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/relay_signer.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/jsonrpc.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/jsonrpc.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/jsonrpc.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/jsonrpc.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/jsonrpc.go Show resolved Hide resolved
pkg/relayer/proxy/proxy.go Show resolved Hide resolved
pkg/relayer/proxy/relay_signer.go Show resolved Hide resolved
@red-0ne
Copy link
Contributor Author

red-0ne commented Nov 8, 2023

This PR should be merged after #150

@red-0ne red-0ne requested a review from Olshansk November 8, 2023 18:47
@red-0ne
Copy link
Contributor Author

red-0ne commented Nov 8, 2023

@h5law , we switched from only signing the RelayResponse payload to signing the whole response (including session headers). Please make sure to reflect this in the AppGate response verification.


// SignRelayResponse is a shared method used by RelayServers to sign the relay response.
SignRelayResponse(relayResponse *types.RelayResponse) ([]byte, error)
// This method may mutate the relay request in the future, so the returned
Copy link
Member

Choose a reason for hiding this comment

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

I think it's kind of confusing that:

  1. We pass in a pointer to an object
  2. That pointer may be mutated
  3. We return it

I believe it should be one of:

  1. Pass a pointer that may be mutated
  2. Pass a value that's copied, mutated, and returned

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see... I'll revert to passing the pointer without returning and mutate it inside the function as I can't clearly foresee the impact of changing the whole workflow to not use pointers. We'll keep that question to when we start implementing the middleware capabilities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Olshansk
Copy link
Member

Olshansk commented Nov 8, 2023

@red-0ne Also, please update this PR w/ main

@red-0ne red-0ne requested a review from Olshansk November 8, 2023 22:37
@red-0ne red-0ne merged commit 6cbdfb4 into main Nov 8, 2023
7 checks passed
okdas pushed a commit that referenced this pull request Nov 14, 2024
* feat: add general proxy logic and server builder

* fix: make var names and comments more consistent

* feat: add relay verification and signature

* fix: interface assignment

* feat: implement relayer proxy full workflow

* chore: improve code comments

* chore: change native terms to proxied

* chore: address change requests

* chore: explain -32000 error code

* chore: add log message on relaying success/failure

* chore: Update AccountI/any unmarshaling comment

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

* chore: update import paths to use GitHub organization
name

* chore: address change requests

* chore: add TODO for test implementation

* chore: Refactor relay request & response methods in
RelayerProxy

* chore: Address request changes

* chore: comment about current proxy signing approach

* Merge remote-tracking branch 'origin/main' into feat/relayer-proxy

* chore: Revert relay request & response signing and
verification interface

---------

Co-authored-by: Daniel Olshansky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proxy Changes related to the Proxy
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants