[DEP] Add Cancel Transaction API #23

ARK4579 · 2024-04-27T14:47:38Z

Added Cancel Transaction API which takes sr_id to find and remove an already scheduled transaction.

Note: Please note that currently this API only removes transaction from our db and doesn't touch electrum wallet.

DEP:

this also includes code from other PRs, we can either review and merge this one PR and close previous ones or review those first and then create a new PR for this one.

…debase. also setup vscode settings.json file.

formatting updates

Add regtest net support

…rvice

docker compose setup

ErfanMN · 2024-07-05T09:47:56Z

I am going to review this, but first, I have some questions:

What is the primary purpose of this PR? It seems that we are just removing the transaction from our database. What is the impact of this change on the overall system, especially since it doesn't touch the Electrum wallet?
you mentioned dependencies on other PRs. Can you specify which PRs these are and how they relate to the changes in this one?
How does this cancellation process affect any dependent systems or subsequent transactions? Are there any scenarios where this could cause inconsistencies?

ARK4579 · 2024-07-05T10:17:28Z

While waiting for review on PRs #18 #21 #22, I merged those branches into master branch on my forked repo. so this PR builds on those PRs. i.e. If we review this PR we won't have to review other PRs and they can be closed instead.

Purpose of this PR:
1. Linting & Formatting updates #18 Linting and formatting updates along with a vscode settings file so all developers have have consistent development experience.
2. [DEP] Add Regtest net support #21 Added Regtest Support so we can develop and test without waiting for testnet to confirm transactions
3. [DEP] Add docker compose for easy setup #22 Added Docker support for easier setup
4. [DEP] Add Cancel Transaction API #23 As mentioned above, added endpoint to cancel a transaction
Cancelling a transaction here refers to deleting a scheduled transaction that's in our sqlite db and not one in BTC network.
This scheduled transaction could be in 3 states.
1. First, when corresponding transaction in bitcoin network is not yet created. this could be because of Insufficient balance, time remaining till next attempt or some other error. Removing transaction in this state should not effect any dependent systems negatively. Instead it frees up the transaction amount to be used to schedule other transactions.
2. Second, it could be that transaction is not queued in Bitcoin Network too but not confirmed yet.
3. Third would be when transaction in BTC network is confirmed. Removing in second and third stage won't effect dependent system as the transaction would already be in BTC network and corresponding balance updated accordingly.

ErfanMN

in overall I think linting will be a good idea. make the code always cleaner. the logic of the cancel function is also good.

ErfanMN · 2024-07-05T13:31:37Z

.vscode/settings.json

+        "debug.log*": true,
+        "wallet_service_db": true,
+    }
+}


I don't think this be a good Idea. as a developer I like to see all the folders and configs in my vs code. I was trying to run your code but was confused at first where is my venv folder!!

I don't recommend to use a settings.json for vs code as every developer may have different desire for his/her work space.

I agree with everyone having their own preferences. But, I think we should have settings.json added with must have settings for that repo. e.g. without settings.json some might have a warning not ignored and be annoyed when someone else checks in their code having that particular warning. or having different line length and ending up with un-wanted auto-formatting changes because of that.
But yeah, settings that have to do with appearance can be moved to user preference. I will make those changes.

ErfanMN · 2024-07-05T13:43:37Z

config.ini.sample

@@ -1,6 +1,7 @@
 [SYSTEM]
 wallet_dir = wallets
 use_testnet = True
+use_regtest = False


use_testnet and use_regtest are mutually exclusive. it means we cannot set both of them True or both of them False. So it is better to define one variable for this purpose. e.g mode = regtest/testnet

[I will explain the issue it may have in the related code]

ErfanMN · 2024-07-05T13:46:09Z

electrum_cmd_util.py

@@ -23,9 +24,17 @@ def __init__(self):
    self.network = None
    if self.config['SYSTEM']['use_testnet'] == 'True':
      electrum.constants.set_testnet()
+    if self.config['SYSTEM']['use_regtest'] == 'True':


what if somebody set use_testnet and use_regtest == True by mistake! it will be overwritten.

ErfanMN · 2024-07-05T13:49:58Z

docker/Dockerfile

+RUN pip install -e Electrum-4.2.1/.
+
+# set electrum config parameters
+RUN export PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python


what is this for ? please add a comment for this line and explain it.

Instead of using RUN export, use the ENV directive for setting environment variables.
best practice here is using ENV.

ErfanMN · 2024-07-05T13:51:00Z

