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

2022 06 24 Suredbits Wallet / Krystal Bull 1.9.2 #45

Merged
merged 5 commits into from
Jun 25, 2022

Conversation

Christewart
Copy link
Contributor

@Christewart Christewart commented Jun 24, 2022

Updates Suredbits apps to the 1.9.2 release of bitcoin-s.

On a platform note, we are capping our max memory usage now on the Suredbits Wallet. Previously we had umbrel users note that SB wallet consumes a lot of memory due to the jvm.

@Christewart
Copy link
Contributor Author

@nevets963 This should be good to merge.

For what it's worth, there seem to be a connectivity issue between the UI docker image and the backend docker image on Krystal Bull on my Umbrel. For whatever reason, this went away after ~1 hour. We don't have any networking changes for the release with Krystal Bull, the app is pretty simple. Suredbits Wallet is much more complex.

@nevets963
Copy link
Contributor

nevets963 commented Jun 24, 2022

@Christewart That sounds wierd and wasn't my experience testing just now on both AMD64 and ARM (Pi). Both apps installed and were ready to go immedaitely.

Do you have any thoughts on this? #44
In short, whether mempool is a hard requirement for Suredbits Wallet and requires the user to have that installed before installing Suredbits Wallet?

@Christewart
Copy link
Contributor Author

Do you have any thoughts on this? #44 In short, whether mempool is a hard requirement for Suredbits Wallet and requires the user to have that installed before installing Suredbits Wallet?

So from talking with @user411 this change will cause a breaking change at the UI level when we display a link to a transaction on a block explorer. We will file an issue to fix this on our repos, but I think this is a small enough bug that we can merge this

@Christewart
Copy link
Contributor Author

Christewart commented Jun 24, 2022

@nevets963 On 8ccc38c we now comment out the url entirely, as we handle the case properly when the url isn't defined. This should be ok to merge now.

More generally, how are we suppose to figure out which apps the user has installed so we could support this feature in the future? It would be nice from a privacy perspective to use the local mempool instance if the app is installed

@nevets963
Copy link
Contributor

@Christewart Currently we have no way to tell an app which apps a user has already installed (but we're thinking this through atm as we have a similar case for another app).

You can remove this feature as it could be broken for some (but perhaps working for most), keep the same as before, or we make mempool a requirement (a dependency). FWIW, it's now a pretty nice experience to install dependencies; here is when I install Lightning (LND) for example:
Screenshot 2022-06-24 at 23 44 43

Either way, I'm happy to approve the PR as I've tested on AMD64 and ARM (Pi) and both apps are working nicely.

@lukechilds Do you have some thoughts on this PR?

Copy link
Contributor

@nevets963 nevets963 left a comment

Choose a reason for hiding this comment

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

Tested on AMD64 and ARM (Pi). Approved by me.

@lukechilds lukechilds merged commit 9d23bb3 into getumbrel:master Jun 25, 2022
@lukechilds
Copy link
Member

lukechilds commented Jun 25, 2022

Looks great! @Christewart Umbrel users will be prompted to update over the next 15 minutes.

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