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

Electrum Endpoints and Prometheus #9

Closed
wants to merge 24 commits into from
Closed

Conversation

jeffreypicard
Copy link
Collaborator

No description provided.

@jeffreypicard jeffreypicard marked this pull request as draft July 14, 2021 09:30
This was linked to issues Jul 14, 2021
@jeffreypicard jeffreypicard changed the title Port get header Electrum Endpoints and Prometheus Jul 14, 2021
@jeffreypicard jeffreypicard linked an issue Jul 14, 2021 that may be closed by this pull request
git fetch --tags

# Get latest tag name
latestTag=$(git describe --tags `git rev-list --tags --max-count=1`)
Copy link
Member

Choose a reason for hiding this comment

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

id check the tag to make sure its "vX.X.X" here to make sure you don't get any unexpected tags

@@ -102,7 +123,18 @@ func parseArgs(searchRequest *pb.SearchRequest) *server.Args {
}

if serveCmd.Happened() {
Copy link
Member

Choose a reason for hiding this comment

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

this can be a switch statement

@@ -174,15 +213,46 @@ func main() {
ctx, cancel := context.WithTimeout(context.Background(), time.Second)
defer cancel()

log.Println(args)
if args.CmdType == server.SearchCmd {
Copy link
Member

Choose a reason for hiding this comment

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

this can be a switch statement

}

message NoParamsThisIsSilly {}
Copy link
Member

Choose a reason for hiding this comment

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

lol is this really necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, I meant to change that name, although the empty message is necessary for a function that doesn't take params in grpc.

}
*/
message BlockHeaderOutput {
string hash = 1;
Copy link
Member

Choose a reason for hiding this comment

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

@jackrobison @shyba we should review these names to make sure we use all these and they make sense for us. they are holdovers from bitcoin/electrum, but we may want to pick better ones (like nTx is not great)

rpc Search (SearchRequest) returns (Outputs) {}
rpc GetBlock (BlockRequest) returns (BlockOutput) {}
rpc GetBlockHeader (BlockRequest) returns (BlockHeaderOutput) {}
Copy link
Member

Choose a reason for hiding this comment

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

do you need both GetBlockHeader and GetHeaders?

rpc Ping (NoParamsThisIsSilly) returns (google.protobuf.StringValue) {}
rpc Version (NoParamsThisIsSilly) returns (google.protobuf.StringValue) {}
rpc Features (NoParamsThisIsSilly) returns (google.protobuf.StringValue) {}
rpc Broadcast(NoParamsThisIsSilly) returns (google.protobuf.UInt64Value) {}
Copy link
Member

Choose a reason for hiding this comment

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

What does this do? in fact it might be good to document all these methods here a bit

func (s *Server) GetBlock(ctx context.Context, blockReq *pb.BlockRequest) (*pb.BlockOutput, error) {

log.Println("In GetBlock")
rpcClient := jsonrpc.NewClientWithOpts("http://localhost"+":29245", &jsonrpc.RPCClientOpts{
Copy link
Member

Choose a reason for hiding this comment

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

the host and port should be configurable, or at the very least global constants of some sort

)

var (
myCounters = map[string]prometheus.Metric{
Copy link
Member

Choose a reason for hiding this comment

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

look at https://github.com/lbryio/odysee-api/tree/master/internal/metrics for how we do prometheus elsewhere. fyi anything you put into the internal top-level package will not be exported. so things like metrics should go there

@lbry-bot lbry-bot assigned lyoshenka and unassigned lyoshenka Jul 21, 2021
@lyoshenka lyoshenka assigned jeffreypicard and unassigned lyoshenka Jul 26, 2021
@jeffreypicard jeffreypicard removed a link to an issue Oct 5, 2021
@jeffreypicard jeffreypicard deleted the port-get-header branch October 7, 2021 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port other electrum endpoints to hub CI/CD setup
2 participants