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

Add CypherpunkPay. #31

Merged
merged 17 commits into from
Oct 7, 2021
Merged

Conversation

nochiel
Copy link
Contributor

@nochiel nochiel commented Aug 24, 2021

No description provided.

@nochiel
Copy link
Contributor Author

nochiel commented Aug 24, 2021

@icculp If you have time, please could you review this?

@nochiel nochiel force-pushed the cypherpunkpay branch 2 times, most recently from 0c3117a to 37745eb Compare August 24, 2021 23:35
@icculp
Copy link
Contributor

icculp commented Aug 25, 2021

Haven't run yet but so far reviewing code changes, both scripts look like they're looking for Debian release for Tor setup and CLI tutorial indicates Debian, as well as scripts README indicates Debian for at least LinodeStandup.sh. The cypherpunk setup you added is adding source for ubuntu focal. Can multiple OSes be added to the source list so the script runs on either platform? Also, it looks like cypherpunk source is limited to Debian 10 so would need a note at least indicating this addon isn't supported on Debian 9. Opened issue #32 regarding tor setup to test on Ubuntu

@@ -62,6 +62,10 @@
# SYS_SSH_IP=
# <UDF name="region" label="Timezone" oneOf="Asia/Singapore,America/Los_Angeles" default="America/Los_Angeles" example="Servers location" optional="false"/>
# REGION=
# <UDF name="cypherpunkPay" label="Install CypherPunkPay" oneOf="YES,NO" default="NO" optional="true"/>
# USE_CYPHERPUNKPAY=
# <UDF name="xpub" label="XPUB from your new CypherpunkPay wallet" default="" example="xpub5SLqN2bLY4WeYfrqh3V99Wn5UF7wqQhuSAnCnycZd8viZ1SHV4ABrG2joGZrezpR" optional="true"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

In the example tag maybe add clarification, an end user not familiar might wonder if they should be using that address as a default or if you're just showing an example of what one looks like.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that's similar to the example for the tor pub key. Maybe a link on yours to reference the wallet reqs

ex: xpub5SLqN2bLY4WeYfrqh3V99Wn5UF7wqQhuSAnCnycZd8viZ1SHV4ABrG2joGZrezpR
See https://cypherpunkpay.org/installation/bitcoin-wallet/ for additional information

I wonder if multiple lines can be added like that. If so we could add a note to the tor pubkey example too

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, looks like it can be xpub or zpub, so maybe rename to XPUB/ZPUB. I'm testing with electrum and the master public key is a zpub, and the cypherpunkpay website says can be either.

@shannona
Copy link
Contributor

shannona commented Sep 8, 2021

Committing to Debian 10 is not necessarily a bad thing. Debian 9 is pretty long-in-the-tooth. (Will look in more depth at everything soon.)

@shannona
Copy link
Contributor

I've in fact pushed the scripts over to Debian 11 as their default, so the versioning should be fine.

Apologies for this sitting for a bit, I was doing other updates. I'm now looking over this, with the hope of fully integrating it next week.

@shannona
Copy link
Contributor

shannona commented Sep 30, 2021

(That merge was to support changes to Tor ports for bitcoin services, per Peter. )

Guessing this may have been causing problems with cyerpunkpay_cause going out of place and being marked mandatory?
@nochiel
Copy link
Contributor Author

nochiel commented Oct 6, 2021

Thank you @shannona. I'm grateful for the review and for improving the script. That typo must have slipped in after I tested on Linode, while working in Docker.

and reverted the label on cypherpunkpay_lite
(still looking for why cypherpunkpay_cause is not showing up right in Linode creation script.
@shannona
Copy link
Contributor

shannona commented Oct 6, 2021

Sure thing! Just a few more iterations and we should be able to merge.

We apparently do need a label or we just get an unlabeled "YES/NO".
@shannona shannona self-requested a review October 6, 2021 00:42
needed the installation of cyperpunkpay to be a bit less quiet
(Matching Stackscript changes)
@shannona
Copy link
Contributor

shannona commented Oct 6, 2021

Testing of Stackscript with cypherpunkpay turned off: works fine (that's the default setup).

However I haven't yet gotten it to successfully install cypherpunkpay, apparently because the USE_CYPHERPUNKPAY variable isn't coming through? I need to investigate that more and will continue in the morning.

@shannona
Copy link
Contributor

shannona commented Oct 7, 2021

Testing now successful on both Linode & script with cypherpunkpay both on and off.

Copy link
Contributor

@shannona shannona left a comment

Choose a reason for hiding this comment

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

Made various changes, and now both script and stackscript should be working properly on Debian 11, which we're now listing as our suggested software.

@shannona
Copy link
Contributor

shannona commented Oct 7, 2021

Thanks, @nochiel !

@shannona shannona merged commit 8d71d86 into BlockchainCommons:master Oct 7, 2021
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.

3 participants