Feature/full wallet page #76

riverKanies · 2024-10-23T17:18:17Z

this is a rough draft, I imagine we will want to highlight snippets rather than just print out the files in full, but for now this gives a good outline

thunderbiscuit · 2024-10-23T17:59:30Z

I'm glad to see this page being built! Will try to leave a review by the end of the day.

riverKanies · 2024-10-23T18:45:41Z

@thunderbiscuit you'll also notice that there is no persistence yet. I haven't played with persistence yet, but assuming it won't work accross crates we may want to just stick with the current wallet recovery method I'm currently using for the full-wallet page. In that case it might make sense to build the CLI wallet to give a clearer picture of how all the core functionality would actually be organized in a real world context, but that's next on my list. beyond the scope of this PR

thunderbiscuit

I'm sorry this might be a lot of comments to sift through.

Overall good work on adding the new page!

thunderbiscuit · 2024-10-23T19:22:44Z

docs/cookbook/full-wallet.md

@@ -0,0 +1,43 @@
+# Full Wallet Example
+
+This page will do a walk through of 3 examples to illustrate core wallet functionality including:


I would remove the reference to the examples. The fact that this page is cut into 3 examples when you look at the code is an implementation detail (although we could have a mention of it somewhere).

Overall I'd rather this page be conceptually "1 full continuous example", even if the code snippets it pulls from are in different places.

This line could simply read:

This page will do a walk through of the core wallet functionalities, including:

I don't think I agree with that. If I'm reading docs and there are runnable examples I want to know that. we could reword it to make it clear you don't need to do that... but it's unclear to me the direction we should go here. we kinda have two options as I see it: don't link to example code and print it all in the docs, or pull only snippets into docs and link to code for dependencies, imports, etc... right now it's at a bit of a middle ground because I feel we haven't really made that decision yet. the quickstart page just does both, but the plan was to have that one be unique, so ya I would expect us to make that choice on the full wallet

thunderbiscuit · 2024-10-23T19:24:36Z

docs/cookbook/full-wallet.md

+
+This page will do a walk through of 3 examples to illustrate core wallet functionality including:
+
+- Generating seeds and keys


You can replace with generating descriptors (which is what the library focuses on). Within that you'll need keys. Seed phrases are sort of orthogonal to all of this and can be omitted here IMO.

thunderbiscuit · 2024-10-23T19:24:39Z

docs/cookbook/full-wallet.md

+This page will do a walk through of 3 examples to illustrate core wallet functionality including:
+
+- Generating seeds and keys
+- Recovering a wallet with seeds


In general you'd use "seed" here. Like Recovering a wallet with a seedphrase. But overall we should demonstrate how to recover it from descriptors.

that's just the quickstart... also idk if "recovering from descriptors" makes sense to me... end users won't be memorizing their descriptors 😅
my approach here is maybe a bit more end user focused than bdk (descriptor) focused. to me recovery from seeds is the most important function of a wallet, and it seems like a great idea to demonstrate it asap

thunderbiscuit · 2024-10-23T19:32:38Z

examples/rust/descriptors/src/main.rs

