rpcclient: Add getchaintxstats JSON-RPC client command #1571
Conversation
lindlof
changed the title
|
@jakesyl Roger! Rebased and includes |
btcjson/chainsvrcmds.go
Outdated
| // Int32OrNil defines a type for representing nullable numeric JSON-RPC parameters. | ||
| // This is a way to explicitly marshall a null param as they are dropped by default. | ||
| // | ||
| // Useful when default value is unknown client-side and null param | ||
| // needs to be included to define following params. | ||
| type Int32OrNil struct { | ||
| Value *int32 | ||
| } |
There was a problem hiding this comment.
Since the rest of the package does not introduce types like these, I don't see how it becomes necessary here. I would opt to stay consistent with the package and handle optional parameters without defining a new type. In btcjson the ...Cmd types use pointers to represent optional parameters, so in this case a *int32 will do.
There was a problem hiding this comment.
Agreed it's a bit bloaty and it's not obvious why it was needed. The reason for Int32OrNil is to allow providing the JSON null value from command line in the following format:
btcctl getchaintxstats null 0000000000000000000039128c26efb79f720e54e4f741171c41b05debde0cdf
It allows user to provide blockhash while also providing nblocks as null. Possible options using *int32:
- Define a default
nblocksnblocksshould be calculated server side with server's chain parameters- Reasonable default could be
4320which is the server default in mainnet Bitcoin Core
- Allow providing
nullfor*int32in command parsinggetchaintxstatsseems to be an exception so I avoided large changes to command parsing
- Forcing command line users to define
nblocksto be able to defineblockhash - Asking user to provide e.g.
0fornullvalue
These are the options that I found. What do you suggest?
There was a problem hiding this comment.
Hmm yeah I see the challenge here. I think the larger issue is that we don't support null for *int32 parameters, and given there are a bunch of RPC commands which have optional integer parameters, we should try to support this at some point. For now, let's stick with a *int32, and we can have a default jsonrpcdefault:"4320".
In the meantime, I'll put up an issue for this, since right now we basically just use defaults which are not null, and also don't allow providing null for *int32 in command parsing. If we eventually do have these features, we can then do a bit of a btcjson cleanup and new command implementations will be a lot more straightforward. Sound good?
There was a problem hiding this comment.
Using *int32 instead of Int32OrNil now. Adding the jsonrpcdefault didn't actually help with either command line nor rpcclient usage. The 4320 is not the real default so I didn't add any jsonrpcdefault. nblocks must now be defined to define blockhash in the JSON-RPC request.
rpcclient/chain.go
Outdated
| // the returned instance. | ||
| // | ||
| // See GetChainTxStats for the blocking version and more details. | ||
| func (c *Client) GetChainTxStatsAsync(nBlocks *int32, blockHash *chainhash.Hash) FutureGetChainTxStatsResult { |
There was a problem hiding this comment.
Unlike btcjson, the rpcclient package consistently exposes multiple method signatures to allow the user to pass optional parameters. I would suggest following that pattern for consistency reasons as well.
In this case GetChainTxStats would have no arguments, and you would implement GetChainTxStatsBlockHash, GetChainTxStatsNBlocks, GetChainTxStatsBlockHashNBlocks. There has been a similar discussion in #1589 about the design of the rpcclient package.
There was a problem hiding this comment.
rpcclient functions GetChainTxStats, GetChainTxStatsNBlocks and GetChainTxStatsNBlocksBlockHash are now provided for consistency
btcjson/cmdparse.go
Outdated
| @@ -475,7 +475,9 @@ func assignField(paramNum int, fieldName string, dest reflect.Value, src reflect | |||
| paramNum, fieldName, destBaseType) | |||
| return makeError(ErrInvalidType, str) | |||
| } | |||
| dest.Set(reflect.ValueOf(concreteVal).Elem()) | |||
| if concreteVal != nil { | |||
| dest.Set(reflect.ValueOf(concreteVal).Elem()) | |||
There was a problem hiding this comment.
If there is a case where we would panic, we should test it as well.
There was a problem hiding this comment.
I've removed this command parsing change because it was part of now removed Int32OrNil. This issue no longer relevant to this PR.
lindlof
force-pushed
the
getchaintxstats
branch
3 times, most recently
from
38656bd
to
b7ba26f
Compare
Add JSON-RPC client command