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

feat: Working SDK with AppGateServer integration #241

Merged
merged 11 commits into from
Dec 9, 2023
Merged

feat: Working SDK with AppGateServer integration #241

merged 11 commits into from
Dec 9, 2023

Conversation

red-0ne
Copy link
Contributor

@red-0ne red-0ne commented Dec 4, 2023

Summary

Human Summary

This PR implements the needed components to have an SDK which other (external) projects could use to implement their own custom Gateways

  1. Define POKTRollSDK interface
type POKTRollSDK interface {
	GetCurrentSession(
		ctx context.Context,
		appAddress string,
		serviceId string,
	) (session []*SupplierEndpoint, err error)

	SendRelay(
		ctx context.Context,
		sessionSupplierEndpoint *SupplierEndpoint,
		request *http.Request,
	) (response *servicetypes.RelayResponse, err error)
}

Which is slightly different from what was proposed in the original document [1]

  1. Implement the NewPOKTRollSDK initialization function which takes a POKTRollSDKConfig that consists of:
  • The PocketNodeUrl used for querying the chain
  • The private key used for signing the RelayRequests. This makes it independent of Cosmos's Keyring for projects that do not rely on Cosmos
  • An optional depinject.Config Useful for testing or for projects that want to have control over their dependencies. If not provided, the SDK is initialized with the default dependencies.
  1. Implement the interface with poktrollSDK struct which takes care of:
  • Getting the current session's suppliers list in a self contained SupplierEndpoint struct which is a flattened version of a hydrated Session
  • Send a relay request to the selected endpoint
  • Verify the relay response signature
  1. Use the SDK in the AppGateServer package.
  2. Make the SDK independent from Cosmos's ClientCtx and thus cobra.Command which makes it importable from other codebases without the need to run it as a standalone binary

An open question remaining is about shipping Poktroll's protobuf generated code. Which seems to be needed when the SDK consumer does not have a local copy of the Poktroll repo.

[1] https://www.notion.so/POKTRoll-SDK-f8f320c268e14e7dabac92edb90077f5#cb56ffd591344bdb968efb10ecb5c214

AI Summary

Summary generated by Reviewpad on 08 Dec 23 21:53 UTC

This pull request includes changes to multiple files in the codebase. Here is a summary of the key changes made:

  1. The "errors.go" file in the "pkg/appgateserver" directory has been modified. Several error codes have been removed and the remaining ones have been adjusted. Please review the changes to ensure the error handling and numbering are correct.

  2. The "appgateserver.go" file in the same directory has been updated. The file now includes the implementation of the appGateServer type. Some fields related to session management, account querying, and block height have been removed and a new field sdk has been added. Please review the changes in this file.

  3. A new file named "errors.go" has been added. This file contains the definition and registration of various SDK errors related to handling relay requests and verifying their response signatures. Please review this new file.

  4. An additional file named "sdk.go" has been added to the "pkg/sdk" directory. This file includes a new Go package called "sdk" with multiple functions and data structures. It provides functions for retrieving session supplier endpoints and managing session information. Please review this new file and ensure its correctness.

  5. The "sessionquerier.go" file has been modified. The import statement has been changed and the struct now implements a new interface. Some variable names and the initialization function have also been updated. Please review these changes.

  6. Another new file named "interface.go" has been added to the "pkg/sdk" package. This file defines an interface called "POKTRollSDK" that allows gateways and applications to interact with the Pocket protocol. Please review the interface and its methods.

  7. The "accquerier.go" file in the "pkg/client/query" directory has been modified. The import statement has been updated and some variable and function names have been changed. Please review these changes.

  8. A new file called "send_relay.go" has been added to the "pkg/sdk" package. This file contains a function that sends a relay request to a supplier's endpoint and verifies the response signature. Please review this new file and its functionalities.

  9. The "session.go" file has been deleted. It contained a function for retrieving the current session for a given service. Please review this deletion.

  10. The file path and package name of the "relay_verifier.go" file have been changed. Several function and method names have also been updated. Some error messages and variable names have been modified. Please review these changes.

  11. The "services.go" file in the "pkg/client" package has been modified. A function has been updated to remove an unused variable and improve code readability.

  12. The "endpoint_selector.go" file has undergone significant changes. The import paths have been updated, some loops and conditions have been simplified, and the return statement has been changed. Please review these changes.

  13. Several import statements and functions have been added to the "deps_builder.go" file in the "pkg/sdk" directory. The logic for building the dependencies of the POKTRollSDK has been implemented. Please review these changes.

Please review all the changes thoroughly and ensure their correctness. Let me know if you have any specific questions or if there is anything else I can assist you with!

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

@red-0ne red-0ne added gateway Changes related to the Gateway actor off-chain Off-chain business logic labels Dec 4, 2023
@red-0ne red-0ne added this to the Shannon TestNet milestone Dec 4, 2023
@red-0ne red-0ne requested review from Olshansk and h5law December 4, 2023 21:44
@red-0ne red-0ne self-assigned this Dec 4, 2023
@Olshansk Olshansk requested a review from ezeike December 4, 2023 22:53
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.

