Add cancel_status param to the cancel order action #1326

aftermath2 · 2024-06-15T02:28:05Z

What does this PR do?

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

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.

api/logics.py

frontend/src/components/TradeBox/index.tsx

Reckless-Satoshi

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.

Reckless-Satoshi · 2024-06-16T21:25:56Z

docs/assets/schemas/api-latest.yaml

@@ -469,6 +469,10 @@ paths:
            - `17` - Maker lost dispute
            - `18` - Taker lost dispute

+            The client can use `cancel_status` to cancel the order only


This doc is unfortunately auto-generated when running test_trade_pipeline, so these strings here will likely be lost I believe. Maybe I am wrong. I think we should add these helpful comments on oas_schemas.py just as #1325 and then the autogenerated api-latest.yaml will always have this text.

Yes it is auto generated in tests/test_api.py at the start of the tests. Also without ENUM_NAME_OVERRIDES and with this change api-latest.yaml gets really polluted :)

Reckless-Satoshi · 2024-06-16T21:36:49Z

api/logics.py

@@ -1009,10 +1009,17 @@ def cancel_order(cls, order, user, state=None):
        if order.status in do_not_cancel:
            return False, {"bad_request": "You cannot cancel this order"}

-        # 1) When maker cancels before bond
+        # 1) If the order status is not the one specified by the user, do not cancel the order.


Best to leave this "do not cancel` comments without enumeration. That is, remove the "1)" , and leave all other cases with the number they had. This is consistent with the first cancel rejection case just above this one:

if order.status in do_not_cancel: return False, {"bad_request": "You cannot cancel this order"}

The intention of enumerating was to have a clear map of the different cancellation conditions and bond slashing because they are all unique in some way.

Reckless-Satoshi · 2024-06-16T21:37:15Z

api/logics.py

        # The order never shows up on the book and order
        # status becomes "cancelled"
-        if order.status == Order.Status.WFB and order.maker == user:
+        elif order.status == Order.Status.WFB and order.maker == user:


let's leave this as it was for simplicity.

Reckless-Satoshi · 2024-06-16T21:39:14Z

api/logics.py

+        if cancel_status is not None and order.status != cancel_status:
+            return False, {
+                "bad_request": f"Order status {order.status} is not {cancel_status}. " +
+                "The order may have been taken before it was cancelled"


This message "The order may have been taken before it was cancelled" only applies to a very specific case of order.status != cancel_status . Maybe best remove it. Very often this bad_request will be returned because of a check between order status that is not public != taken.

Yes agree, also I think it is better to put this check before the if order.status in do_not_cancel like in my pull request, because otherwise if the client has a wrong cancel_status, it gets returned "You cannot cancel this trade", but this is referring to the correct order status. It may confuse a bit the client, it is better to first check if the cancel_status is correct.

tests/utils/trade.py

jerryfletcher21 · 2024-06-16T22:54:56Z

@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 · 2024-06-16T22:59:59Z

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 · 2024-06-17T14:21:46Z

@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 #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.

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

aftermath2 · 2024-06-17T14:36:17Z

robosats/middleware.py

@@ -8,7 +8,6 @@
 from django.utils.deprecation import MiddlewareMixin
 from django.http import JsonResponse
 from rest_framework.authtoken.models import Token
-from rest_framework.exceptions import AuthenticationFailed


Removed this line to fix the lint check error

robosats/middleware.py:11:39: F401 [*] `rest_framework.exceptions.AuthenticationFailed` imported but unused

because the import is not used, but it failed with the same message in the run of the commit that removed it 🤔

It passed locally, may it need a re-run?

It's a linting error that made it to main on a merge conflict resolution. Should now be fine if this branch is updated.

Reckless-Satoshi · 2024-06-17T21:19:24Z

@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 · 2024-06-17T21:23:08Z

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 · 2024-06-17T21:24:38Z

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

aftermath2 · 2024-06-18T21:52:03Z

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 · 2024-06-29T14:37:09Z

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

Reckless-Satoshi · 2024-07-07T12:03:08Z

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 · 2024-07-07T13:47:33Z

@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 · 2024-09-29T23:17:08Z

@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.

aftermath2 mentioned this pull request Jun 15, 2024

POST /api/order cancel should include current status #1311

Open

aftermath2 commented Jun 15, 2024

View reviewed changes

api/logics.py Show resolved Hide resolved

aftermath2 commented Jun 15, 2024

View reviewed changes

frontend/src/components/TradeBox/index.tsx Outdated Show resolved Hide resolved

aftermath2 force-pushed the add_order_cancel_status branch 2 times, most recently from f6ca9c4 to a45355a Compare June 16, 2024 13:54

Reckless-Satoshi requested changes Jun 16, 2024

View reviewed changes

Reckless-Satoshi mentioned this pull request Jun 16, 2024

add new param current_status to /api/order cancel #1325

Closed

1 task

aftermath2 force-pushed the add_order_cancel_status branch from 5a3d63f to a9bf7f3 Compare June 17, 2024 14:12

aftermath2 commented Jun 17, 2024

View reviewed changes

aftermath2 force-pushed the add_order_cancel_status branch from 237682c to eb692bd Compare June 17, 2024 14:49

aftermath2 requested a review from Reckless-Satoshi June 17, 2024 17:38

aftermath2 force-pushed the add_order_cancel_status branch 2 times, most recently from ad0af4c to 6840242 Compare June 18, 2024 21:01

aftermath2 force-pushed the add_order_cancel_status branch from 6840242 to 27f1129 Compare June 29, 2024 14:15

aftermath2 force-pushed the add_order_cancel_status branch from 27f1129 to 2bb4891 Compare July 7, 2024 13:42

Reckless-Satoshi force-pushed the main branch 2 times, most recently from 9054b87 to f9244e1 Compare September 10, 2024 01:28

aftermath2 force-pushed the add_order_cancel_status branch from 2bb4891 to 2d8d529 Compare September 29, 2024 23:11

aftermath2 force-pushed the add_order_cancel_status branch from 2d8d529 to 58f5ee1 Compare November 12, 2024 00:59

aftermath2 force-pushed the add_order_cancel_status branch 3 times, most recently from 72c9e74 to 1d24380 Compare November 12, 2024 01:10

aftermath2 added 2 commits November 11, 2024 19:11

Add cancel_status to POST /api/order

99ecf54

Set cancel_status if it's not None

ec0356f

aftermath2 force-pushed the add_order_cancel_status branch from 1d24380 to ec0356f Compare November 12, 2024 01:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cancel_status param to the cancel order action #1326

Add cancel_status param to the cancel order action #1326

aftermath2 commented Jun 15, 2024 •

edited

Loading

Reckless-Satoshi left a comment •

edited

Loading

Reckless-Satoshi Jun 16, 2024

jerryfletcher21 Jun 16, 2024 •

edited

Loading

Reckless-Satoshi Jun 16, 2024

Reckless-Satoshi Jun 16, 2024

Reckless-Satoshi Jun 16, 2024

jerryfletcher21 Jun 16, 2024 •

edited

Loading

jerryfletcher21 commented Jun 16, 2024 •

edited

Loading

jerryfletcher21 commented Jun 16, 2024 •

edited

Loading

aftermath2 commented Jun 17, 2024 •

edited

Loading

aftermath2 Jun 17, 2024 •

edited

Loading

Reckless-Satoshi Jun 17, 2024

Reckless-Satoshi commented Jun 17, 2024

Reckless-Satoshi commented Jun 17, 2024

Reckless-Satoshi commented Jun 17, 2024

aftermath2 commented Jun 18, 2024

aftermath2 commented Jun 29, 2024

Reckless-Satoshi commented Jul 7, 2024

aftermath2 commented Jul 7, 2024

aftermath2 commented Sep 29, 2024

Add cancel_status param to the cancel order action #1326

Are you sure you want to change the base?

Add cancel_status param to the cancel order action #1326

Conversation

aftermath2 commented Jun 15, 2024 • edited Loading

What does this PR do?

Checklist before merging

Tests

Reckless-Satoshi left a comment • edited Loading

Choose a reason for hiding this comment

Reckless-Satoshi Jun 16, 2024

Choose a reason for hiding this comment

jerryfletcher21 Jun 16, 2024 • edited Loading

Choose a reason for hiding this comment

Reckless-Satoshi Jun 16, 2024

Choose a reason for hiding this comment

Reckless-Satoshi Jun 16, 2024

Choose a reason for hiding this comment

Reckless-Satoshi Jun 16, 2024

Choose a reason for hiding this comment

jerryfletcher21 Jun 16, 2024 • edited Loading

Choose a reason for hiding this comment

jerryfletcher21 commented Jun 16, 2024 • edited Loading

jerryfletcher21 commented Jun 16, 2024 • edited Loading

aftermath2 commented Jun 17, 2024 • edited Loading

aftermath2 Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Reckless-Satoshi Jun 17, 2024

Choose a reason for hiding this comment

Reckless-Satoshi commented Jun 17, 2024

Reckless-Satoshi commented Jun 17, 2024

Reckless-Satoshi commented Jun 17, 2024

aftermath2 commented Jun 18, 2024

aftermath2 commented Jun 29, 2024

Reckless-Satoshi commented Jul 7, 2024

aftermath2 commented Jul 7, 2024

aftermath2 commented Sep 29, 2024

aftermath2 commented Jun 15, 2024 •

edited

Loading

Reckless-Satoshi left a comment •

edited

Loading

jerryfletcher21 Jun 16, 2024 •

edited

Loading

jerryfletcher21 Jun 16, 2024 •

edited

Loading

jerryfletcher21 commented Jun 16, 2024 •

edited

Loading

jerryfletcher21 commented Jun 16, 2024 •

edited

Loading

aftermath2 commented Jun 17, 2024 •

edited

Loading

aftermath2 Jun 17, 2024 •

edited

Loading