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

[Documentation] Add AppGateServer Docs #200

Closed
wants to merge 146 commits into from
Closed

Conversation

h5law
Copy link
Contributor

@h5law h5law commented Nov 19, 2023

Summary

Human Summary

This PR:

  • Introduces a first pass at the documentation for the appgateserver

AI Summary

Summary generated by Reviewpad on 23 Nov 23 19:32 UTC

This pull request includes changes to multiple files.

  • The file relay_fixtures_test.go has changes to the variables marshaledMinableRelaysHex and marshaledUnminableRelaysHex. These variables have been updated with new values.

  • The file server.go has changes to the function recordLocalToScalar. The error messages returned by this function have been formatted to provide more information about the types involved.

  • A new file server.md has been added to the package. It serves as comprehensive documentation for the AppGateServer component, covering various aspects of its usage and functionality.

  • The file synchronous.go in the pkg/relayer/proxy package has changes to the serveHTTP function. The changes involve using the relayRequest.Meta.SessionHeader on the relayResponse session header, along with an added log statement for debugging purposes.

  • The file diff also shows changes to the dependency versions in a separate file.

If you need further assistance, please let me know.

Issue

  • Fixes (in part) 159

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

bryanchriswhite and others added 30 commits November 9, 2023 14:41
* pokt/main:
  [Off-chain] feat: observable utils (#171)
  [Off-chain] refactor: `MapFn`s receive context arg (#170)
RelayerOption parameter
- Fixed helpers for localnet regenesis
- Added an application & supplier to the genesis file
- Initializing appMap & supplierMap in E2E tests
- Add support for the app's codec (for unmarshaling responses) in E2E tests
- Adding a placeholder for `e2e/tests/relay.feature`

---

Co-authored-by: harry <[email protected]>
* refactor: `MapFn`s receive context arg

* feat: add `MapExpand` observable operator

* refactor: `RelayerSessionsManager` to be more reactive

* chore: add godoc comment

* chore: review feedback improvements

* trigger CI
* pokt/main:
  [Relayer] refactor: simplify `RelayerSessionsManager`  (#169)
  [Test] First step for automated E2E Relay test (#167)
* pokt/main:
  [Proxy] chore: Use depinject for relayerProxy (#173)
  [Sessions] chore: Use depinject for sessions mgr construction (#175)
@h5law h5law added this to the Shannon TestNet milestone Nov 19, 2023
@h5law h5law self-assigned this Nov 19, 2023
Copy link
Contributor

@red-0ne red-0ne left a comment

Choose a reason for hiding this comment

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

Left a few chage request and questions but this seems promising 👍

pkg/appgateserver/docs/server.md Outdated Show resolved Hide resolved
pkg/appgateserver/docs/server.md Outdated Show resolved Hide resolved

### Rings

Rings are created using the application's on-chain state. This includes a list
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Rings are created using the application's on-chain state. This includes a list
Rings are created using the application's on-chain state. In gateway mode, this includes a list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will always include the gateways, it depends on whether the application has delegated or not, but if it has and it starts its own appgateserver to self sign it will still have those in its ring

}
return partialRequest.GetMethodWeighting()
}

// partiallyUnmarshalRequest unmarshals the payload into a partial request
// that contains only the fields necessary to generate an error response and
// handle accounting for the request's method.
func partiallyUnmarshalRequest(payloadBz []byte) (PartialPayload, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious to know how the RPCType is inferred when we'll have more than one

Copy link
Contributor Author

@h5law h5law Nov 20, 2023

Choose a reason for hiding this comment

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

Here will be sort of like a switch logic but instead but unmarshalling - the first example would fail if it was not a json request with at least one of those fields. If this is not the case we will get to the next type - once i have a better understanding of REST request format I will put it in it will make more sense how we add RPC types

The PartialPayload interface has a GetRPCType() method that returns the rpc type for the payload hardcoded, so all we need to do is the partial unmarshal and were good to go for accoutning and errors

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 Did not do a deep review, but left a few large overaching naming questions that'll have major diffs if we go down that path. Lmk wyt

@red-0ne PTAL as well

pkg/appgateserver/docs/server.md Show resolved Hide resolved
proto/pocket/shared/service.proto Outdated Show resolved Hide resolved
pkg/relayer/proxy/symmetric.go Outdated Show resolved Hide resolved
func NewJSONRPCServer(
// NewSymmetricServer creates a new HTTP server that listens for incoming relay
// requests and forwards them to the supported proxied service endpoint.
// It takes the serviceId, endpointUrl, and the main RelayerProxy as arguments
Copy link
Member

Choose a reason for hiding this comment

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

The types are already getting outdated (e.g. serviced is incorrect), so lets remove that and just focus on the goal of the comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Little confused here - I renamed the method in the comment and havent touched the rest. What is outdated and how?

pkg/relayer/proxy/symmetric.go Outdated Show resolved Hide resolved
pkg/partials/interface.go Outdated Show resolved Hide resolved
pkg/appgateserver/docs/server.md Outdated Show resolved Hide resolved
pkg/partials/types/rest.go Outdated Show resolved Hide resolved
@h5law h5law requested review from Olshansk and red-0ne November 22, 2023 15:53
@h5law h5law changed the title [AppGate | RelayProxy | Documentation] Add AppGateServer Docs and Update Naming of Servers [Documentation] Add AppGateServer Docs Nov 22, 2023
Base automatically changed from feat/json_rpc_structure to main November 23, 2023 19:21
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 I can appreciate merging in PRs to make building on top of main easier, but I still have not had a chance to review pkg/appgateserver/docs/server.md (the documentation here), so I don't see a reason to rush it.

If there's code in this PR that we should get in ASAP, let's split it out.

Screenshot 2023-11-26 at 5 09 10 PM

@Olshansk
Copy link
Member

Olshansk commented Dec 5, 2023

@h5law Going to review this tomorrow. Thanks for the patience!

@Olshansk
Copy link
Member

@h5law Does this need any updates?

Do you mind synching this with main after #252 is merged in and I'll re-review?

@Olshansk
Copy link
Member

Olshansk commented Jan 3, 2024

@h5law Are we going to pick this PR up again?

@h5law
Copy link
Contributor Author

h5law commented Jan 5, 2024

@Olshansk @red-0ne I will close this PR and create a new document as lots has changed since I originally made it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application Changes related to the Application actor documentation Improvements or additions to documentation gateway Changes related to the Gateway actor relayminer Changes related to the Relayminer
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants