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(wallet): special-case descriptor for single-guardian instances #3821

Merged
merged 2 commits into from Dec 5, 2023

Conversation

elsirion
Copy link
Contributor

@elsirion elsirion commented Dec 3, 2023

Using p2wpkh is more efficient than a 1-of-1 multisig

Using p2wpkh is more efficient than a 1-of-1 multisig
Copy link

codecov bot commented Dec 3, 2023

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (f6f368d) 57.48% compared to head (5b9c5ac) 57.51%.
Report is 3 commits behind head on master.

Files Patch % Lines
devimint/src/main.rs 0.00% 10 Missing ⚠️
modules/fedimint-wallet-common/src/config.rs 30.76% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3821      +/-   ##
==========================================
+ Coverage   57.48%   57.51%   +0.03%     
==========================================
  Files         193      193              
  Lines       42185    42209      +24     
==========================================
+ Hits        24248    24275      +27     
+ Misses      17937    17934       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@elsirion elsirion marked this pull request as ready for review December 3, 2023 16:07
@elsirion elsirion requested a review from a team as a code owner December 3, 2023 16:07
@benthecarman
Copy link
Contributor

Could also do taproot

Are no changes needed for the spending? Does miniscript handle it all here?

@elsirion
Copy link
Contributor Author

elsirion commented Dec 3, 2023

Are no changes needed for the spending? Does miniscript handle it all here?

Yes, the beauty of descriptors and generic satisfiers! I tested it and it works, I didn't expect any problems, it's only getting easier for the satisfier.

Could also do taproot

  1. Will limit who can send to these addresses
  2. It, and thus also implementations like in rust-miniscript, are less lindy. I read and worked with the non-taproot part of rust-miniscript in the past, not at all the TR part of it, so I'm less confident in that.

@dpc
Copy link
Contributor

dpc commented Dec 4, 2023

Is there a single test anywhere that makes sure this works?

dpc
dpc previously approved these changes Dec 4, 2023
@elsirion
Copy link
Contributor Author

elsirion commented Dec 4, 2023

I added devimint CLI tests for single guardian federations (FYI @joschisan), should have done that sooner. I think it won't slow down CI much since all these tests can run in parallel (they don't take a lot of CPU once compiling is done).

Copy link
Contributor

@justinmoon justinmoon left a comment

Choose a reason for hiding this comment

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

LGTM

@elsirion elsirion added this pull request to the merge queue Dec 5, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 5, 2023
@elsirion elsirion added this pull request to the merge queue Dec 5, 2023
Merged via the queue into fedimint:master with commit bb0b06e Dec 5, 2023
22 checks passed
@elsirion elsirion deleted the 2023-12-single-guardian-p2wkh branch December 5, 2023 18:51
@justinmoon
Copy link
Contributor

Does recovery tool need to be updated to support this?

@bradleystachurski
Copy link
Member

Does recovery tool need to be updated to support this?

Capturing what we discussed in the dev call: We don't need to update the recovery tool to support, however it doesn't hurt to add a test case.

#3916

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.

None yet

5 participants