docker/Dockerfile

how much is the final image size? is it large ?

ErfanMN · 2024-07-05T13:54:57Z

docker/docker-compose.yml

I am not a docker expert but as you have dependency between your services, consider adding health checks for services to ensure they are ready before dependent services start to avoid race condition. this is optional and up to you.

ErfanMN · 2024-07-05T13:57:04Z

electrum_cmd_util.py

+      electrum.constants.set_regtest()
+      # default server add in regtest in electrum is 127.0.0.1
+      # this allows us to replace 127.0.0.1 with REGTEST_SERVER
+      regtest_server = os.environ.get('REGTEST_SERVER')


did you get REGTEST_SERVER from docker-compose ?
can you explain what does this mean inside docker-compose:
REGTEST_SERVER=host.docker.internal

does it using an external host or it is internal ?? what is the ip it is using ?

did you get REGTEST_SERVER from docker-compose ?

yes

REGTEST_SERVER is basically a env variable containing to regtest address . while running without docker you would most probably have regtest server running on 127.0.0.1, depending on docker networking mode that might not be the case for our containers. Now, using this variable we can easily configure regetest server address.
Here we have set it to host.docker.internal to point to docker's docker container's localhost

ErfanMN · 2024-07-05T14:10:52Z

wallet_service_api.py

I was thinking, don't we need this cancel operation in cli mode like what you implement in api mode ?

ark4579 and others added 12 commits April 17, 2024 18:13

added pylint and pyaf linting and formating settings and formatted co…

12da3b9

…debase. also setup vscode settings.json file.

minor EOF line fixes

527f0cb

minor change to vscode settings

4d487da

Merge pull request #1 from ARK4579/ark/formatting-updates

29379e2

formatting updates

added use_regtest flag to regtest net support in wallet_service

41c87b8

Merge pull request #2 from ARK4579/ark/add_regtest_support

8dc52ba

Add regtest net support

docker compose and related files added

4d94d58

added comments to docker file

75630a1

updated readme file to generate and send btc to wallet 0 in wallet_se…

5059bb3

…rvice

updated readme.md

1504f83

Merge pull request #3 from ARK4579/ark/docker-compose-setup

9f9faac

docker compose setup

added cancel transaction API

da3cc04

ARK4579 changed the title ~~Add Cancel Transaction API~~ [DEP] Add Cancel Transaction API Apr 27, 2024

added db file sync to container volumes

d312493

ErfanMN reviewed Jul 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DEP] Add Cancel Transaction API #23

[DEP] Add Cancel Transaction API #23

ARK4579 commented Apr 27, 2024 •

edited

Loading

ErfanMN commented Jul 5, 2024

ARK4579 commented Jul 5, 2024

ErfanMN left a comment

ErfanMN Jul 5, 2024

ARK4579 Jul 5, 2024

ErfanMN Jul 5, 2024

ErfanMN Jul 5, 2024

ErfanMN Jul 5, 2024

ErfanMN Jul 5, 2024

ErfanMN Jul 5, 2024

ErfanMN Jul 5, 2024 •

edited

Loading

ARK4579 Jul 5, 2024 •

edited

Loading

ErfanMN Jul 5, 2024 •

edited

Loading

[DEP] Add Cancel Transaction API #23

Are you sure you want to change the base?

[DEP] Add Cancel Transaction API #23

Conversation

ARK4579 commented Apr 27, 2024 • edited Loading

DEP:

ErfanMN commented Jul 5, 2024

ARK4579 commented Jul 5, 2024

ErfanMN left a comment

Choose a reason for hiding this comment

ErfanMN Jul 5, 2024

Choose a reason for hiding this comment

ARK4579 Jul 5, 2024

Choose a reason for hiding this comment

ErfanMN Jul 5, 2024

Choose a reason for hiding this comment

ErfanMN Jul 5, 2024

Choose a reason for hiding this comment

ErfanMN Jul 5, 2024

Choose a reason for hiding this comment

ErfanMN Jul 5, 2024

Choose a reason for hiding this comment

ErfanMN Jul 5, 2024

Choose a reason for hiding this comment

ErfanMN Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

ARK4579 Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

ErfanMN Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

ARK4579 commented Apr 27, 2024 •

edited

Loading

ErfanMN Jul 5, 2024 •

edited

Loading

ARK4579 Jul 5, 2024 •

edited

Loading

ErfanMN Jul 5, 2024 •

edited

Loading