From that review club: “we try to make the RPC layer as thin as possible so functionality can be shared between RPC and QT”. This PR applies that rule to the createwallet RPC method.
There should be no behaviour change from this PR.
Questions
We ensure that refactors don’t change behaviour by having the functionality covered by tests. Where is this functionality tested?
Are all the failure modes tested? If not, why not?
Where is the warning string used?
There are several suggestions for changing the function from ryanofsky, promag and empact. Are those good suggestions?
<hugohn> in the PR, variable decelerations are moved closer to where they are used (instead of being at the top of the function), is that a Core style thing?
<jnewbery> hugohn: usually declarations are just above where the variable is used, but I think that's just convention rather than anything in the style guide
<jnewbery> I think there are some error conditions that it's quite difficult to simulate in the test framework. For example if there are file system errors
<jnewbery> ok, last question from me. There are several suggestions for changing the function from ryanofsky, promag and empact. Are those good suggestions?
<hugohn> RE: names. do you think the RPC* methods should be maybe prefixed with rpc_ or something, to avoid having the same function names everywhere? slightly confusing
<digi_james> I am unsure of the flags, some is handled before CreateWallet, some of the flag manipulation happen sin CreateWallet. The discussion in regards to consistency with QT i couldnt fully understand.
<lightlike> jnewbery: I found it interesting that the author decided not to address nits in order not to invalidate existing ACKs. Would prior ACKs really be ignored if he changed something small like enum to enum class?
<jnewbery> digi_james: the flags are persistant and stored in the wallet bdb file. I think Russ's comment was that the RPC method shouldn't really use these flags. It should just take options and pass them through as booleans to the CreateWallet() function
<achow101> digi_james: I felt that a lot of bools would have the same effect as using the wallet flags because it basically is. it's also less typing to use the flags
<jnewbery> lightlike: Yes. When reviewers ACK, they ACK the commit hash. _Any_ change to the branch changes the commit hash, so invalidates the existing ACKs
<hugohn> I do agree in principle that external-facing APIs should be more explicit regarding param names, options, etc. The optimization (turned into a bitmask) can be done at a lower level. Helps readability & debugging.
<jnewbery> what you want to check is basically an interdiff - that the diff of the pre-rebase branch is equivalent to the diff of the post-rebase branch. If it's a simple rebase, then often I'll just replicate the rebase myself and verify that I get the same end result as the author
<jnewbery> digi_james: multiprocess bitcoin is here: https://github.com/bitcoin/bitcoin/pull/10102 . Probably worth chatting to Russ if you have questions about how the wallet RPC would be implemented
<achow101> hugohn: it's kind of unintuitive and uses a bunch of default settings which waste space and time. e.g. you can't make a born-encrypted default wallet, so if you want to encrypt your wallet, you first generate 2000 keys that will never be used, encrypt it, and generate 2000 more keys
<achow101> or perhaps you don't want to use keys at all and just want a watch only wallet, well too bad, your gonna have to first have the default wallet and if you don't set your config files correctly, it will be loaded which can result in funds being sent there instead of the intented recipient of your watcho nly