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(grpc): add fee config to get node info api response #1561

Conversation

alidevjimmy
Copy link
Contributor

Description

This PR adds a node fee config to GetNodeInfoResponse of GetNodeInfo method.

Related issue(s)

@alidevjimmy alidevjimmy requested review from b00f and Ja7ad October 20, 2024 04:59
@Ja7ad Ja7ad requested a review from akbariandev October 20, 2024 05:18
@@ -91,7 +91,7 @@ func NewNode(genDoc *genesis.Genesis, conf *config.Config,
if conf.GRPC.BasicAuth != "" {
enableHTTPAuth = true
}
grpcServer := grpc.NewServer(conf.GRPC, st, syn, net, consMgr, walletMgr)
grpcServer := grpc.NewServer(conf.GRPC, st, syn, net, consMgr, walletMgr, conf.TxPool)
Copy link
Contributor

@Ja7ad Ja7ad Oct 20, 2024

Choose a reason for hiding this comment

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

@b00f What you think we pass just conf instead part of configuration to gRPC?

Copy link
Collaborator

@b00f b00f Oct 20, 2024

Choose a reason for hiding this comment

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

I am wondering maybe we can find a better way

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's think out of box; gRPC almost have everything. Right?
Why don't we simply give him Node.

Copy link
Contributor

@Ja7ad Ja7ad Oct 20, 2024

Choose a reason for hiding this comment

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

We can give public access to configuration via node?

Copy link
Contributor Author

@alidevjimmy alidevjimmy Oct 21, 2024

Choose a reason for hiding this comment

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

Node itself depends on gRPC

Copy link
Collaborator

Choose a reason for hiding this comment

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

Node has all modules...

Copy link
Contributor Author

@alidevjimmy alidevjimmy Oct 23, 2024

Choose a reason for hiding this comment

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

If we pass node to gRPC we will encounter cycle import.
I suggest passing conf as Javad said.

@Ja7ad
Copy link
Contributor

Ja7ad commented Oct 20, 2024

@alidevjimmy In http server you need add fee configuration to node handler.

pactus/www/http/network.go

Lines 80 to 118 in 80ea4db

func (s *Server) NodeHandler(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
res, err := s.network.GetNodeInfo(ctx,
&pactus.GetNodeInfoRequest{})
if err != nil {
s.writeError(w, err)
return
}
id, _ := hex.DecodeString(res.PeerId)
sid, _ := lp2ppeer.IDFromBytes(id)
tm := newTableMaker()
tm.addRowString("Peer ID", sid.String())
tm.addRowString("Agent", res.Agent)
tm.addRowString("Moniker", res.Moniker)
tm.addRowTime("Started at", int64(res.StartedAt))
tm.addRowString("Reachability", res.Reachability)
tm.addRowFloat64("Clock Offset", res.ClockOffset)
tm.addRowInt("Services", int(res.Services))
tm.addRowString("Services Names", res.ServicesNames)
tm.addRowString("Connection Info", "---")
tm.addRowInt("-- Total connections", int(res.ConnectionInfo.Connections))
tm.addRowInt("-- Inbound connections", int(res.ConnectionInfo.InboundConnections))
tm.addRowInt("-- Outbound connections", int(res.ConnectionInfo.OutboundConnections))
tm.addRowString("Protocols", "---")
for i, p := range res.Protocols {
tm.addRowString(fmt.Sprint(i), p)
}
tm.addRowString("Local Addresses", "---")
for i, la := range res.LocalAddrs {
tm.addRowString(fmt.Sprint(i), la)
}
s.writeHTML(w, tm.html())
}

www/grpc/proto/network.proto Show resolved Hide resolved
www/grpc/proto/network.proto Show resolved Hide resolved
www/grpc/config.go Outdated Show resolved Hide resolved
@alidevjimmy
Copy link
Contributor Author

@alidevjimmy In http server you need add fee configuration to node handler.

pactus/www/http/network.go

Lines 80 to 118 in 80ea4db

func (s *Server) NodeHandler(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
res, err := s.network.GetNodeInfo(ctx,
&pactus.GetNodeInfoRequest{})
if err != nil {
s.writeError(w, err)
return
}
id, _ := hex.DecodeString(res.PeerId)
sid, _ := lp2ppeer.IDFromBytes(id)
tm := newTableMaker()
tm.addRowString("Peer ID", sid.String())
tm.addRowString("Agent", res.Agent)
tm.addRowString("Moniker", res.Moniker)
tm.addRowTime("Started at", int64(res.StartedAt))
tm.addRowString("Reachability", res.Reachability)
tm.addRowFloat64("Clock Offset", res.ClockOffset)
tm.addRowInt("Services", int(res.Services))
tm.addRowString("Services Names", res.ServicesNames)
tm.addRowString("Connection Info", "---")
tm.addRowInt("-- Total connections", int(res.ConnectionInfo.Connections))
tm.addRowInt("-- Inbound connections", int(res.ConnectionInfo.InboundConnections))
tm.addRowInt("-- Outbound connections", int(res.ConnectionInfo.OutboundConnections))
tm.addRowString("Protocols", "---")
for i, p := range res.Protocols {
tm.addRowString(fmt.Sprint(i), p)
}
tm.addRowString("Local Addresses", "---")
for i, la := range res.LocalAddrs {
tm.addRowString(fmt.Sprint(i), la)
}
s.writeHTML(w, tm.html())
}

It's Done.

@b00f
Copy link
Collaborator

b00f commented Oct 24, 2024

Thanks @alidevjimmy but This PR only adds complexity to the code, without any clear reason. Let's close it.
In case we really need this information, we can think about it.

@b00f b00f closed this Oct 24, 2024
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.

3 participants