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

App Submission: DCRDEX #430

Closed
wants to merge 1 commit into from
Closed

Conversation

peterzen
Copy link

@peterzen peterzen commented Feb 23, 2023

@peterzen peterzen changed the title DCRDEX app submission App Submission: DCRDEX Feb 23, 2023
@nmfretz
Copy link
Contributor

nmfretz commented Mar 2, 2023

Sorry for the delay on this @peterzen. Thanks very much for submitting the DCRDEX app. Currently, for crypto-related apps, we're exclusively accepting apps that solely focus on bitcoin for the official Umbrel App Store.

That being said, you can still create a Community App Store to distribute DCRDEX to Umbrel users:
https://github.com/getumbrel/umbrel-community-app-store

I'd be happy to help with this process and can still review the code you have submitted here to make sure DCRDEX runs smoothly on Umbrel.

@peterzen
Copy link
Author

peterzen commented Mar 2, 2023

Thank you @nmfretz for looking into this app submission. We are currently working on the Community App Store, if you could please review the code that would be much appreciated.

https://github.com/peterzen/umbrel-decred-app-store

Many thanks!

@chappjc
Copy link

chappjc commented Mar 3, 2023

It might be worth noting that this application provides a neutrino-powered (privacy preserving) Bitcoin wallet, no other dependencies required. It's not a lie to say that despite the name DCRDEX, the application prioritizes it's native BTC wallet.
(DCR or any other coin is not required in any way to use this app, e.g. you can use and swap BTC with LTC)

Copy link
Contributor

@nmfretz nmfretz left a comment

Choose a reason for hiding this comment

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

Very high quality PR. Fantastic work @peterzen! I have left some questions and suggested changes. I also took a look at the community app store implementation and everything looks good there.

repo: https://github.com/decred/dcrdex
support: https://github.com/decred/dcrdex
port: 5758
icon: https://svgshare.com/i/pNb.svg
Copy link
Contributor

Choose a reason for hiding this comment

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

I get a 503 server error when trying to access this SVG. This ends up manifesting on the dashboard as an empty icon image.

Copy link
Author

Choose a reason for hiding this comment

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

Moved the files into the Github, this should fix the issue.

decred-dcrdex/umbrel-app.yml Show resolved Hide resolved
APP_PORT: 5758

web:
image: decred/dcrdex:v0.5.9
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I would include an image digest for the multi-arch build. See here for example:

image: mempool/frontend:v2.4.0@sha256:f71722f1e3abfb3d8a3df6f2a32e384d39434e311bb83d810629077b4fa6ffaf

version: "0.5.9"
tagline: Decentralized exchange built by the Decred Project
description: >-
Trade Bitcoin, Decred, Dogecoin, Dogecoin, ZCash and more peer-to-peer using atomic swaps.
Copy link
Contributor

Choose a reason for hiding this comment

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

Dogecoin is written twice here. much doge. such silly. very duplication.

decred-dcrdex/docker-compose.yml Show resolved Hide resolved
decred-dcrdex/docker-compose.yml Show resolved Hide resolved
@peterzen
Copy link
Author

peterzen commented Mar 9, 2023

Thank you for the review @nmfretz, I've implemented your suggestions in the Community App Store repo which now works correctly.

@nmfretz nmfretz closed this Mar 16, 2023
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

3 participants