Really great and clean work! I left a handful of comments, but almost all of them are just related to minor code cleanup so we should be able to get this in soon!

pkg/appgateserver/endpoint_selector.go Outdated Show resolved Hide resolved
pkg/appgateserver/server.go Show resolved Hide resolved
pkg/appgateserver/server.go Show resolved Hide resolved
pkg/appgateserver/server.go Outdated Show resolved Hide resolved
pkg/client/interface.go Outdated Show resolved Hide resolved
pkg/sdk/session.go Outdated Show resolved Hide resolved
// SupplierEndpoint is the structure that represents a supplier's endpoint
// augmented with the session's header and the supplier's address for easy
// access to the needed information when sending a relay request.
type SupplierEndpoint struct {
Copy link
Member

Choose a reason for hiding this comment

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

  1. I feel like Address and Header should go into sessionSuppliers
  2. We can remove this and use the SupplierEndpoint struct (based on the protobuf)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GetCurrentSession (now named GetSessionSupplierEndpoints) was initially returning the whole Session struct as defined in our protobufs.

The flow was that the SDK consumer gets the hydrated Session, performs its logic to pick a Session.Suppliers[i].Services[serviceId].Endpoints[j].Url then passes it to SendRelay.

But the fact that SendRelay needs more than the Url to send a relay (session header, supplier address and application address) would lead us to adopt one of with the following approaches:

  1. Pass in just the Url and let the SendRelay method match it with the relevant SupplierAddress, ApplicationAddress, ServiceId and RpcType before sending the relay.

This approach requires the Url to be unique across all the Session and forces the SendRelay method to perform that search and match at each call.

  1. Make the SDK consumer collect all the needed data while picking an Url, then pass it to SendRelay.

This makes the method to require at least two more arguments (SupplierAddress and session Header) in addition to ctx, Url and request

It puts more work and responsibility on the SDK consumer:

  • Collect the relevant info while selecting an endpoint
  • Pass it to a SendRelay method which has now a long"ish" list of arguments

The approach of having a flattened list of (to be renamed) SupplierEndpoints

  • Generated only when GetSession fetches a fresh Session which makes it benefit from the caching mechanism;
  • Returned to the SDK consumer that would only need to pick one item from it and pass it to SendRelay, avoiding him additional work, potential errors and further processing at the SendRelay level

Is the most optimal and DX friendly IMO.

We could replace sessionSuppliers.SessionEndBlockHeight by sessionSuppliers.Session and return the whole sessionSuppliers struct so we expose the hydrated Session for consumers who need it.

We should definitely rename the flattened SupplierEndpoint struct to avoid confusion with the original proto defined version.

pkg/sdk/session.go Outdated Show resolved Hide resolved
pkg/sdk/session.go Outdated Show resolved Hide resolved
pkg/sdk/session.go Outdated Show resolved Hide resolved
@red-0ne
Copy link
Contributor Author

red-0ne commented Dec 5, 2023

Addressed most of the requests and mainly waiting to settle on the "flattened" supplier endpoint subject. I left replies on the unresolved requests.

@red-0ne red-0ne requested a review from Olshansk December 5, 2023 16:45
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 In an effort to do things in parallel, I suggested a few TODOs, comments and renamings to get this over the finish line.

Please make sure all corresponding comments, variable names, etc are renamed as well, and we can always iterate on main after!

pkg/sdk/session.go Outdated Show resolved Hide resolved
pkg/sdk/session.go Outdated Show resolved Hide resolved
pkg/sdk/session.go Outdated Show resolved Hide resolved
pkg/sdk/session.go Outdated Show resolved Hide resolved
pkg/sdk/sdk.go Outdated Show resolved Hide resolved
pkg/sdk/send_relay.go Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Dec 8, 2023

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks.

@github-actions github-actions bot added devnet push-image CI related - pushes images to ghcr.io labels Dec 8, 2023
@red-0ne red-0ne requested a review from Olshansk December 8, 2023 21:51
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.

:shipit: !

@red-0ne red-0ne merged commit c80aed0 into main Dec 9, 2023
9 checks passed
@bryanchriswhite bryanchriswhite removed the push-image CI related - pushes images to ghcr.io label May 16, 2024
@github-actions github-actions bot removed the devnet label May 16, 2024
okdas pushed a commit that referenced this pull request Nov 14, 2024
* feat: Working SDK with AppGateServer integration

* chore: Address change requests

* fix: Chain depinject configs

* chore: Expose the full SessionSuppliers struct

* fix: Avoid nil sessions on GetSessionSupplierEndpoints

* chore: Explain SingleSupplierEndpoint struct usage

* fix: Rename client/event import as per main chainges

* fix: Add missing fmt package
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gateway Changes related to the Gateway actor off-chain Off-chain business logic
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants