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.

Sign up for GitHub

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

Jump to bottom

Improve TWAP order mapping with `activeOrderUid` #1785

Merged

iamacook merged 6 commits into main from large-twap-order-status

Jul 30, 2024

Member

iamacook commented Jul 24, 2024 •

edited

Loading

Summary

In order to cancel a TWAP, the current order UID needs to be referenced. As we calculate each of these, it makes sense to return it as part of the transaction info (as well as at the request of the client).

This adds a new activeOrderUid to the TwapOrderTransactionInfo. On top of this, as we were previously calculating the status based on the initial 10 parts of a TWAP, or the last for those with >10, we now fetch the status from the active part. This fixes the issue whereby large part TWAPs were returning the incorrect status - final parts are not created meaning they would always throw.

Changes

Add activeOrderUid to TwapOrderTransactionInfo.
Fetch status relative to activeOrderUid.
Add appropriate test coverage.


          Mark TWAP orders as unknown if getting part orders fails

88c9d4e

iamacook self-assigned this

iamacook requested a review from a team as a code owner

July 24, 2024 15:01

coveralls commented Jul 24, 2024 •

edited

Loading

Pull Request Test Coverage Report for Build 10112332202

Details

2 of 45 (4.44%) changed or added relevant lines in 3 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.2%) to 48.248%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/routes/transactions/entities/confirmation-view/confirmation-view.entity.ts	1	2	50.0%
src/routes/transactions/entities/swaps/twap-order-info.entity.ts	1	2	50.0%
src/routes/transactions/mappers/common/twap-order.mapper.ts	0	41	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/routes/transactions/transactions-view.service.ts	1	15.85%
src/routes/transactions/mappers/common/twap-order.mapper.ts	1	15.24%

Totals
Change from base Build 10054294132:	-0.2%
Covered Lines:	4328
Relevant Lines:	7215

💛 - Coveralls

iamacook marked this pull request as draft

July 24, 2024 15:34


          Add activeOrderUid

abdb021

iamacook changed the title ~~Mark TWAP orders as unknown if getting part orders fails~~ Improve TWAP order mapping with activeOrderUid

iamacook commented

View reviewed changes

src/routes/transactions/mappers/common/twap-order.mapper.spec.ts

@@ @@ -680,6 +786,7 @@ describe('TwapOrderMapper', () => { @@
                 });
                 it('should map the TWAP order if source apps are restricted and a part order fullAppData matches any of the allowed apps', async () => {
+                  configurationService.set('swaps.maxNumberOfParts', 2);

Member Author

iamacook Jul 25, 2024

This test began failing without this.

Member

hectorgomezv Jul 30, 2024

It's weird, I can't replicate the failure, maybe it's a flaky test for some reason. Or maybe the value is set on another test as the configuration is not cleaned between the tests if I'm not wrong. Anyway, I'm good with setting this explictly here.

iamacook marked this pull request as ready for review

July 25, 2024 07:48

compojoom reviewed

View reviewed changes

src/routes/transactions/mappers/common/twap-order.mapper.ts Outdated

+                  if (args.partOrders) {
+                    const order = args.partOrders.find((order) => order.uid === orderUid);
+                    if (order) {
+                      return order.status;

Contributor

compojoom Jul 25, 2024

Maybe I'm misunderstanding this, but if you just return the order status, wouldn't that be wrong for "fulfilled" orders? If the status is fulfilled for one part, it doesn't mean that this applies to the whole TWAP.

Member Author

iamacook Jul 25, 2024

The logic regarding status generation is different as of #1785 (comment) which should now solve this.

schmanu reviewed

View reviewed changes

src/routes/transactions/mappers/common/twap-order.mapper.ts Outdated

+                    const order = await this.swapsRepository.getOrder(args.chainId, orderUid);
+                    // If the active order is fulfilled, the next one may not yet exist
+                    // and fetching it would fail so we assume it's open
+                    if (args.activeOrderUid && order.status === OrderStatus.Fulfilled) {

Member

schmanu Jul 25, 2024

Whenever this order exists we have to return the total twap order status as Open.
Even if the activePart is still open or even if it got cancelled (as it is possible to cancel a single part without cancelling the whole order)

Member Author

iamacook Jul 25, 2024

As per discussion, we now assume open, fullfilled or cancelled depending on the existince of an active/final order part as of dfea594.

iamacook added 2 commits

July 25, 2024 10:45


          Adjust status calculation logic

dfea594


          Fix lint

3734ad1

iamacook requested review from schmanu and compojoom

July 25, 2024 08:50

schmanu reviewed

View reviewed changes

src/routes/transactions/mappers/common/twap-order.mapper.ts Outdated

+                    // If the order was created, the TWAP is either open or fulfilled depending
+                    // on whether the order part is active
+                    return args.activeOrderUid ? OrderStatus.Open : OrderStatus.Fulfilled;

Member

schmanu Jul 25, 2024

I think there is one more edge case we missed when we talked about this:
If the activeOrderUid is the last part.

activeOrderUid === this.gpv2OrderHelper.computeOrderUid({
        chainId: args.chainId,
        owner: args.safeAddress,
        order: args.twapParts.slice(-1)[0], // Last part's order
      });

In that case we could return Fulfilled if the order is already in a final state (fulfilled, cancelled).

Member Author

iamacook Jul 25, 2024

Added in 686505d.


          Check if active part is final part

686505d

iamacook requested a review from schmanu

July 25, 2024 11:44

schmanu reviewed

View reviewed changes

src/routes/transactions/mappers/common/twap-order.mapper.ts Outdated

+                  });
+                  // If active order is final one, it's fulfilled as it exists
+                  if (args.activeOrderUid === finalOrderUid) {

Member

schmanu Jul 26, 2024

I think we still have to fetch in order to know that it exists.
twapParts and activeOrderUid are both just computed here. So if the last twap would be active this will always be true.

Member Author

iamacook Jul 26, 2024

Adjusted in f20a158.

src/routes/transactions/mappers/common/twap-order.mapper.ts Outdated

+                    // If the order exists, the TWAP is either open or fulfilled depending
+                    // on whether the order is active
+                    return args.activeOrderUid ? OrderStatus.Open : OrderStatus.Fulfilled;

Member

schmanu Jul 26, 2024

I think we could remove the above check (where I commented) and instead do something like this here:

Suggested change

      
                  return args.activeOrderUid ? OrderStatus.Open : OrderStatus.Fulfilled;
          
                  return args.activeOrderUid ? args.activeOrderUid === finalOrderUid ? OrderStatus.Fulfilled : OrderStatus.Open : OrderStatus.Fulfilled;

Member Author

iamacook Jul 26, 2024

Adjusted in f20a158.

src/routes/transactions/mappers/common/twap-order.mapper.spec.ts

@@ @@ -841,6 +948,7 @@ describe('TwapOrderMapper', () => { @@
                   });
                   expect(result).toEqual({
+                    activeOrderUid: null,
                     buyAmount: '51576509680023161648',
                     buyToken: {
                       address: buyToken.address,

Member

schmanu Jul 26, 2024 •

edited

Loading

Should we add these (or similar) testcases for the Order Status mapping?

Cancelled Order with 3 parts where only the first part exists and the second part is currently active.
Cancelled ORder with 3 parts where only the first 2 parts exist and the last part is currently active (This would be a case that should currently fail)
Fulfilled Order with 3 parts where the last part is currently active and the repository returns fulfilled for it.

Member Author

iamacook Jul 26, 2024

Added in f20a158.

iamacook marked this pull request as draft

July 26, 2024 09:31


          Check if final order exists

f20a158

iamacook commented

View reviewed changes

src/routes/transactions/mappers/common/twap-order.mapper.spec.ts

Comment on lines +433 to +440

+                  mockSwapsRepository.getOrder.mockImplementation(
+                    async (_chainId: string, orderUid: string) => {
+                      if (orderUid === part2.uid) {
+                        return Promise.resolve(part2);
+                      }
+                      return Promise.reject(new NotFoundException());
+                    },
+                  );

Member Author

iamacook Jul 26, 2024

I encountered issues in the new test cases that were due to incorrectly calculated order UIDs. As such, I adjusted these mocks to be more specific.

iamacook requested a review from schmanu

July 26, 2024 13:48

iamacook marked this pull request as ready for review

July 26, 2024 13:48

schmanu approved these changes

View reviewed changes

Member

schmanu left a comment

Looks correct to me :) Thanks for going through those edge cases 🥇

hectorgomezv approved these changes

View reviewed changes

src/routes/transactions/mappers/common/twap-order.mapper.spec.ts

@@ @@ -680,6 +786,7 @@ describe('TwapOrderMapper', () => { @@
                 });
                 it('should map the TWAP order if source apps are restricted and a part order fullAppData matches any of the allowed apps', async () => {
+                  configurationService.set('swaps.maxNumberOfParts', 2);

Member

hectorgomezv Jul 30, 2024

It's weird, I can't replicate the failure, maybe it's a flaky test for some reason. Or maybe the value is set on another test as the configuration is not cleaned between the tests if I'm not wrong. Anyway, I'm good with setting this explictly here.

iamacook merged commit 1519f67 into main

16 checks passed

iamacook deleted the large-twap-order-status branch

July 30, 2024 15:13

DenSmolonski pushed a commit to Zilliqa/safe-client-gateway that referenced this pull request


          Improve TWAP order mapping with activeOrderUid (safe-global#1785)

fc89ac2

Adds a new `activeOrderUid` to the `TwapOrderTransactionInfo`, calculating the status from it:

- Add `activeOrderUid` to `TwapOrderTransactionInfo`.
- Fetch status relative to `activeOrderUid`.
- Add appropriate test coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet