-
Notifications
You must be signed in to change notification settings - Fork 396
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
Update tdex to v1.0.0 #754
Conversation
* Update docker-compose * Update images * Fixes * Expose dashboard port * Fixes * Update dashboard image * Update dashboard image * Get ready for being merged * Get ready for the merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this update @altafan! I have left some suggestions below for you to address before I test.
Also, are you okay if I push a commit directly to your update-tdex
branch? We need to add a pre-start hook to make sure that users who already have tdex installed receive the new ocean-data
directory with appropriate ownership when they update.
ports: | ||
- ${APP_TDEX_PORT}:${APP_TDEX_PORT} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need to map ports here, as Docker's internal DNS resolution will allow the containers in this compose file to connect to each other. Unless you are trying to expose this port on the host so that it is reachable externally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the idea is to make this service accessible from the outside world.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah okay, so in that case we need to change ${APP_TDEX_PORT}
to something other than 9090
because this port has been taken by Urbit Bitcoin Connector, meaning if a user is running both TDEX and Urbit Bitcoin Connector there will be a port clash on the host and one of the apps will break:
port: 9090 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it shouldn't be a problem at all, do you suggest any port number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about 9000
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @altafan , port 9000
is currently occupied by portainer
. To avoid conflicts, please consider using 9002
, which is the closest available option. Before selecting a port, it's a good practice to search within this repository to ensure it doesn't overlap with existing port assignments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry! I just realized I changed this for the ease of testing and it wasn't meant to be a committed change. I reverted to port 9090
🙏
Thank you @nmfretz for the review! I guess I have to give you access to my repo in order to directly push changes? Can you wait until all required changes are fixed? |
Actually, here's what I think will be needed. You can add this above the Tor Hidden Service logic in the existing set -euo pipefail
APP_DATA_DIR="$(readlink -f $(dirname "${BASH_SOURCE[0]}")/..)"
OCEAN_DATA_DIR="${APP_DATA_DIR}/ocean-data"
[ ! -d "${OCEAN_DATA_DIR}" ] && mkdir -p "${OCEAN_DATA_DIR}" && chown 1000:1000 "${OCEAN_DATA_DIR}" |
Hey @altafan can you please update the exposed daemon port to 9002: #754 (comment) Also, what would be the best way for us to test external connection in the tdex ecosystem? |
@nmfretz I made a test on a remote host, and once I installed the tdex app, I just went to |
Any news? @nmfretz |
oh sorry, @tiero and @altafan can you guys push the quick fix noted here: #754 (comment). Port 9090 clashes with an existing app. 9092 is free for example. Also, since this is an update, we can now add release notes to the umbrel-app.yml file. See Lightning app as an example: umbrel-apps/lightning/umbrel-app.yml Lines 24 to 28 in 7765871
|
@nmfretz i pushed the changes. Now the port is |
Thanks @altafan. I have made a few changes and we are nearly there:
I tested on a new install and it works well 👌. However, something breaks in the tdexd container when updating the app from the previous version. Here is the error I get from the tdexd container
Could you please take a look into this? Once this is fixed we can go live! |
@nmfretz sorry for the late reply
The reason is that the database of the prev version of tdexd must be migrated in order to be compatible with the new version. Otherwise, it could just be enough to delete the existing datadir, the daemon will create a new one at first startup and the user will restore the wallet, which seems an easier solution. |
@nmfretz let's go for the easiest way and eventually remove the datadir if existing. Can you tell me if I can add this conditional operation to some hook script? |
@tiero @altafan sorry for the delay.
For this option we'd run the risk of users clicking update without reading the release notes, so there could be a subset of users who update and don't have (or have lost) their recovery phrase, which would be pretty bad I think. Additionally we'd have to make sure that this database deletion logic doesn't run on subsequent app starts/restarts/updates, which is totally possible but adds complexity.
Yes, we can do this with a pre-start script. The benefit here is that we users won't accidently lose access to their funds. We could do something like this:
I can write this script on priority for you guys and test it out, I would just need guidance on the commands to run for migration. If I don't answer here, please feel free to DM me on X, Nostr, or Telegram! |
@nmfretz the migration is an interactive process that requires the user to enter the password to unlock the data stored in the db. Can this be a problem for the workflow? |
Guys can we merge this? People are still using the old deprecated version. Let's override the old data directory with bash? |
Would maybe make an additional App ie TDEX V1 and deprecate this one be better? So we assume user do not expect a migration being a full restore |
Hey @tiero. Alrighty, let's get this done. Without an automatic migration process on update, let's go with the following:
to:
Let me make those changes and push them here so we can all test. |
@tiero @altafan, here is the warning I have added to the top of the release notes. Does this look okay to you? We could also include a link to your Telegram channel and say something like: If you encounter any issues when recovering your wallet after update, please reach out to the official TDEX Telegram group: https://t.me/tdexnetwork If any users reach out with issues after updating, their old tdex daemon data directory will be located at: The new tdex daemon data directory is located at: Can you please take a look at this issue and see if you can recreate:I just tested updating TDEX from v0.9.1 to v1.0.1. I went through the wallet recovery process and see this error in the UI: But nothing in the networking tab, and nothing related in the container logs:
On fresh install I see a different error: |
Hey @altafan @tiero just pinging again in case you missed this comment here: #754 (comment) |
@altafan @Janaka-Steph Can you please double-check this when you got time? To test with latest version (without involving migration of db) |
@altafan @Janaka-Steph @tiero, just pinging in case this has fallen off your radar. |
I just tried to run tdex 1.0.1 in multipass VM. I encountered the same error "verification failed: signature mismatch after caveat verification". But after refreshing the page I was able to access the Tdex provider and load the wallet. |
Thanks @Janaka-Steph. Did you also try a fresh install and wallet creation where I encounter this: If you are comfortable moving forward with both issues then we can merge. |
@Janaka-Steph @tiero pinging again for the above. Thanks. |
@nmfretz I just tried and I didn't get error. It should have been fixed. Ok for merge! |
Thanks @Janaka-Steph! Clearing local storage has gotten rid of this issue for me FYI, in case anyone reaches out with a similar issue. Going live. |
🎉 Linting finished with no errors or warnings 🎉Thank you for your submission! This is an automated linter that checks for common issues in pull requests to the Umbrel App Store. Please review the linting results below and make any necessary changes to your submission. Linting Results
Legend
|
This updates TDEX from
v0.9.1
tov1.0.0
.The compose file defines a brand new
oceand
service required by the new version oftdexd
v1.0.0. Updated also the dashboard to v1.0.1.