Skip to content

[Utility] Local Proof of Stake CLI - RPC server [part 2/2] - Issue #112#176

Merged
deblasis merged 80 commits intopokt-network:mainfrom
deblasis:issue/utility_local_proof_of_stake_cli_RPC
Nov 11, 2022
Merged

[Utility] Local Proof of Stake CLI - RPC server [part 2/2] - Issue #112#176
deblasis merged 80 commits intopokt-network:mainfrom
deblasis:issue/utility_local_proof_of_stake_cli_RPC

Conversation

@deblasis
Copy link
Contributor

@deblasis deblasis commented Aug 25, 2022

Description

This PR introduces a simple RPC (JSONRPC) server that will be used to interact with the CLI and/or other clients

Issue

Part of Issue #112 but we decided to split the work in 2 separate PRs

Type of change

Please mark the options that are relevant.

  • New feature, functionality or library

List of changes

  • Updated node to start an RPC server if enabled via config

Testing

  • make test_all
  • LocalNet w/ all of the steps outlined in the README

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have tested my changes using the available tooling
  • If applicable, I have made corresponding changes to related local or global README
  • If applicable, I have added new diagrams using mermaid.js
  • If applicable, I have added tests that prove my fix is effective or that my feature works

@Olshansk Olshansk added core Core infrastructure - protocol related persistence Persistence specific changes priority:high client work needed to interface with the node (rpc, cli, etc..) labels Aug 27, 2022
@Olshansk Olshansk added tooling tooling to support development, testing et al and removed persistence Persistence specific changes labels Aug 27, 2022
@deblasis deblasis marked this pull request as ready for review August 29, 2022 15:04
@deblasis deblasis requested a review from a team August 29, 2022 15:04
@deblasis deblasis changed the title [WIP] [Utility] Local Proof of Stake CLI - RPC server [part 2/2] - Issue #112 [Utility] Local Proof of Stake CLI - RPC server [part 2/2] - Issue #112 Aug 29, 2022
@deblasis deblasis mentioned this pull request Aug 29, 2022
16 tasks
Copy link
Contributor

@andrewnguyen22 andrewnguyen22 left a comment

Choose a reason for hiding this comment

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

Great start. A few changes requested.

Needs README/CHANGELOG (see the structure of the other modules)

Open separate issue for specification doc

In addition, I'd like to see some tests with this

app/app.go Outdated
package app

