Add getnodeaddresses JSON-RPC support (rpc client & server)#1590
Add getnodeaddresses JSON-RPC support (rpc client & server)#1590jcvernaleo merged 1 commit intobtcsuite:masterfrom
Conversation
rpcserver.go
Outdated
| if int32(len(nodes)) < count { | ||
| count = int32(len(nodes)) | ||
| } |
There was a problem hiding this comment.
Will an extra assignment be better than traversing nodes twice?
| if int32(len(nodes)) < count { | |
| count = int32(len(nodes)) | |
| } | |
| if numOfNodes := int32(len(nodes)); numOfNodes < count { | |
| count = numOfNodes | |
| } |
There was a problem hiding this comment.
Implementations of len should be fast because slice knows its length. I do like the reduced repetition of your suggestion especially since int32 conversion is duplicated here. I'm keen to name this variable something short like nn given its narrow scope.
| } | ||
|
|
||
| // GetNodeAddresses returns data about known node addresses. | ||
| func (c *Client) GetNodeAddresses(count *int32) ([]btcjson.GetNodeAddressesResult, error) { |
There was a problem hiding this comment.
There are plenty of examples in rpcclient where each optional parameter has its own dedicated method. For example, instead of GetNodeAddresses(count *int32), we can have the following:
GetNodeAddresses()GetNodeAddressesCount(count int32)
What do you think?
There was a problem hiding this comment.
Thank you for bringing this up. After getting similar feedback in another PR I was looking around rpcclient. In RPC calls that take a single optional variable I can see many examples where a single function which takes a pointer is provided - similarly to GetNodeAddresses(count *int32)
Trying to be consistent is the best solution I think. I can change it to the non-pointer style that you're suggesting if you think that's better.
There was a problem hiding this comment.
Fine with me as long as we are consistent. 👍
lindlof
force-pushed
the
getnodeaddresses
branch
2 times, most recently
from
4270f15 to
1954064
Compare
|
@lindlof Could you please rebase this? Will have this merged tomorrow. 🙂 |
|
Rebase/conflict fix and then this is good to go in. |
Add NodeAddresses function to rpcserverConnManager interface for fetching known node addresses.
lindlof
force-pushed
the
getnodeaddresses
branch
from
1954064 to
c1b6b40
Compare
Pull Request Test Coverage Report for Build 245431911
💛 - Coveralls |
|
@onyb @jcvernaleo Sweet, rebased |
6 participants
Add NodeAddresses function to rpcserverConnManager
interface for fetching known node addresses.