+
+fn main() -> () {
+    let mnemonic: GeneratedKey<_, Tap> =
+    Mnemonic::generate((WordCount::Words12, Language::English))


We are changing the example from working with raw randomness to direct creation of mnemonics. Mnemonics are not directly related to descriptors, and belong in a separate example that addresses the specifics of interop between mnemonics/recovery phrases and descriptors.

thunderbiscuit · 2024-10-23T19:35:44Z

docs/cookbook/keys-descriptors/descriptors.md

@@ -1,4 +1,4 @@
-# Working with Descriptors
+# Creating Seeds, Keys, and Descriptors


I think this page should keep it's original title and intent (although I also like your title in the mkdocs file Creating Descriptors.

I suggest keeping the code we currently have for this page too (although printing the public descriptor is a good addition).

ok, ya one thing here is to clarify the distinction btw pubkey and privkey descriptors. using seed phrase means we don't have to address that. but we can use the quickstart code with privkey descriptors for the full-wallet purposes... just need to note that I'd say

thunderbiscuit · 2024-10-23T19:40:39Z

examples/rust/wallet-recovery/src/main.rs

@@ -1,73 +1,74 @@
-// detailed documentation for this code can be found at https://bitcoindevkit.github.io/book-of-bdk/cookbook/quickstart/


I don't think we need a wallet recovery example in this PR, because the code presented is the same as simply creating a wallet with a given mnemonic.

thunderbiscuit · 2024-10-23T19:41:06Z

docs/cookbook/keys-descriptors/seed-phrase.md

@@ -1,38 +0,0 @@
-# From Seed Phrase to Descriptors


The purpose of this page is to address a common question/need we have seen over and over again on Discord and in private conversation, which is to convert between mnemonics and descriptors.

It is not related to wallet recovery (see comment below).

thunderbiscuit · 2024-10-23T19:43:26Z

docs/cookbook/keys-descriptors/wallet-recovery.md

@@ -0,0 +1,18 @@
+# Wallet Recovery from Seeds


This page is not needed IMO, as one needs descriptors to use and recover on BDK, and we address the mnemonic to descriptor conversions in the From Seed Phrase to Descriptors page.

mmm ok, so I guess we could move to having the user copy paste descriptors instead of seed words... then this part of the full-wallet would just basically be the quickstart but with custom descriptors (using private keys). so we could replace the recovery section on the full-wallet page with a quickstart section (the purpose being to just check balance and generate an address)

I didn't add this page, just renamed it and moved code to runnable crate

riverKanies · 2024-10-23T20:52:59Z

@thunderbiscuit the reason I split it out this way logically is because there is no persistence (yet) and seeds are the most succinct way to save the wallet. Descriptors are significantly longer, and you need two. in a sense I'm just asking the reader to copy their seeds from place to place as the easiest way to tie the three examples together to fully demonstrate transaction broadcasting (which requires requesting from a faucet and therefore cannot be done in one single action). I separated seed creation from recovery because that way recovery can also act as a check script the reader can run multiple times to verify the state they are in. I understand that the bdk project is descriptor first, but that doesn't so much make it obvious to me that copying descriptors from place to place is better than copying seeds and using consistent descriptor derivation logic.

riverKanies · 2024-10-23T21:02:29Z

thanks for looking through it man! I appreciate the feedback

most of this stuff I think warrants some further discussion, so maybe we can find some time to go over it soon. I don't expect to be able to talk at length with the rest of the team until tuesday.

If you want to go ahead and merge this and revisit these notes in a future PR, that might be ideal, since it's kindof holding me up otherwise. I think the important thing is that you gave the feedback here, so we have that; but I don't totally understand all of the feedback and I probably disagree with some of it. In many cases you're asking me to revert changes I made for a specific reason, so there's some sort of disconnect in reasoning which warrants discussion IMO. Alternatively I'd be fine with you merging this as is and then making those changes in your own PR, that way we have a diff that exclusively highlights those disconnects, which would make it extra easy to get other's opinions aswell. still kinda brainstorming the best workflow for this team dynamic. I've worked on a lot of teams with diff dynamics, but this is new. expectations are still very unclear

So ya, if we can chat soon great, otherwise maybe just merge and we can revisit (this is just a draft). I'll just keep working on what makes sense to me until we do get a chance to chat

thunderbiscuit · 2024-10-23T21:55:37Z

Yeah I think it makes sense to merge what we can and I can fix later. I agree it's not worth getting into review loops that never end.

I can tell you my 2 showstoppers, and if you fix those I'll merge:

The removal of the "From Seed Phrase to Descriptors" page and its replacement with the "Wallet Recovery from Seeds" page.
The use of mnemonics in the example code (as you mentioned above people can copy/paste the descriptor for the example to run).

Everything else is smaller and can be adjusted later.

Side note: I think for the example code for this page, you could probably make it work with 1 single full-wallet example that exits with a message like "send signet sats to address {}" when the balance is empty, and runs fully if the wallet has funds. That way people can simply run it as many times as they want and will only go so far as they are able to, while allowing you to pull snippets from a single file. Just a thought, not a requested change at all.

riverKanies · 2024-10-24T09:51:08Z

ok, I'll look closer at those showstoppers. I could make just those changes and then address the reasons I originally made them in a future PR focused on that.

with regards to putting it all in a single file: we cannot do so while also including key/seed creation, so the page has to be broken into 2 steps anyway. yes, we could combine the other two steps, and maybe we should... I'll have to think about that a bit more. I know the bdk example has a min sats check I could add back in. seemed like clutter, but could make sense.

I noticed you didn't mention trying to get on a call. I do understand that you're busy with higher priority work. but I will say that I much prefer discussing nuanced topics through audio, and I think we've amassed a significant backlog of topics to discuss at this point. but I guess we can get to that tuesday

thunderbiscuit · 2024-10-24T12:51:00Z

Oh sorry yes I meant to also say I can jump on a call Thursday (today) if you'd like!

I'll be around most of the day working on the TxBuilder API for the bindings.

riverKanies · 2024-10-24T20:35:23Z

ok after our chat I have a much better understanding of your feedback. I will do my best to address it and update the PR in the next day or so

riverKanies · 2024-10-25T14:53:21Z

@thunderbiscuit I think I maybe ended up addressing all of your feedback. Thanks again for being patient with me!

I also went ahead and used section snippets to highlight only most relevant code.

thunderbiscuit

ACK 4983809. Thanks for the work!

riverKanies added 3 commits October 23, 2024 10:03

feat: pull snippets for descriptor examples from code EXs

7df5fb6

feat: full-wallet core functionality

5e74e7f

feat: full-wallet page v1

c929773

thunderbiscuit requested changes Oct 23, 2024

View reviewed changes

riverKanies requested a review from thunderbiscuit October 25, 2024 14:56

riverKanies force-pushed the feature/full-wallet-page branch from 6cc98d6 to df514e4 Compare October 25, 2024 15:01

fix: don't push seed words in docs (it's against bdk philosophy)

