RPC: Return external_signer command in getwalletinfo #24353

pull kristapsk wants to merge 1 commits into bitcoin:master from kristapsk:getwalletinfo-external_signer-2 changing 2 files +7 −4

kristapsk commented at 8:39 AM on February 16, 2022: contributor

Alternative / follow-up to #24307. As was proposed by @luke-jr (https://github.com/bitcoin/bitcoin/pull/24307#issuecomment-1037577925):

Seems like this should probably be a string indicating the external signer program...

"external_signer" is changed from bool to optional string and is returned only if WALLET_FLAG_EXTERNAL_SIGNER is set. Will return empty string if wallet is configured to use external signer but Core is started without -signer.
RPC: Return external_signer command in getwalletinfo 81e1bbcf79
laanwj added the label Wallet on Feb 16, 2022
achow101 commented at 5:29 PM on February 16, 2022: member

This doesn't seem like it's really useful, and perhaps it can be misleading? The value will change depending on what -signer is set to, and it will always be the same for all wallets. Furthermore, because -signer is an explicit option, I would expect the user knows that information already. This could also be misleading and cause people to think that the value there is stored in the wallet itself rather than a startup option.
w0xlt commented at 11:20 PM on February 16, 2022: contributor

I think #24307 is a better approach.
luke-jr commented at 11:43 PM on February 16, 2022: member

This could also be misleading and cause people to think that the value there is stored in the wallet itself rather than a startup option.

IMO it should be the former eventually
kristapsk commented at 5:26 AM on February 17, 2022: contributor

Alternative approach would be to close this, keep the way of #24307 and later add another return value 'signer_program" to result object of getwalletinfo. Otherwise, if "external_signer" has to be changed at some point in the future, better to do it sooner, not introduce incompatible RPC change later.
DrahtBot commented at 8:35 AM on February 17, 2022: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #21928 (wallet: allow toggling external_signer flag by Sjors)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
achow101 commented at 10:26 PM on February 17, 2022: member

This could also be misleading and cause people to think that the value there is stored in the wallet itself rather than a startup option.

IMO it should be the former eventually

Sure, but I think we should only add this output at that time rather than now.
kristapsk commented at 5:58 AM on February 18, 2022: contributor

Closing, agree this can be added later.
kristapsk closed this on Feb 18, 2022
DrahtBot locked this on Feb 18, 2023

Contributors

Labels

Linked (view graph)

#24307 RPC: Return external_signer in getwalletinfo

RPC: Return external_signer command in getwalletinfo #24353

Conflicts