Skip to content

RPC: Return external_signer command in getwalletinfo#24353

Closed
kristapsk wants to merge 1 commit intobitcoin:masterfrom
kristapsk:getwalletinfo-external_signer-2
Closed

RPC: Return external_signer command in getwalletinfo#24353
kristapsk wants to merge 1 commit intobitcoin:masterfrom
kristapsk:getwalletinfo-external_signer-2

Conversation

@kristapsk
Copy link
Copy Markdown
Contributor

Alternative / follow-up to #24307. As was proposed by @luke-jr (#24307 (comment)):

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.

@laanwj laanwj added the Wallet label Feb 16, 2022
@achow101
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Contributor

w0xlt commented Feb 16, 2022

I think #24307 is a better approach.

@luke-jr
Copy link
Copy Markdown
Member

luke-jr commented Feb 16, 2022

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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Contributor Author

Closing, agree this can be added later.

@kristapsk kristapsk closed this Feb 18, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Feb 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants