Skip to content

RPC: Return external_signer in getwalletinfo #24307

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 11, 2022
Merged

RPC: Return external_signer in getwalletinfo #24307

merged 1 commit into from
Feb 11, 2022

Conversation

kristapsk
Copy link
Contributor

Add external_signer to the result object of getwalletinfo RPC which indicates whether WALLET_FLAG_EXTERNAL_SIGNER flag is set for the wallet.

ghost
ghost approved these changes Feb 10, 2022
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK b75f4c8

@S3RK
Copy link
Contributor

S3RK commented Feb 10, 2022

utACK b75f4c8

@Sjors
Copy link
Member

Sjors commented Feb 10, 2022

#21928 contains this change as well, though it's valid on its own.

@ghost
Copy link

ghost commented Feb 10, 2022

#21928 contains this change as well, though it's valid on its own.

@Sjors why not build on top of this PR?

@Sjors
Copy link
Member

Sjors commented Feb 10, 2022

@prayank23 my PR is almost a year old. But if this gets merged first I'll rebase it of course.

@kristapsk
Copy link
Contributor Author

@Sjors Didn't know about your PR. Was playing around with external signer stuff and at one moment wanted to check is that flag really set for a specific wallet and noticed I can't do that. Anyway, this one is simple change, should be easy to review, test and merge sooner.

@ghost
Copy link

ghost commented Feb 10, 2022 •
edited by ghost
Loading

@prayank23 my PR is almost a year old. But if this gets merged first I'll rebase it of course.

TBH I can't review all the things in #21928

This looks ready for review and merge :)

@fanquake fanquake requested a review from achow101 February 11, 2022 08:38
@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:

  • #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.

brunoerg
brunoerg reviewed Feb 11, 2022
View reviewed changes
Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK b75f4c8

@achow101
Copy link
Member

ACK b75f4c8

@achow101 achow101 merged commit 2d7ea20 into bitcoin:master Feb 11, 2022
@kristapsk kristapsk deleted the getwalletinfo-external_signer branch February 11, 2022 20:09
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 12, 2022
@achow101
Merge bitcoin#24307: RPC: Return external_signer in getwalletinfo
e80e1e1 
b75f4c8 RPC: Return external_signer in getwalletinfo (Kristaps Kaupe)

Pull request description:

  Add `external_signer` to the result object of `getwalletinfo` RPC which indicates whether `WALLET_FLAG_EXTERNAL_SIGNER` flag is set for the wallet.

ACKs for top commit:
  S3RK:
    utACK b75f4c8
  achow101:
    ACK b75f4c8
  prayank23:
    utACK bitcoin@b75f4c8
  brunoerg:
    utACK b75f4c8

Tree-SHA512: 066ccb97541fd4dc3d9728834645db714a3c8c93ccf29142811af4d79cfb9440a97bbb6c845434a909bc6e1775ef3737fcbb368c1f0582bc63973f6deb17a45f
@DrahtBot DrahtBot mentioned this pull request Feb 12, 2022
Closed
@luke-jr
Copy link
Member

luke-jr commented Feb 12, 2022

Seems like this should probably be a string indicating the external signer program... but I guess changing it later is okay?

@Sjors
Copy link
Member

Sjors commented Feb 15, 2022

Fine by me :-) I'll rebase.

@luke-jr that would just echo -signer though

@luke-jr
Copy link
Member

luke-jr commented Feb 15, 2022

For now, yes - but eventually it'd be ideal to have wallets with different external signers.

@kristapsk
Copy link
Contributor Author

I agree with @luke-jr, different external signers for different wallets in the future makes sense.

@Sjors
Copy link
Member

Sjors commented Feb 15, 2022 •
edited
Loading

different external signers

To clarify: different external signer programs, e.g. alternatives to HWI. You can already have multiple wallets each using a different (type of) physical device, all via HWI.

@kristapsk kristapsk mentioned this pull request Feb 16, 2022
Closed
w0xlt
w0xlt reviewed Feb 16, 2022
View reviewed changes
Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b75f4c8

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

@achow101 achow101 Awaiting requested review from achow101

3 more reviewers

@brunoerg brunoerg brunoerg left review comments

@w0xlt w0xlt w0xlt left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
RPC/REST/ZMQ Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@kristapsk @S3RK @Sjors @DrahtBot @achow101 @luke-jr @brunoerg @w0xlt