4983809

riverKanies force-pushed the feature/full-wallet-page branch from df514e4 to 4983809 Compare October 25, 2024 15:07

thunderbiscuit approved these changes Oct 25, 2024

View reviewed changes

thunderbiscuit merged commit 4983809 into bitcoindevkit:master Oct 25, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/full wallet page #76

Feature/full wallet page #76

riverKanies commented Oct 23, 2024

thunderbiscuit commented Oct 23, 2024

riverKanies commented Oct 23, 2024

thunderbiscuit left a comment

thunderbiscuit Oct 23, 2024

thunderbiscuit Oct 23, 2024 •

edited

Loading

riverKanies Oct 23, 2024

thunderbiscuit Oct 23, 2024

thunderbiscuit Oct 23, 2024 •

edited

Loading

riverKanies Oct 23, 2024

thunderbiscuit Oct 23, 2024

riverKanies Oct 23, 2024

thunderbiscuit Oct 23, 2024

riverKanies Oct 23, 2024

thunderbiscuit Oct 23, 2024

thunderbiscuit Oct 23, 2024 •

edited

Loading

thunderbiscuit Oct 23, 2024

riverKanies Oct 23, 2024

riverKanies Oct 25, 2024

riverKanies commented Oct 23, 2024

riverKanies commented Oct 23, 2024

thunderbiscuit commented Oct 23, 2024 •

edited

Loading

riverKanies commented Oct 24, 2024

thunderbiscuit commented Oct 24, 2024 •

edited

Loading

riverKanies commented Oct 24, 2024

riverKanies commented Oct 25, 2024

thunderbiscuit left a comment

		@@ -0,0 +1,43 @@
		# Full Wallet Example

		This page will do a walk through of 3 examples to illustrate core wallet functionality including:


		This page will do a walk through of 3 examples to illustrate core wallet functionality including:

		- Generating seeds and keys

		@@ -1,4 +1,4 @@
		# Working with Descriptors
		# Creating Seeds, Keys, and Descriptors

		@@ -1,73 +1,74 @@
		// detailed documentation for this code can be found at https://bitcoindevkit.github.io/book-of-bdk/cookbook/quickstart/

Feature/full wallet page #76

Feature/full wallet page #76

Conversation

riverKanies commented Oct 23, 2024

thunderbiscuit commented Oct 23, 2024

riverKanies commented Oct 23, 2024

thunderbiscuit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thunderbiscuit Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thunderbiscuit Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thunderbiscuit Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riverKanies commented Oct 23, 2024

riverKanies commented Oct 23, 2024

thunderbiscuit commented Oct 23, 2024 • edited Loading

riverKanies commented Oct 24, 2024

thunderbiscuit commented Oct 24, 2024 • edited Loading

riverKanies commented Oct 24, 2024

riverKanies commented Oct 25, 2024

thunderbiscuit left a comment

Choose a reason for hiding this comment

thunderbiscuit Oct 23, 2024 •

edited

Loading

thunderbiscuit Oct 23, 2024 •

edited

Loading

thunderbiscuit Oct 23, 2024 •

edited

Loading

thunderbiscuit commented Oct 23, 2024 •

edited

Loading

thunderbiscuit commented Oct 24, 2024 •

edited

Loading