Add cancel_status param to the cancel order action

[aftermath2]

What does this PR do?

Fixes #1311

This PR adds the cancel_status parameter to the POST /api/order endpoint for users to specify the status in which an order should be for the cancellation to take place.

Checklist before merging

• [x] Install pre-commit and initialize it: pip install pre-commit, then pre-commit install. Pre-commit installs git hooks that automatically check the codebase. If pre-commit fails when you commit your changes, please fix the problems it points out.

Tests

test_cancel_order_cancel_status

aftermath:~ $ docker exec test-coordinator coverage run manage.py test tests.test_trade_pipeline.TradeTest.test_cancel_order_cancel_status
Creating test database for alias 'default'...
Found 1 test(s).
System check identified no issues (0 silenced).
Regtest network was already ready. Skipping initalization.
.
----------------------------------------------------------------------
Ran 1 test in 5.832s

OK
Destroying test database for alias 'default'...

test_cancel_order_different_cancel_status

aftermath:~ $ docker exec test-coordinator coverage run manage.py test tests.test_trade_pipeline.TradeTest.test_cancel_order_different_cancel_status
Creating test database for alias 'default'...
Found 1 test(s).
System check identified no issues (0 silenced).
.
----------------------------------------------------------------------
Ran 1 test in 5.898s

OK
Destroying test database for alias 'default'...
Regtest network was already ready. Skipping initalization.

Some of the tests that weren't modified and are not affected by the changes failed, but I think they should be fixed in a different PR.

[jerryfletcher21]

@Reckless-Satoshi I think that tests/api_specs.yaml was auto generated in the past but it is no longer used anywhere, I can not see any reference to it in the code, and it is also quite outdated (v0.5.3), I think it can be removed?

[jerryfletcher21]

Looks good! I left a few comments for some minor changes.

We can get inspiration from #1325 for 1) the fewer/simpler changes to logics.py, 2) the addition of comments into oas_schemas.py and 3) ENUM_NAME_OVERRIDES as this hack leads into a more complete api-latest generated schema.

Thank you for implementing the test cases, this is very important.

We will merge this PR in favor of #1325 but both of them are technically good, so what if we split a bit the rewards? 70K for this PR and 40K for #1325.

Too many #1325 :) But I understand, perfectly fine with me.

[aftermath2]

@Reckless-Satoshi Just applied the changes suggested, let me know if there's anything else I should modify 👍

We will merge this PR in favor of https://github.com/RoboSats/robosats/pull/1325 but both of them are technically good, so what if we split a bit the rewards? 70K for this PR and 40K for https://github.com/RoboSats/robosats/pull/1325.

That's totally fine with me, thanks @jerryfletcher21 for your contributions!

[Reckless-Satoshi]

@Reckless-Satoshi I think that tests/api_specs.yaml was auto generated in the past but it is no longer used anywhere, I can not see any reference to it in the code, and it is also quite outdated (v0.5.3), I think it can be removed?

Yes, in fact, I removed it from main yesterday as I also realized this fact :)

[Reckless-Satoshi]

Too many #1325 :) But I understand, perfectly fine with me.

Thank you, I actually don't know how to handle this situation 😅😂 Kudos is yours for thinking about sending the expected status. 😁

[Reckless-Satoshi]

I'll need to test the frontend part before we merge. Will likely not be able until sunday.

[aftermath2]

Update: rebased the branch to pull the changes from main and run the lint check again.

I'll need to test the frontend part before we merge. Will likely not be able until sunday.

Alright, no problem 👍

[aftermath2]

Update: rebased to the updated base branch and fixed a conflict in the tests/test_trade_pipeline.py file.

[Reckless-Satoshi]

I might be doing something wrong here. Currently 17 tests are failing for me

======================================================================
ERROR: test_withdraw_reward_after_unilateral_cancel (tests.test_trade_pipeline.TradeTest.test_withdraw_reward_after_unilateral_cancel)
Tests withdraw rewards as taker after maker cancels order unilaterally
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/src/robosats/tests/test_trade_pipeline.py", line 1226, in test_withdraw_reward_after_unilateral_cancel
    trade.cancel_order(trade.maker_index)
  File "/usr/local/lib/python3.12/unittest/mock.py", line 1390, in patched
    return func(*newargs, **newkeywargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/robosats/tests/utils/trade.py", line 120, in cancel_order
    self.response = self.client.post(path + params, body, **headers)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/rest_framework/test.py", line 296, in post
    response = super().post(
               ^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/rest_framework/test.py", line 209, in post
    data, content_type = self._encode_data(data, format, content_type)
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/rest_framework/test.py", line 180, in _encode_data
    ret = renderer.render(data)
          ^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/rest_framework/renderers.py", line 916, in render
    return encode_multipart(self.BOUNDARY, data)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/test/client.py", line 300, in encode_multipart
    raise TypeError(
TypeError: Cannot encode None for key 'cancel_status' as POST data. Did you mean to pass an empty string or omit the value

[aftermath2]

@Reckless-Satoshi That was caused by what we discussed here. I may have executed the tests with a cached environment so I didn't notice the error after I changed it (my bad). I've just updated that part of the code so it sets the cancel_status only if it's not None.

[aftermath2]

@Reckless-Satoshi Just rebased, solved some conflicts and ran the tests (results in the description) to validate that everything works as expected. Please let me know if there's anything else I should do from my side to get this merged.

[MrImmortal09]

@aftermath2 are you still working on this?

[aftermath2]

@MrImmortal09 Yes, just waiting for it to be reviewed 👍

[KoalaSat]

@aftermath2 Seems like tests still fails

======================================================================
ERROR: test_cancel_order_cancel_status (tests.test_trade_pipeline.TradeTest.test_cancel_order_cancel_status)
Tests the cancellation of a public order using cancel_status.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/src/robosats/tests/test_trade_pipeline.py", line 1124, in test_cancel_order_cancel_status
    data["bad_request"], "This order has been cancelled by the maker"
    ~~~~^^^^^^^^^^^^^^^
KeyError: 'bad_request'

----------------------------------------------------------------------
Ran 1 test in 5.783s

[aftermath2]

@KoalaSat There was a behavior change that caused the data variable to not hold the field bad_request. I changed it to access the bad request message like other tests do and now both of the ones added run successfully.

aftermath:~ $ docker exec test-coordinator coverage run manage.py test tests.test_trade_pipeline.TradeTest.test_cancel_order_cancel_status
Creating test database for alias 'default'...
Found 1 test(s).
System check identified no issues (0 silenced).
Updating order with new Locked bond from maker
Regtest network was already ready. Skipping initalization.
2025-03-30 00:11:41.415953+00:00
{'num_active_invoices': 1, 'invoices': [{0: {'payment_hash': '92c0f53024e789e7506bc8041d1105395116f06fbbc2f680cd2c30df428d93c8', 'old_status': 'Generated', 'new_status': 'Locked'}}], 'time': 0.04764270782470703}
.
----------------------------------------------------------------------
Ran 1 test in 4.290s

OK
Destroying test database for alias 'default'...
aftermath:~ $ docker exec test-coordinator coverage run manage.py test tests.test_trade_pipeline.TradeTest.test_cancel_order_different_cancel_status
Creating test database for alias 'default'...
Found 1 test(s).
System check identified no issues (0 silenced).
Updating order with new Locked bond from maker
Regtest network was already ready. Skipping initalization.
2025-03-30 00:12:04.577482+00:00
{'num_active_invoices': 1, 'invoices': [{0: {'payment_hash': '1b378e15fbf2e41792c489fb7a0687e0eaa5a839327a62145af31d83d6e1739f', 'old_status': 'Generated', 'new_status': 'Locked'}}], 'time': 0.04600787162780762}
.
----------------------------------------------------------------------
Ran 1 test in 4.962s

OK
Destroying test database for alias 'default'...

My bad, I thought they would pass because there were no functional changes since the last run (PR desc), but I should have double checked it.

There are two other tests failing test_basic_frontend_url_content and test_pro_frontend_url_content but they are not related to the changes from this PR.

[KoalaSat]

@KoalaSat There was a behavior change that caused the data variable to not hold the field bad_request. I changed it to access the bad request message like other tests do and now both of the ones added run successfully.

aftermath:~ $ docker exec test-coordinator coverage run manage.py test tests.test_trade_pipeline.TradeTest.test_cancel_order_cancel_status
Creating test database for alias 'default'...
Found 1 test(s).
System check identified no issues (0 silenced).
Updating order with new Locked bond from maker
Regtest network was already ready. Skipping initalization.
2025-03-30 00:11:41.415953+00:00
{'num_active_invoices': 1, 'invoices': [{0: {'payment_hash': '92c0f53024e789e7506bc8041d1105395116f06fbbc2f680cd2c30df428d93c8', 'old_status': 'Generated', 'new_status': 'Locked'}}], 'time': 0.04764270782470703}
.
----------------------------------------------------------------------
Ran 1 test in 4.290s

OK
Destroying test database for alias 'default'...
aftermath:~ $ docker exec test-coordinator coverage run manage.py test tests.test_trade_pipeline.TradeTest.test_cancel_order_different_cancel_status
Creating test database for alias 'default'...
Found 1 test(s).
System check identified no issues (0 silenced).
Updating order with new Locked bond from maker
Regtest network was already ready. Skipping initalization.
2025-03-30 00:12:04.577482+00:00
{'num_active_invoices': 1, 'invoices': [{0: {'payment_hash': '1b378e15fbf2e41792c489fb7a0687e0eaa5a839327a62145af31d83d6e1739f', 'old_status': 'Generated', 'new_status': 'Locked'}}], 'time': 0.04600787162780762}
.
----------------------------------------------------------------------
Ran 1 test in 4.962s

OK
Destroying test database for alias 'default'...

My bad, I thought they would pass because there were no functional changes since the last run (PR desc), but I should have double checked it.

There are two other tests failing test_basic_frontend_url_content and test_pro_frontend_url_content but they are not related to the changes from this PR.

thank you :smiley: