Skip to content

Conversation

fanquake
Copy link
Member

These are a few followups after #21467.

Rather than 3 different messages that are confusing / leak
implementation details, use a single message, that is similar to other
wallet related messages. i.e:
"Compiled without sqlite support (required for descriptor wallets)".
It's not clear why this need it's own exception class, as opposed to just
throwing std::runtime_error().
@Sjors
Copy link
Member

Sjors commented Apr 13, 2021

tACK c8f469c

Tested with a locally rebased https://github.com/bitcoin-core/gui/pull/4/files (with minimal changes).

06a0673 was added so that the GUI doesn't throw an error in the create wallet menu if HWI is configured and there's a problem. But it looks like I'm not even using that. I'll bring it back if I need it.

How did you find the includes that were needed for add missing includes to external signer code?

With regards to c8f469c it may be useful to catch HWI specific errors in the GUI in order to present more actionable advise, but we can revert if needed. For now the GUI just throws rather useless "Can't list signers" / "Sign failed" / "Can't display address" messages anyway.

@instagibbs
Copy link
Member

utACK c8f469c

straight forward cleanups

@DrahtBot
Copy link
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:

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.

@fanquake
Copy link
Member Author

How did you find the includes that were needed for add missing includes to external signer code?

Just by reading through the code and looking at std:: usage.

I agree that other code can be (re-)added if/when it's actually used.

@fanquake fanquake merged commit e7af2f3 into bitcoin:master Apr 14, 2021
@fanquake fanquake deleted the external_signer_followups branch April 14, 2021 02:09
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 14, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants