chaincodelabs / bitcoin-tx-tutorial

A technical tutorial for understanding how bitcoin transactions are created and signed
105 stars 42 forks source link

Simplify notebooks' setup by using a configuration file #12

Open kouloumos opened 1 year ago

kouloumos commented 1 year ago

I believe that way that the taproot-workshop is handling the reference to the bitcoin core directory is much more elegant and simpler for the user than having to redefine it in every notebook. But this can only be done when the notebooks are in the root directory of the project.

In case there is no strong preference to the encapsulation of each chapter in its own directory, this PR allows for a single point of configuration by:

DariusParvin commented 1 year ago

Thanks @kouloumos! This way is definitely more elegant and easier to maintain going forward 👍

I noticed that the code execution outputs seem to have been removed from the notebooks. Was this intentional? I prefer having them there because you can still see the example outputs as a useful reference without having to run the scripts. What do you think?

kouloumos commented 1 year ago

I noticed that the code execution outputs seem to have been removed from the notebooks. Was this intentional?

I actually thought that those were committed by mistake 😅, that's why I tried to reset all the notebooks as part of my PR.

I prefer having them there because you can still see the example outputs as a useful reference without having to run the scripts. What do you think?

I don't have a lot of experience with jupyter notebooks, but that was the first time seeing this, that's why I got confused. I guess this is upon the author of the notebook, and because the notebooks here don't require the user to fill any blanks, it doesn't sound bad to have the outputs there. On the other hand, if this ever becomes interactive (I'm missing context on its indented usage), having the outputs might be confusing for the reader as you typically not expect the outputs to be there.

I'll push a change to restore the example outputs.

DariusParvin commented 1 year ago

Thanks @kouloumos ! Yeah the idea was the beginning parts are more instructional to show how the transaction works, then at the end it would be more interactive with an exercise. I thought it's still nice to have the beginning part as something executable though because then you can always experiment with it in your own way, as well as having it be a useful reference.

liorwavebl commented 2 months ago

I must say that this is very helpful PR! Please merge it to the main branch.

DariusParvin commented 1 month ago

hey @liorwavebl, thanks a lot for going through the tutorial and opening useful PRs! I appreciate your feedback on this PR too. At first I was hesitant to merge because I personally found it useful to review these notebooks with the outputs included for reference, but given your and @kouloumos 's feedback I am happy to merge it.

I notice though that there are some merge conflicts. If you're up for it, @liorwavebl , would you be able to have a go resolving them? then I can go ahead and merge.

kouloumos commented 1 month ago

Thanks @liorwavebl for resurfacing this. @DariusParvin, re-adding the outputs has been on my to-do list, but it ended up slipping through the cracks 😅. I'll rebase the PR by the end of the week!

DariusParvin commented 1 month ago

hey @kouloumos, no rush but just bumping in case you forgot :)