const (
AppVersion = "RC-1.0.0.alpha" // DISCUSS (team): see app/pocket/main.go, one of the two should be a source of truth, potentially replaced at build-time as described in `docs/build/README.md`
Copy link
Contributor

Choose a reason for hiding this comment

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

@iajrz can you confirm the versioning schema?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I updated here a little while ago: https://github.com/pokt-network/pocket/blob/main/docs/build/README.md#release-tag-versioning.

The currently release is v0.0.1-pre-alpha.1 so we should use that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@iajrz bump on this

Copy link
Collaborator

@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.

Took a first round of review over everything and left a couple of open-ended comments.

Similar to the other PR, I think the scope and functionality of this PR is great once we tend to the outstanding items.

app/app.go Outdated
package app

const (
AppVersion = "RC-1.0.0.alpha" // DISCUSS (team): see app/pocket/main.go, one of the two should be a source of truth, potentially replaced at build-time as described in `docs/build/README.md`
Copy link
Collaborator

Choose a reason for hiding this comment

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

cc @okdas for context

@@ -0,0 +1,56 @@
package rpc
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of interest (since I haven't done my research), what drove the usage of these libraries/design?

V0? Ignite? Some other project? Is this just the standard way of doing thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

KISS and wanting to show where I was going fast by using mostly a codebase (V0) that you guys already are familiar with but that I don't know much about.

It's easy to switch library while I learn more about the requirements.
For example, you mentioned that it would be nice to have the ability to generate the RPC server code starting from an openAPI specification.

I am going to simplify the codebase quite a bit and also implement this nice feature (along with the Makefile command to generate/regenerate the server code whenever we'll update the spec).

The next update of this PR tomorrow will have this too together with the fixes suggested by you guys, for which I thank you both obviously.

Sneak peek:
image

@deblasis deblasis requested a review from Olshansk November 3, 2022 12:17
Copy link
Collaborator

@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.

2/2!

@andrewnguyen22
Copy link
Contributor

Looks good - just conflict resolve, a nit, and and ask from @iajrz on versioning schema


Definitely we'll need ways to retrieve transactions as well so we can envisage:

- Get a transaction by hash (**GET /v1/client/tx**)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be /v1/query/tx atleast in V0 the v1/client is for API client only (relay / dispatch / submitting transactions) while /v1/query is for blockchain data

app/app.go Outdated
package app

const (
AppVersion = "RC-1.0.0.alpha" // DISCUSS (team): see app/pocket/main.go, one of the two should be a source of truth, potentially replaced at build-time as described in `docs/build/README.md`
Copy link
Contributor

Choose a reason for hiding this comment

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

@iajrz bump on this

…_proof_of_stake_cli_RPC

Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
app/app.go Outdated
package app

const (
AppVersion = "v0.0.1-pre-alpha.1" // DISCUSS (team): see app/pocket/main.go, one of the two should be a source of truth, potentially replaced at build-time as described in `docs/build/README.md`
Copy link
Contributor

Choose a reason for hiding this comment

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

since main depends on any and everything else, directly or indirectly, version number should be provided by a non-main package. I suspect this should be the source of truth.

Corollary: this package shouldn't use many (or any?) packages from the rest of the app so that anyone can import the version information.

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 have updated accordingly and also added some pointers on how to override this at build time:

image

Useful if we also want to tag with the git-commits our official releases later on.

5ce3563

@andrewnguyen22 PTAL if you are happy about this

rpc/handlers.go Outdated
Comment on lines +45 to +47
Height: int(consensus.CurrentHeight()),
Round: int(consensus.CurrentRound()),
Step: int(consensus.CurrentStep()),
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
Height: int(consensus.CurrentHeight()),
Round: int(consensus.CurrentRound()),
Step: int(consensus.CurrentStep()),
Height: int64(consensus.CurrentHeight()),
Round: int64(consensus.CurrentRound()),
Step: int64(consensus.CurrentStep()),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in e9a742f

rpc/module.go Outdated
}

const (
RPCModuleName = "rpc"
Copy link
Contributor

Choose a reason for hiding this comment

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

since we're implementing InitializableModule, this const does not need to be public. Arguably it shouldn't be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, applied consistently across the board 9958b01

@deblasis
Copy link
Contributor Author

This, as well as the other ones, was great teamwork 🙏

image

@deblasis deblasis merged commit 9fe53d9 into pokt-network:main Nov 11, 2022
Olshansk added a commit that referenced this pull request Nov 16, 2022
… (#176)

## Description

This PR introduces a simple RPC (JSONRPC) server that will be used to interact with the CLI and/or other clients

## Issue

Part of Issue #112 but we decided to split the work in 2 separate PRs

## Type of change

Please mark the options that are relevant.

- [x] New feature, functionality or library

## List of changes

- Updated node to start an RPC server if enabled via config

## Testing

- [x] `make test_all`
- [x] [LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md) w/ all of the steps outlined in the `README`

## Checklist

- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have tested my changes using the available tooling
- [x] If applicable, I have made corresponding changes to related local or global README
- [x] If applicable, I have added new diagrams using [mermaid.js](https://mermaid-js.github.io)
- [x] If applicable, I have added tests that prove my fix is effective or that my feature works

* feat(RPC): scaffolding

Signed-off-by: Alessandro De Blasis <alex@deblasis.net>

* feat(RPC): scaffolding

* feat(RPC): scaffolding

Signed-off-by: Alessandro De Blasis <alex@deblasis.net>

* fix(RPC): updated config handling

* fix(config.proto): updated timeout type

* feat(RPC): basic RPC server with naive sync TX broadcast

* fix(RPC): fixed HTTP method for Health and Version routes

* style(RPC): format

* refactor(Utility): RPC server RoutesMap for CLI/client use

* refactor(Utility): RPC server exporting RouteKey and RoutesMap

for CLI/client use

* feat(Utility): RPC OpenApi spec and code generation

* refactor(Utility): RPC server refactoring using code generation

RPC server refactoring with code generation from openapi spec

* feat(Utility): Updated RPC spec

* feat(Utility): Regenerated RPC boilerplate and updates

* docs(Utility): RPC and node docs barebones + RPC spec notes

* refactor(Shared): RPC config defaults -changing soon,I'm centralizin

Signed-off-by: Alessandro De Blasis <alex@deblasis.net>

* fix(Utility): RPC: updated to use test_artifacts defaults

* docs(Utility): RPC and node basic docs

* chore(Utility): fixed versioning schema

* fix(Utility): RPC configs post-merge

* feat(Consensus): configOptions

* feat(P2P): configOptions

* fix(Utility): RPC fix post merge

* fix(Utility): RPC disabled by default because of TECHDEBT

* fix(RPC): test_artifacts in runtime

* fix(go.mod): tidy

* fix(RPC): added imports used in codegen files

* feat(RPC): config proto

* feat(RPC): RPCModule and noopRpcModule

* refactor(Shared): shared.Create -> shared.CreateNode

* docs(RPC): updated code organization post refactoring

* fix(RPC): gitignoring generated files

Signed-off-by: Alessandro De Blasis <alex@deblasis.net>

* refactor(Consensus): mocks with Return(nil) and not Do(...)

* docs(RPC): updated changelog versions

* fix(Makefile): generate_rpc_openapi

* fix(Makefile): fix for git clone + make develop_test

* Update rpc/v1/openapi.yaml

Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>

* Update rpc/doc/CHANGELOG.md

Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>

* Update rpc/noop_module.go

Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>

* chore(Shared): added TODOes for ValidateXXX() in modules

* docs(RPC): broadcast_tx_sync summary updated

* Update rpc/doc/README.md

Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>

* Update rpc/doc/README.md

Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>

* Update rpc/doc/README.md

Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>

* fix(Makefile): gitignoring generated files breaks tests fix

* feat(Consensus): CurrentRound() and CurrentStep()

* feat(RPC): /v1/consensus/round_state

* feat(RPC): /v1/consensus/round_state handler

* docs(Docs): Adding some more color config + raw_hex_bytes

* chore(Runtime): comment spacing

* Update runtime/docs/README.md

Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>

* chore(Consensus): SetBus mock Do(func(modules.Bus) {}) -> Return()

* docs(Pocket): changelog

* Update app/pocket/doc/README.md

Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>

* Update app/pocket/doc/README.md

Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>

* docs(RPC): changelog

* docs(RPC): docs TOC

* docs(RPC): Transports -> Endpoints

* feat(Tooling): swagger-ui

Signed-off-by: Alessandro De Blasis <alex@deblasis.net>

* docs(RPC): swagger ui link to editor and ref to make cmd

* feat(rpcServer): rpcServer is now IntegratableModule

* chore(Shared): tracking TODO (implement validations) #334

* fix(RPC): merge fix

* chore(go.mod): tidy

* docs(RPC): nit

* fix(RPC): int64 on RoundState fields

* refactor(Shared): unexporting XXModuleName

* feat(node): single source of truth for version + overridable

Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>
@jessicadaugherty
Copy link
Contributor

jessicadaugherty commented Dec 20, 2022

Note for future RPC server owners: neither the RPC spec in

https://github.com/pokt-network/pocket-core/blob/staging/doc/specs/rpc-spec.yaml
or
https://docs.pokt.network/api-docs/pokt/#/

are complete sources of truth ATM. docs/specs/rpc-spec.yaml will need to reference docs to fill in missing gaps. There is also information not included from docs/specs/rpc-spec.yaml in docs.pokt.network/api-docs/pokt.

When the RPC server is revisited in the future, we should update docs/specs/rpc-server.yaml to reflect the API docs and then update the API docs with any missing information from docs/specs/rpc-server.yaml that wasn't already included.

@Olshansk
Copy link
Collaborator

Note for future RPC server owners: neither the RPC spec in

pokt-network/pocket-core@staging/doc/specs/rpc-spec.yaml
or
docs.pokt.network/api-docs/pokt/#

are complete sources of truth ATM. docs/specs/rpc-spec.yaml will need to reference docs to fill in missing gaps. There is also information not included from docs/specs/rpc-spec.yaml in docs.pokt.network/api-docs/pokt.

When the RPC server is revisited in the future, we should update docs/specs/rpc-server.yaml to reflect the API docs and then update the API docs with any missing information from docs/specs/rpc-server.yaml that wasn't already included.

To add to the above, the current V1 in-progress RPC spec can be found at the following link at the time of writing: https://github.com/pokt-network/pocket/blob/main/rpc/v1/openapi.yaml

The RPC spec at docs.pokt.network and in the pocket-core repo is for V0, and should be used as a guiding reference for future features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

client work needed to interface with the node (rpc, cli, etc..) core Core infrastructure - protocol related tooling tooling to support development, testing et al

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants