Skip to content
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.

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

feat: introduce getXpub RPC method #4958

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

vinlim
Copy link

@vinlim vinlim commented Feb 19, 2024

This PR introduces a new getXpub RPC method to facilitate easy access to the wallet's xpub, addressing the requirements outlined in issue #4785. This method enabled various use cases, such as accounting and non-custodial services, by allowing permissioned access to the xpub. It simplifies integration processes, improves user experience, and maintains security standards.

Screen.Recording.2024-02-19.at.5.27.51.PM.mov

@kyranjamie
Copy link
Collaborator

Thanks for the PR @vinlim

If we're to allow users to get xpub, I wonder if this should just be exposed from the same API we have now, getAddresses rather than creating a new API.

@vinlim
Copy link
Author

vinlim commented Feb 20, 2024

Thanks for the input @kyranjamie .

If we're to allow users to get xpub, I wonder if this should just be exposed from the same API we have now, getAddresses rather than creating a new API.

Exposing the xpub from the same getAddresses API while being much simpler, may introduce privacy risks by potentially allowing services to track all past and future transactions of a wallet, beyond their need for the address information. Maintaining separate API methods may be better aligned with the principle of least privilege. Ensuring services access only the data they need based on the permission granted.

Also, both BTCKit & WebBTC suggest that getAddresses is called to obtain an on-chain address from the wallet, with a similar proposed response. Adding xpub information may not align with the standard/specification.

Looking forward to your thoughts on this approach.

@shivaenigma
Copy link

If leather wallet has multiple accounts, connect should ask which account to connect to

@shivaenigma
Copy link

@markmhendrickson looking to have this merged soon. Let us know how we can help to expedite the process

@markmhendrickson
Copy link
Collaborator

@kyranjamie thoughts on the above comments for proceeding here?

@markmhendrickson
Copy link
Collaborator

If leather wallet has multiple accounts, connect should ask which account to connect to

We're releasing this enhancement soon

Copy link
Collaborator

@kyranjamie kyranjamie left a comment

Choose a reason for hiding this comment

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

Sorry for the delay reviewing this work, we've been discussing it internally.

We're okay with this API, however before merging, it'd be great if you could:

  • Format code per our linting standards, prettier, eslint etc yarn lint
  • Write automated tests using Playwright. See tests/ folder for examples of how this is done

@vinlim
Copy link
Author

vinlim commented Apr 8, 2024

Sorry for the delay reviewing this work, we've been discussing it internally.

We're okay with this API, however before merging, it'd be great if you could:

  • Format code per our linting standards, prettier, eslint etc yarn lint
  • Write automated tests using Playwright. See tests/ folder for examples of how this is done

Hi @kyranjamie thanks for the feedback. As suggested, I've added a test and formatted code style per yarn lint. It's ready for another review. Please let me know if further improvement is required.

@kyranjamie
Copy link
Collaborator

kyranjamie commented Apr 10, 2024

@vinlim I've pushed your PR here to let it run properly against our CI and there are many failures. Are you able to take a look at these?

@vinlim
Copy link
Author

vinlim commented Apr 15, 2024

Hi @kyranjamie it seems I've imported the incorrect component that is resulting in the error. I've rectified it. Appreciate you'd try it again.

@cnohall
Copy link

cnohall commented Oct 17, 2024

Hello, just wondering what's the status on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants